Add IS [NOT] DISTINCT FROM to SQL and join matchers. #14976

gianm · 2023-09-13T20:31:05Z

Changes:

Add "isdistinctfrom" and "notdistinctfrom" native expressions.
Add "IS [NOT] DISTINCT FROM" to SQL. It uses the new native expressions
when generating expressions, and is treated the same as equals and
not-equals when generating native filters on literals.
Update join matchers to have an "includeNull" parameter that determines
whether we are operating in "equals" mode or "is not distinct from"
mode.

Changes: 1) Add "isdistinctfrom" and "notdistinctfrom" native expressions. 2) Add "IS [NOT] DISTINCT FROM" to SQL. It uses the new native expressions when generating expressions, and is treated the same as equals and not-equals when generating native filters on literals. 3) Update join matchers to have an "includeNull" parameter that determines whether we are operating in "equals" mode or "is not distinct from" mode.

abhishekagarwal87 · 2023-09-14T06:56:17Z

processing/src/main/java/org/apache/druid/math/expr/Function.java

+   * SQL function "x IS NOT DISTINCT FROM y". Very similar to "x = y", i.e. {@link BinEqExpr}, except this function
+   * never returns null, and this function considers NULL itself to be not-distinct-from NULL.


this function considers NULL itself to be not-distinct-from NULL - this part is a bit unclear. How about
this function considers NULL as a value

Hmm, maybe adding an example helps too. I changed it to this:

/** * SQL function "x IS NOT DISTINCT FROM y". Very similar to "x = y", i.e. {@link BinEqExpr}, except this function * never returns null, and this function considers NULL as a value, so NULL itself is not-distinct-from NULL. For * example: `x == null` returns `null` in SQL-compatible null handling mode, but `notdistinctfrom(x, null)` is * true if `x` is null. */

This looks good.

abhishekagarwal87 · 2023-09-14T06:58:10Z

processing/src/main/java/org/apache/druid/math/expr/Function.java

+    @Override
+    public String name()
+    {
+      return "notdistinctfrom";


Suggested change

return "notdistinctfrom";

return "is_not_distinct_from";

eh, it is consistent with isnull/notnull

it's consistent with the SQL expression though.

Yeah, I was modeling the names after isnull and notnull. Like,

SQL IS NULL -> native isnull

SQL IS NOT NULL -> native notnull (drop the "is")

I would prefer to keep this whole family of functions consistent with each other. IMO if we also want consistency with SQL then we should make new aliases for isnull and notnull too. That'd be fine by me, fwiw, although I don't personally feel it's necessary.

abhishekagarwal87 · 2023-09-14T06:59:53Z

processing/src/main/java/org/apache/druid/math/expr/Function.java

+          if (leftVal.isNumericNull() || rightVal.isNumericNull()) {
+            return ExprEval.ofLongBoolean(leftVal.isNumericNull() && rightVal.isNumericNull());


I am curious why this wasn't done if the comparison type was Long.

i think it doesn't really matter because its covered by the leftVal.value() == null || rightVal.value == null check that happens before we get here.

This default case is also handling array and complex types, which .. is probably not super cool since the former always claims to be numeric null unless it contains only a single element which can be converted to or is a number, and the latter always claims to be numeric null, but maybe ok if we document the behavior doesn't work with these types.

I thought this part was weird too, but didn't change it (just copied and adapted the code for ==). I added this comment to make it clear where the code comes from:

// Code copied and adapted from BinaryBooleanOpExprBase and BinEqExpr. // The code isn't shared due to differences in code structure: BinaryBooleanOpExprBase + BinEqExpr have logic // interleaved between parent and child class, but we can't use BinaryBooleanOpExprBase as a parent here, because // (a) this is a function, not an expr; and (b) our logic for handling and returning nulls is different from most // binary exprs, where null in means null out.

abhishekagarwal87 · 2023-09-14T07:00:32Z

processing/src/main/java/org/apache/druid/math/expr/Function.java

+    @Override
+    public String name()
+    {
+      return "isdistinctfrom";


Suggested change

return "isdistinctfrom";

return "is_distinct_from";

clintropolis · 2023-09-14T07:36:44Z

processing/src/main/java/org/apache/druid/math/expr/Function.java

+    @Override
+    public String name()
+    {
+      return "notdistinctfrom";


eh, it is consistent with isnull/notnull

clintropolis · 2023-09-14T07:49:28Z

processing/src/main/java/org/apache/druid/math/expr/Function.java

+          if (leftVal.isNumericNull() || rightVal.isNumericNull()) {
+            return ExprEval.ofLongBoolean(leftVal.isNumericNull() && rightVal.isNumericNull());


i think it doesn't really matter because its covered by the leftVal.value() == null || rightVal.value == null check that happens before we get here.

This default case is also handling array and complex types, which .. is probably not super cool since the former always claims to be numeric null unless it contains only a single element which can be converted to or is a number, and the latter always claims to be numeric null, but maybe ok if we document the behavior doesn't work with these types.

clintropolis · 2023-09-14T07:50:33Z

processing/src/main/java/org/apache/druid/math/expr/Function.java

+        case LONG:
+          return ExprEval.ofLongBoolean(leftVal.asLong() == rightVal.asLong());
+        case DOUBLE:
+        default:


hmm, the comparison expressions all handle ARRAY types correctly, should we add it here too? See evalArray methods on the various BinaryBooleanOpExprBase implementations.

- Add ARRAY handling to "notdistinctfrom" and "isdistinctfrom". - Include null in pushed-down filters when using "notdistinctfrom" in a join. Other changes: - Adjust join filter analyzer to more explicitly use InDimFilter's ValuesSets, relying less on remembering to get it right to avoid copies.

gianm · 2023-09-14T17:27:18Z

Pushed a new commit that does:

Merge from master to resolve conflicts with Add IS [NOT] TRUE, IS [NOT] FALSE native functions. #14977.
Add array handling Add IS [NOT] DISTINCT FROM to SQL and join matchers. #14976 (comment).
Fix a bug where pushed-down filters from the join-filter analyzer didn't properly include null when using notdistinctfrom.
Some new comments as noted above.

gianm · 2023-09-19T17:24:54Z

As mentioned on #14978, we'll either need this patch merged prior to the next release, or we'll need the IS NOT DISTINCT FROM doc stuff in #14978 reverted.

gianm added the Area - SQL label Sep 13, 2023

github-actions bot added Area - Documentation Area - Batch Ingestion Area - Querying Area - Segment Format and Ser/De MSQ labels Sep 13, 2023

abhishekagarwal87 reviewed Sep 14, 2023

View reviewed changes

clintropolis reviewed Sep 14, 2023

View reviewed changes

gianm added 2 commits September 14, 2023 10:20

Merge branch 'master' into sql-is-not-distinct-from

f15b41f

gianm added 4 commits September 14, 2023 10:45

Remove unused "wrap" method.

5d838c4

Fixes.

96d1da4

Remove methods we do not need.

c23d4a3

Fix bug with INPUT_REF.

1b15bea

LakshSingla removed the MSQ label Sep 15, 2023

vogievetsky added the Needs web console change Backend API changes that would benefit from frontend support in the web console label Sep 15, 2023

LakshSingla mentioned this pull request Sep 16, 2023

SQL: Plan non-equijoin conditions as cross join followed by filter. #14978

Merged

gianm added this to the 28.0 milestone Sep 19, 2023

Merge branch 'master' into sql-is-not-distinct-from

45af37e

github-actions bot added the Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262 label Sep 19, 2023

clintropolis approved these changes Sep 20, 2023

View reviewed changes

gianm merged commit 823f620 into apache:master Sep 20, 2023
74 checks passed

gianm deleted the sql-is-not-distinct-from branch September 20, 2023 17:44

LakshSingla mentioned this pull request Nov 4, 2023

[DRAFT] 28.0.0 release notes #15326

Closed

vogievetsky mentioned this pull request Nov 8, 2023

Web console: use is not distinct from #15349

Merged

vogievetsky removed the Needs web console change Backend API changes that would benefit from frontend support in the web console label Dec 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add IS [NOT] DISTINCT FROM to SQL and join matchers. #14976

Add IS [NOT] DISTINCT FROM to SQL and join matchers. #14976

gianm commented Sep 13, 2023

abhishekagarwal87 Sep 14, 2023

gianm Sep 14, 2023 •

edited

Loading

abhishekagarwal87 Sep 15, 2023

abhishekagarwal87 Sep 14, 2023

clintropolis Sep 14, 2023

abhishekagarwal87 Sep 14, 2023

gianm Sep 14, 2023

abhishekagarwal87 Sep 14, 2023

clintropolis Sep 14, 2023

gianm Sep 14, 2023

abhishekagarwal87 Sep 14, 2023

clintropolis Sep 14, 2023

clintropolis Sep 14, 2023

clintropolis Sep 14, 2023

gianm Sep 14, 2023

gianm commented Sep 14, 2023 •

edited

Loading

gianm commented Sep 19, 2023

		* SQL function "x IS NOT DISTINCT FROM y". Very similar to "x = y", i.e. {@link BinEqExpr}, except this function
		* never returns null, and this function considers NULL itself to be not-distinct-from NULL.

		if (leftVal.isNumericNull() \|\| rightVal.isNumericNull()) {
		return ExprEval.ofLongBoolean(leftVal.isNumericNull() && rightVal.isNumericNull());

Add IS [NOT] DISTINCT FROM to SQL and join matchers. #14976

Add IS [NOT] DISTINCT FROM to SQL and join matchers. #14976

Conversation

gianm commented Sep 13, 2023

Choose a reason for hiding this comment

gianm Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gianm commented Sep 14, 2023 • edited Loading

gianm commented Sep 19, 2023

gianm Sep 14, 2023 •

edited

Loading

gianm commented Sep 14, 2023 •

edited

Loading