feat(unparser): support DISTINCT FROM operators in the MySQL dialect#22999
feat(unparser): support DISTINCT FROM operators in the MySQL dialect#22999zyuiop wants to merge 4 commits into
Conversation
zyuiop
changed the title
zyuiop
changed the title
| } | ||
|
|
||
| /// The style to use when unparsing DISTINCT FROM style expressions | ||
| fn distinct_from_style(&self) -> DistinctFromStyle { |
There was a problem hiding this comment.
I think this new default changes existing behavior for downstream and custom dialects. Before this PR, a Dialect that did not override this area could still unparse IS DISTINCT FROM and IS NOT DISTINCT FROM using the standard full-text syntax.
With the default now set to Unsupported, those same dialects can start returning not_impl_err! for expressions that previously worked. The MySQL fix only needs a MySQL-specific override, so I think the trait default should preserve the old behavior. For example, this could default to DistinctFromStyle::FullText and only dialects that need a different spelling would override it.
There was a problem hiding this comment.
Okay, I have therefore removed the Unsupported enum variant
| ast::Expr::IsDistinctFrom(Box::new(l), Box::new(r)), | ||
| ))), | ||
| DistinctFromStyle::DiamondOperators => { | ||
| Ok(ast::Expr::Nested(Box::new(ast::Expr::UnaryOp { |
There was a problem hiding this comment.
I think the MySQL spelling for IS DISTINCT FROM needs to force the <=> comparison to be parsed as the operand of NOT.
As written, NOT <=> can be ambiguous with MySQL HIGH_NOT_PRECEDENCE, where NOT a <=> b may be parsed as (NOT a) <=> b. That is not equivalent to a IS DISTINCT FROM b.
Could this build the unary expression over a nested spaceship comparison instead? For example: NOT (c1 <=> true), or with outer parentheses as (NOT (c1 <=> true)).
There was a problem hiding this comment.
Okay, I have added Expr::Nested inside the UnaryOp
| /// DBMS supports `IS (NOT) DISTINCT FROM` | ||
| FullText, | ||
| /// DMBS supports equivalent operations via `<=>` and `<>` | ||
| DiamondOperators, |
There was a problem hiding this comment.
Small naming/comment suggestion: the DiamondOperators comment says this style uses <=> and <>, but the implementation uses <=> and NOT (<=>), which seems right.
Since <> is not null-safe and would not be equivalent to IS DISTINCT FROM, it might be clearer to rename this or update the comment to describe it as a spaceship or null-safe-equality style.
There was a problem hiding this comment.
Correct, I initially intended to use <> and after checking the docs more carefully I noticed it was not equivalent.
I changed the comment and renamed the enum variant to Spaceship.
|
Regarding the PR feedback, should I squash all my changes, or will it be done either way when merging? |
it's be squashed during merge into 1 commit. |
There was a problem hiding this comment.
@zyuiop
Thanks for the updates. I re-reviewed the latest changes and confirmed the previous blocking comments have been addressed:
Dialect::distinct_from_stylenow defaults toDistinctFromStyle::FullText, preserving behavior for existing downstream and custom dialects.- MySQL
IS DISTINCT FROMnow unparses asNOTapplied to a nested<=>comparison, which avoidsHIGH_NOT_PRECEDENCEambiguity. - The enum variant has been renamed from
DiamondOperatorstoSpaceship, and the comment no longer implies<>is equivalent.
No blocking findings from this follow-up review.
I left one optional documentation nit below.
|
Merging with |
2 participants
Which issue does this PR close?
Closes #22997
Rationale for this change
MySQL does not support the
IS NOT DISTINCT FROMsyntax, but has an alternative syntax which does the same, the spaceship operator.What changes are included in this PR?
Adds a
DistinctFromStyleattribute to dialects that dictates howIS DISTINCT FROMis unparsed.Are these changes tested?
Yes
Are there any user-facing changes?
No, we provide a default implementation for
DistinctFromStyle