kosiew commented on code in PR #22999:
URL: https://github.com/apache/datafusion/pull/22999#discussion_r3434633317
##########
datafusion/sql/src/unparser/expr.rs:
##########
@@ -156,10 +156,24 @@ impl Unparser<'_> {
let l = self.expr_to_sql_inner(left.as_ref())?;
let r = self.expr_to_sql_inner(right.as_ref())?;
- Ok(ast::Expr::Nested(Box::new(ast::Expr::IsDistinctFrom(
- Box::new(l),
- Box::new(r),
- ))))
+ match self.dialect.distinct_from_style() {
+ DistinctFromStyle::FullText =>
Ok(ast::Expr::Nested(Box::new(
+ ast::Expr::IsDistinctFrom(Box::new(l), Box::new(r)),
+ ))),
+ DistinctFromStyle::DiamondOperators => {
+ Ok(ast::Expr::Nested(Box::new(ast::Expr::UnaryOp {
Review 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 <left> <=> <right> 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)).
##########
datafusion/sql/src/unparser/dialect.rs:
##########
@@ -333,6 +338,17 @@ pub enum CharacterLengthStyle {
CharacterLength,
}
+/// `DistinctFromStyle` to use for unparsing `IsDistinctFrom` and
`IsNotDistinctFrom` operators
+#[derive(Clone, Copy, PartialEq)]
+pub enum DistinctFromStyle {
+ /// DBMS supports `IS (NOT) DISTINCT FROM`
+ FullText,
+ /// DMBS supports equivalent operations via `<=>` and `<>`
+ DiamondOperators,
Review 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.
##########
datafusion/sql/src/unparser/dialect.rs:
##########
@@ -94,6 +94,11 @@ pub trait Dialect: Send + Sync {
DateFieldExtractStyle::DatePart
}
+ /// The style to use when unparsing DISTINCT FROM style expressions
+ fn distinct_from_style(&self) -> DistinctFromStyle {
Review 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.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
