You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "phillipleblanc (via GitHub)" <gi...@apache.org> on 2024/05/22 16:17:01 UTC
[PR] Phillip/240523 fix mysql order by [datafusion]
phillipleblanc opened a new pull request, #10625:
URL: https://github.com/apache/datafusion/pull/10625
## Which issue does this PR close?
Closes #10624
Closes #.
## Rationale for this change
When unparsing LogicalPlans back to SQL statements for the MySQL dialect, any plan that includes a `Sort` node will produce a SQL statement that looks like:
```
SELECT "ta"."j1_id" FROM "j1" AS "ta" ORDER BY "ta"."j1_id" ASC NULLS LAST LIMIT 10
^--------^
```
The `NULLS LAST` causes issues when run against a MySQL server, as it doesn't recognize that syntax.
## What changes are included in this PR?
Adds a new function to the Unparser Dialect called `supports_nulls_first_in_sort` which is default `true`, but set to `false` for the MySQL dialect. And in the relevant unparsing code, we set the AST node to None.
## Are these changes tested?
Yes, added a new test to cover dialect-specific behavior.
## Are there any user-facing changes?
No breaking changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Omit NULLS FIRST/LAST when unparsing ORDER BY clauses for MySQL [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #10625:
URL: https://github.com/apache/datafusion/pull/10625
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Omit NULLS FIRST/LAST when unparsing ORDER BY clauses for MySQL [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #10625:
URL: https://github.com/apache/datafusion/pull/10625#issuecomment-2126896025
Thanks @phillipleblanc and @backkem
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Omit NULLS FIRST/LAST when unparsing ORDER BY clauses for MySQL [datafusion]
Posted by "backkem (via GitHub)" <gi...@apache.org>.
backkem commented on PR #10625:
URL: https://github.com/apache/datafusion/pull/10625#issuecomment-2126244362
Also, I think it's the idea to move the entire unparser to the SQLParser package eventually.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Omit NULLS FIRST/LAST when unparsing ORDER BY clauses for MySQL [datafusion]
Posted by "phillipleblanc (via GitHub)" <gi...@apache.org>.
phillipleblanc commented on PR #10625:
URL: https://github.com/apache/datafusion/pull/10625#issuecomment-2126820455
I closed the upstream `sqlparser-rs` PR for now, since it is more of a semantic issue vs a syntax issue (thanks @alamb for pointing that out). But now we're back to square one on my dilemma of whether it makes sense to upstream a dialect behavior that isn't used by the parser library itself. I'll think about this some more - also would like to get people's feedback.
So for now I'll just remove the comment on the Unparser Dialect linking to the now closed PR and we can revisit later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Omit NULLS FIRST/LAST when unparsing ORDER BY clauses for MySQL [datafusion]
Posted by "phillipleblanc (via GitHub)" <gi...@apache.org>.
phillipleblanc commented on PR #10625:
URL: https://github.com/apache/datafusion/pull/10625#issuecomment-2126149300
> I wonder if we should update the comments in `Dialect` so that we can upstream the trait once we have some more experience with what features are needed in DataFusion 🤔
I can see the argument both for upstreaming and for keeping them separate. Although the argument for upstreaming is stronger imo.
The argument for upstreaming is: We want a single `Dialect` trait that handles concerns from both parsing and unparsing. That centralizes the logic and makes it so that there is just "one" place to encode dialect-specific differences. 👍 Makes perfect sense.
The argument for keeping them separate is: By putting in logic to `sqlparser-rs` that only DataFusion cares about (or more generally, systems that want to handle unparsing) - we set a precedent that "I need this for my use-case, so now everyone is forced to have it even though they don't use it", which I'm not 100% happy with (although I do think its very minor). It would make more sense if (for example) the Dialect was taken into consideration by `sqlparser-rs` when writing out the AST to String, but its only considered on parsing.
I'll update the comment for `Dialect`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Omit NULLS FIRST/LAST when unparsing ORDER BY clauses for MySQL [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #10625:
URL: https://github.com/apache/datafusion/pull/10625#issuecomment-2126775297
> Also, I think it's the idea to move the entire unparser to the SQLParser package eventually.
I think this may be hard, as the Unparser relies in `Expr`s (which are a DataFusion struct)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Omit NULLS FIRST/LAST when deparsing ORDER BY clauses for MySQL [datafusion]
Posted by "phillipleblanc (via GitHub)" <gi...@apache.org>.
phillipleblanc commented on PR #10625:
URL: https://github.com/apache/datafusion/pull/10625#issuecomment-2125198690
cc @backkem @devinjdangelo to take a look
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org