Omit NULLS FIRST/LAST when unparsing ORDER BY clauses for MySQL

[phillipleblanc]

Which issue does this PR close?

Closes #10624

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.

[phillipleblanc]

cc @backkem @devinjdangelo to take a look

[backkem]

LGTM, we'll have to remember to upstream this to the SQLParser package.

[alamb]

Thanks @phillipleblanc and @backkem

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 🤔

[phillipleblanc]

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 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.

edit: I realized after posting this, it does make sense to throw an error when parsing a MySQL dialect if the string contains NULLS FIRST, so it does make sense to include this in the sqlparser-rs Dialect. I'll create a PR to sqlparser-rs for it.

edit 2: Here is the sqlparser-rs PR: apache/datafusion-sqlparser-rs#1284

edit 3: I closed that PR, see below discussion

[backkem]

Also, I think it's the idea to move the entire unparser to the SQLParser package eventually.

[alamb]

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 Exprs (which are a DataFusion struct)

[phillipleblanc]

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.

[alamb]

Thanks @phillipleblanc and @backkem