[CALCITE-6592] RelMdPredicates pull up wrong predicate from UNION whe… #3975

NobiGo · 2024-09-24T12:30:44Z

…n it's input include NULL VALUE

core/src/test/java/org/apache/calcite/test/RelMetadataTest.java

…n it's input include NULL VALUE

NobiGo · 2024-09-25T01:25:59Z

Forch-pushed to rerun CI.

caicancai

LGTM

zabetak · 2024-09-25T07:59:02Z

core/src/main/java/org/apache/calcite/rel/metadata/RelMdPredicates.java

@@ -475,7 +475,7 @@ public RelOptPredicateList getPredicates(Union union, RelMetadataQuery mq) {
    RexNode disjunctivePredicate =
        new RexSimplify(rexBuilder, RelOptPredicateList.EMPTY, executor)
            .simplifyUnknownAs(rexBuilder.makeCall(SqlStdOperatorTable.OR, finalResidualPredicates),
-                RexUnknownAs.FALSE);


Some methods in this class are considering RexUnknownAs.FALSE and some others RexUnknownAs.UNKNOWN when performing the simplification. Why do we need to simplify differently?

This question needs to be answered before merging.

@zabetak I will check this.

zabetak · 2024-09-25T08:03:40Z

core/src/test/java/org/apache/calcite/test/RelMetadataTest.java

+    final RelMetadataQuery mq = rel.getCluster().getMetadataQuery();
+    RelOptPredicateList inputSet = mq.getPulledUpPredicates(rel);
+    ImmutableList<RexNode> pulledUpPredicates = inputSet.pulledUpPredicates;
+    assertThat(pulledUpPredicates, sortsAs("[OR(null, =($0, 5))]"));


The result does not look 100% accurate. Shouldn't this rather be OR(IS NULL($0), =($0, 5)). What happens if there were more columns involved?

@zabetak Hi, testPullUpPredicatesFromUnionWithValues3 added for more columns. Maybe we should extract nothing when we can't get a specific value like OR(IS NULL($0), =($0, 5)). WDYT?

I don't understand why we can't extract IS NULL($0) in this case. Can you please elaborate?

zabetak

I have the impression that the problem is actually on the way that we extract predicates from Values and not so on what happens in Union. I think we should use the eqConstant method instead of plain equals.

zabetak · 2024-09-25T08:19:25Z

If the problem is indeed in Values please add specific tests for this expression and update the JIRA/PR accordingly.

NobiGo · 2024-09-26T01:13:26Z

If the problem is indeed in Values please add specific tests for this expression and update the JIRA/PR accordingly.

@zabetak Hi, It is not a VALUES problem. But When I try to add a test for VALUERS. I find another problem in VALUES.
For example:

VALUES(1,2,3)

Predicates should be ($0=1,$1=2,$2=3), but now is $0=1. In my PR I have make it right and add the unit test.

sonarcloud · 2024-09-26T02:02:33Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

zabetak · 2024-09-26T07:25:46Z

core/src/test/java/org/apache/calcite/test/RelMetadataTest.java

+    final RelMetadataQuery mq = values.getCluster().getMetadataQuery();
+    RelOptPredicateList inputSet = mq.getPulledUpPredicates(values);
+    ImmutableList<RexNode> pulledUpPredicates = inputSet.pulledUpPredicates;
+    assertThat(pulledUpPredicates, sortsAs("[=($0, null)]"));


The result seems wrong.The predicate =($0, null) does not hold in the output rows of the values(null) query. If you add a filter on top of the values that says =($0, null) it will evaluate to false but here we know that $0 is always null so the correct predicate to infer is IS NULL($0).

More general, I think that the Predicates metadata should never use the = operator with the null literal.

Yes. We can use IS NULL($0) to replace =($0, null). I will take care of it.

CALCITE-6599 has covered the [IS NULL($0)] replace [=$($0, null)]. IS NULL($0) can eliminate ambiguity caused by [=$($0, null)].

Consider is_not_distinct_from

@zabetak @mihaibudiu I noticed we have a method to handle this:

If field i is equal to a constant expression, then we use EQUALS

If field i is nullable, then we use IS NOT DISTINCT FROM

If filed i is the literal null, then we use IS NULL

Yes, we have a method. See the link on my previous comment: #3975 (review)

Yes. I missed it.

I think that is not distinct from can be used in all cases

zabetak · 2024-09-26T07:32:30Z

core/src/test/java/org/apache/calcite/test/RelMetadataTest.java

+    final RelMetadataQuery mq = rel.getCluster().getMetadataQuery();
+    RelOptPredicateList inputSet = mq.getPulledUpPredicates(rel);
+    ImmutableList<RexNode> pulledUpPredicates = inputSet.pulledUpPredicates;
+    assertThat(pulledUpPredicates, sortsAs("[OR(null, =($0, 5))]"));


I don't understand why we can't extract IS NULL($0) in this case. Can you please elaborate?

zabetak · 2024-09-26T07:33:05Z

core/src/main/java/org/apache/calcite/rel/metadata/RelMdPredicates.java

@@ -475,7 +475,7 @@ public RelOptPredicateList getPredicates(Union union, RelMetadataQuery mq) {
    RexNode disjunctivePredicate =
        new RexSimplify(rexBuilder, RelOptPredicateList.EMPTY, executor)
            .simplifyUnknownAs(rexBuilder.makeCall(SqlStdOperatorTable.OR, finalResidualPredicates),
-                RexUnknownAs.FALSE);


This question needs to be answered before merging.

zabetak · 2024-09-26T09:14:20Z

I see that you are performing the changes that we discuss here in another pull request (#3981). This is a bit confusing and I am losing track on where I should comment on.

NobiGo · 2024-09-26T09:23:30Z

I see that you are performing the changes that we discuss here in another pull request (#3981). This is a bit confusing and I am losing track on where I should comment on.

These are two different questions. So I open another PR for VALUES. But we can talk about it together here.
These are some of my opinions：

For Values:
values(1, 2, 3, null), (1, 2, null, null), (5, 2, 3, null)
We pull up the predicate is [=($1, 2), IS NULL($3)].
Do we need to extract [OR(IS NULL($2), =($2, 3))]? I think we don't need it. Because when VALUES size is large, It will be complicated.

caicancai reviewed Sep 24, 2024

View reviewed changes

core/src/test/java/org/apache/calcite/test/RelMetadataTest.java Show resolved Hide resolved

mihaibudiu approved these changes Sep 24, 2024

View reviewed changes

mihaibudiu added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Sep 24, 2024

NobiGo force-pushed the CALCITE-6592 branch from 478fbf4 to 804257d Compare September 25, 2024 00:26

[CALCITE-6592] RelMdPredicates pull up wrong predicate from UNION whe…

13935d2

…n it's input include NULL VALUE

NobiGo force-pushed the CALCITE-6592 branch from 804257d to 13935d2 Compare September 25, 2024 01:24

caicancai approved these changes Sep 25, 2024

View reviewed changes

zabetak reviewed Sep 25, 2024

View reviewed changes

julianhyde force-pushed the main branch from 8d3cb82 to 4ab8d80 Compare September 25, 2024 18:04

[CALCITE-5156] Add test for VALUES

2d565c2

[CALCITE-5156] Add test for UNION

47bea6a

zabetak reviewed Sep 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CALCITE-6592] RelMdPredicates pull up wrong predicate from UNION whe… #3975

[CALCITE-6592] RelMdPredicates pull up wrong predicate from UNION whe… #3975

NobiGo commented Sep 24, 2024

NobiGo commented Sep 25, 2024

caicancai left a comment

zabetak Sep 25, 2024

zabetak Sep 26, 2024

NobiGo Sep 26, 2024

zabetak Sep 25, 2024

NobiGo Sep 26, 2024

zabetak Sep 26, 2024

zabetak left a comment

zabetak commented Sep 25, 2024

NobiGo commented Sep 26, 2024

sonarcloud bot commented Sep 26, 2024

zabetak Sep 26, 2024

zabetak Sep 26, 2024

NobiGo Sep 26, 2024

NobiGo Sep 26, 2024 •

edited

Loading

mihaibudiu Sep 26, 2024

NobiGo Sep 26, 2024

zabetak Sep 26, 2024

NobiGo Sep 26, 2024

mihaibudiu Sep 26, 2024

zabetak Sep 26, 2024

zabetak Sep 26, 2024

zabetak commented Sep 26, 2024

NobiGo commented Sep 26, 2024 •

edited

Loading

[CALCITE-6592] RelMdPredicates pull up wrong predicate from UNION whe… #3975

Are you sure you want to change the base?

[CALCITE-6592] RelMdPredicates pull up wrong predicate from UNION whe… #3975

Conversation

NobiGo commented Sep 24, 2024

NobiGo commented Sep 25, 2024

caicancai left a 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

zabetak left a comment

Choose a reason for hiding this comment

zabetak commented Sep 25, 2024

NobiGo commented Sep 26, 2024

sonarcloud bot commented Sep 26, 2024

Quality Gate passed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NobiGo Sep 26, 2024 • 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

zabetak commented Sep 26, 2024

NobiGo commented Sep 26, 2024 • edited Loading

NobiGo Sep 26, 2024 •

edited

Loading

NobiGo commented Sep 26, 2024 •

edited

Loading