Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix table alias location tracking #158

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

gabotechs
Copy link

First of all, thanks for this awesome project!

I spotted a bug with the location tracking where table aliases are not tracked if the AS keyword is omitted, for example:

  • with the AS keyword.
SELECT * FROM test as t
              |_______| <- in this case, the tracking works correctly
  • without the AS keyword.
SELECT * FROM test t
              |__| <- in this case, the tracking omits the ` t` part

I have proposed a solution that makes the test pass with a minimal change, but I'm not sure if it's the best approach, as I'm unfamiliar with the codebase, so I would really appreciate suggestions.

ident_aliased -> (%kw_as ident {% last %}) | ident {% unwrap %}
ident_aliased -> (%kw_as ident {% last %}) | ident {% last %}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestions on this? my only reasoning for using last here is because, unlike unwrap, it handles tracking under the hood.

Comment on lines +1064 to +1084
checkSelectLoc('select * from test as t', {
_location: { start: 0, end: 23 },
type: 'select',
from: [{
_location: { start: 14, end: 23 },
type: 'table',
name: {
_location: { start: 14, end: 23 },
name: 'test',
alias: 't',
}
}],
columns: [{
_location: { start: 7, end: 8 },
expr: {
_location: { start: 7, end: 8 },
type: 'ref',
name: '*',
}
}]
});
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test passes on master and on this branch

Comment on lines +1086 to +1106
checkSelectLoc('select * from test t', {
_location: { start: 0, end: 20 },
type: 'select',
from: [{
_location: { start: 14, end: 20 },
type: 'table',
name: {
_location: { start: 14, end: 20 },
name: 'test',
alias: 't',
},
}],
columns: [{
_location: { start: 7, end: 8 },
expr: {
_location: { start: 7, end: 8 },
type: 'ref',
name: '*',
},
}]
});
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test fails on master. The end positions there are 18 instead of 20, because the t part is omitted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant