Make SQL parser more BQ compatible #43
Conversation
There was a problem hiding this comment.
parser changes look good minus 2 comments, test looks good. I would say if you decide to get this upstreamed that the test could have a comment linking the JIRA, and the commit message could be a bit more descriptive (making it more compatible how?). otherwise looks great!
| <RPAREN> | ||
| { | ||
| final SqlParserPos pos = s.end(this); | ||
| tableRef = createCall(tableName, pos, |
There was a problem hiding this comment.
here you initialize tableRef by creating a call, and then the next line you reassign its value by wrapping it in another call, i was curious why you did that. to keep the code simple by breaking across two lines? should it be two separate variables?
There was a problem hiding this comment.
I was following the convention of all the other code in the grammar, and the code in this production in particular.
It's really common to have this accumulator pattern:
sqlNode = ParseSomething()
sqlNode = ParseSomethingElse(sqlNode)
sqlNode = ParseAThirdThing(sqlNode)
So I followed the same pattern in the construction of the SqlNode tree here.
But I don't care strongly about it.
barrkel
force-pushed
the
bk-make-parser-more-bq
branch
from
5973d23
to
de3d1b1
Compare
PR created for code review purposes, not for merging purposes.