-
Notifications
You must be signed in to change notification settings - Fork 686
fix(duckdb): implement IGNORE NULLS for FirstValue and LastValue #11763
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
base: main
Are you sure you want to change the base?
fix(duckdb): implement IGNORE NULLS for FirstValue and LastValue #11763
Conversation
b1ed9f6 to
c14cdbb
Compare
c14cdbb to
c69158e
Compare
| op.name, table=table.alias_or_name, quoted=self.quoted, copy=False | ||
| ) | ||
|
|
||
| def visit_FirstLastValue(self, op, *, arg, include_null): |
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.
I would prefer more verbose but less tricky, and just list them separately.
Also, currently handling include_null is opt-in: the base compiler silently ignores that arg, so individual backends like duckdb need to implement it themselves. This leads to the risk that a backend that DOES handle IGNORE NULL will forget to do it (eg pyspark, as in this PR?) and we will get incorrect behavior.
I would prefer to handle the include_null at the SqlGlotCompiler level and then deriving compilers to opt-out. Then users will get an error, instead of the incorrect result, which I think is a better outcome.
So can you change this bit to this:
def visit_FirstValue(self, op, *, arg, include_null):
if include_null:
return sge.RespectNulls(this=self.f.first_value(arg))
else:
return sge.IgnoreNulls(this=self.f.first_value(arg))
def visit_LastValue(self, op, *, arg, include_null):
if include_null:
return sge.RespectNulls(this=self.f.last_value(arg))
else:
return sge.IgnoreNulls(this=self.f.last_value(arg))and then I'll put some other comments in the other files for the related changes
|
See #11311, where I tried to implement this. I'm not sure which version we should abandon and which we should pursue, but in general I think
This PR has inspired me to resurrect that PR, once that test is passing we can have two options to choose between |
|
Also related, I'm not sure if this unsupported error should get pushed down in sqlglot: tobymao/sqlglot#6376 and then possibly surfaced in ibis automatically: #11772 |
Description of changes
Reported in #11726, this PR set the base changes for FirstValue and LastValue. Namely:
include_nullfield to both classesfirst_to_firstvalueto account for the new fieldOnce this change gets merged, the idea is to add the implementation of at least the following backends (progressively):
Issues closed
N/A