-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Expand wildcard expressions in distinct on #12941
Conversation
1d53a7a
to
d237a93
Compare
d237a93
to
4f73eec
Compare
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.
Looks good to me
Thank you @epsio-banay
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.
Thanks @epsio-banay it looks to me. Just one suggestion:
How about adding an SQL test in datafusion/sqllogictest/test_files/distinct_on.slt
?
It would be nice to ensure the plan won't be affected by other changes.
I verified that before this PR this query fails like this: > create table foo as values (1), (2);
0 row(s) fetched.
> select distinct on (column1) * from foo;
Optimizer rule 'replace_distinct_aggregate' failed
caused by
replace_distinct_aggregate
caused by
Internal error: Failed due to a difference in schemas, original schema: DFSchema { inner: Schema { fields: [Field { name: "column1", data_type: Int64, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }], metadata: {} }, field_qualifiers: [Some(Bare { table: "foo" })], functional_dependencies: FunctionalDependencies { deps: [] } }, new schema: DFSchema { inner: Schema { fields: [Field { name: "column1", data_type: Null, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }], metadata: {} }, field_qualifiers: [Some(Bare { table: "foo" })], functional_dependencies: FunctionalDependencies { deps: [] } }.
This was likely caused by a bug in DataFusion's code and we would welcome that you file an bug report in our issue tracker And after this PR it works
this is an excellent call @goldmedal -- I will make a follow on PR |
PR with SLT tests: #12968 |
Closes #12940
Rationale for this change
What changes are included in this PR?
Are these changes tested?
yes
Are there any user-facing changes?
No