feat(databricks): support and, or function syntax - #8065
feat(databricks): support and, or function syntax#8065fivetran-amrutabhimsenayachit wants to merge 3 commits into
and, or function syntax#8065Conversation
e6db6ec to
9652c7e
Compare
georgesittas
changed the title
and, or function syntax
georgesittas
left a comment
georgesittas
left a comment
There was a problem hiding this comment.
Is this only relevant to Databricks? What about other dialects in the Hive hierarchy?
| "AND": lambda args: exp.And(this=seq_get(args, 0), expression=seq_get(args, 1)), | ||
| "OR": lambda args: exp.Or(this=seq_get(args, 0), expression=seq_get(args, 1)), |
There was a problem hiding this comment.
What happens if you mix and() and or() w.r.t. operator precedence? Do we respect Databricks' semantics? Try the following:
and(or(true, false), false)
and(false, or (true, true))You should also see how this affects other expressions that involve these function calls, such as:
and(false, false) = false
not and(true, false)There was a problem hiding this comment.
The and() or() function calls is supported by Spark as well apart from Databricks, but it is not supported by HIVE.py. So moving the fix to SparkParser.
There was a problem hiding this comment.
@fivetran-amrutabhimsenayachit did you test the last two cases? I don't think your PR handles them correctly. Similarly for:
or(true, false) and false
and(null, true) is nullThere was a problem hiding this comment.
Moreover, ClickHouse appears to have a similar problem because the logic is the same.
| self, this: exp.Expr, parse_function_unit: bool = True | ||
| ) -> exp.Interval: | ||
| # AND/OR in FUNC_TOKENS would be consumed as interval units; they never are. | ||
| if self._curr and self._curr.token_type in (TokenType.AND, TokenType.OR): |
There was a problem hiding this comment.
Are these the only tokens that can follow an INTERVAL value and be incorrectly consumed as units? Are there any others? If yes, are the others handled correctly in main today?
There was a problem hiding this comment.
Looks like it. I tried running the fusion schema tests and caught this failure.
There was a problem hiding this comment.
I don't think this set is complete, for example this fails to parse in this PR:
SELECT INTERVAL '1 day' LIKE ('a')The reason I commented on this is because it introduces specialized logic for dealing with a token subset and I don't think it scales well to solve the more general issue. Let's just revert this for now, I don't think we should mix this in.
SQLGlot Integration Test Results❌ 2 regressions — see details belowComparing:
By Dialect
Overallmain: 182937 total, 160861 passed (pass rate: 87.9%) sqlglot:fix_parser_databricks_AND_function: 170743 total, 149709 passed (pass rate: 87.7%) Transitions: Dialect pair changes: 0 previous results not found, 3 current results not found ❌ 2 regressions (view logs) |
ee53599 to
b65725f
Compare
b65725f to
ed5415d
Compare
ed5415d to
0240b59
Compare
22a62d9 to
ff0bb2c
Compare
Databricks allows and(a, b) as a function, but SQLGlot's parser only knew AND as a keyword. Adding AND to the function-name whitelist fixed the parsing, and a small override prevented a side-effect where the interval parser mistook AND ( for a function call.