-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat(snowflake)!: fix input rounding issue when transpiling boolean logic functions from Snowflake to DuckDB #6849
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
feat(snowflake)!: fix input rounding issue when transpiling boolean logic functions from Snowflake to DuckDB #6849
Conversation
…m snowflake to duckDB
SQLGlot Integration Test ResultsComparing:
By Dialect
Overallmain: 5697 total, 5465 passed (pass rate: 95.9%), sqlglot version: sqlglot:snowflake_to_duckdb_boolean_function_rounding: 5697 total, 5465 passed (pass rate: 95.9%), sqlglot version: Difference: No change |
geooo109
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, and also great catch!
| def _boolnot_sql(self: DuckDB.Generator, expression: exp.Boolnot) -> str: | ||
| arg = expression.args.get("this") | ||
| if expression.args.get("round_input"): | ||
| arg = self.func("ROUND", arg, exp.Literal.number(0)) | ||
| return f"NOT ({self.sql(arg)})" | ||
|
|
||
|
|
||
| def _booland_sql(self: DuckDB.Generator, expression: exp.Booland) -> str: | ||
| left = expression.args.get("this") | ||
| right = expression.args.get("expression") | ||
| if expression.args.get("round_input"): | ||
| left = self.func("ROUND", left, exp.Literal.number(0)) | ||
| right = self.func("ROUND", right, exp.Literal.number(0)) | ||
| return f"(({self.sql(left)}) AND ({self.sql(right)}))" | ||
|
|
||
|
|
||
| def _boolor_sql(self: DuckDB.Generator, expression: exp.Boolor) -> str: | ||
| left = expression.args.get("this") | ||
| right = expression.args.get("expression") | ||
| if expression.args.get("round_input"): | ||
| left = self.func("ROUND", left, exp.Literal.number(0)) | ||
| right = self.func("ROUND", right, exp.Literal.number(0)) | ||
| return f"(({self.sql(left)}) OR ({self.sql(right)}))" | ||
|
|
||
|
|
||
| def _xor_sql(self: DuckDB.Generator, expression: exp.Xor) -> str: | ||
| left = expression.args.get("this") | ||
| right = expression.args.get("expression") | ||
| if expression.args.get("round_input"): | ||
| left = self.func("ROUND", left, exp.Literal.number(0)) | ||
| right = self.func("ROUND", right, exp.Literal.number(0)) | ||
| return f"({self.sql(left)} AND (NOT {self.sql(right)})) OR ((NOT {self.sql(left)}) AND {self.sql(right)})" | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use the exp.Round for the call of the rounding function.
Moreover, let's use the expression builders for OR, AND, NOT, Paren.
We can also factor out the logic here for the rounding of each arg, and minimze the code of the functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 – we should not be generating SQL by hand in cases like this where binary operators are involved. Instead, construct the proper AST and generate those via self.sql if possible, so that they'll be pretty-formatted when the user sets the option.
georgesittas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed with Geo's feedback, let's clean this up a bit and then we can merge.
|
|
||
|
|
||
| def _boolnot_sql(self: DuckDB.Generator, expression: exp.Boolnot) -> str: | ||
| arg = expression.args.get("this") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| arg = expression.args.get("this") | |
| arg = expression.this |
| left = expression.args.get("this") | ||
| right = expression.args.get("expression") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| left = expression.args.get("this") | |
| right = expression.args.get("expression") | |
| left = expression.this | |
| right = expression.expression |
| left = expression.args.get("this") | ||
| right = expression.args.get("expression") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| left = expression.args.get("this") | |
| right = expression.args.get("expression") | |
| left = expression.this | |
| right = expression.expression |
| left = expression.args.get("this") | ||
| right = expression.args.get("expression") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| left = expression.args.get("this") | |
| right = expression.args.get("expression") | |
| left = expression.this | |
| right = expression.expression |
|
I'll take this to the finish line. |
…functions to DuckDB (#6849) Co-authored-by: Jo <46752250+georgesittas@users.noreply.github.com>
For BOOLNOT, BOOLAND, BOOLOR and BOOLXOR, Snowflake rounds floating point inputs, while duckDB's boolean functions don't do rounding on the inputs. So BOOLAND(-0.4, 5) is evaluated to FALSE (same as BOOLAND(0, 5)) on Snowlfake, while its transpiled DuckDB query ((-0.4) AND (5)) is evaluated to TRUE. We should add rounding to the transpiled duckDB query.
The PR adds this rounding step when transpiling these functions from Snowflake to DuckDB.
Testing
Source query:
Transpiled query:
Both queries produce the same result
TRUE | FALSE | FALSE | TRUE | TRUE
Without this change, the transpiled query produces
false │ false │ true │ true │ false