[CALCITE-6435] SqlToRel conversion of IN expressions may lead to incorrect simplifications by kgyrtkirk · Pull Request #3835 · apache/calcite

kgyrtkirk · 2024-06-27T15:09:10Z

Conversion path for comparisions generated from IN expressions was handling types differently. This may have lead to some over-simplification in some cases.

Altered the conversion to do the full SqlToRex conversion steps for these generated nodes as well. Added an extra safeguard check to RexSimplify to prevent the bug from being triggered.

mihaibudiu · 2024-06-27T18:13:46Z

core/src/main/java/org/apache/calcite/rex/RexSimplify.java

+
+            if (prevLiteral != null
+                && literal.getType().equals(prevLiteral.getType())
+                && !literal.equals(prevLiteral)) {


so two literals can be equal but their types can be different?
one could argue that this is a bug in SqlLiteral::equals.

the situation was caused by two literals which had the same value; but different types (VARCHAR; CHAR(n))

actually the predicate based simplification also does this (beyond other things); but there are situations in which its disabled.

I think VARCHAR and CHAR should be considered equivalent if RexLiteral.value is the same

CHAR(N) indicates that the literal has to be padded with spaces, whereas VARCHAR indicates that trailing spaces are to be ignored. So two literals with identical strings but different types can have different values.

mihaibudiu · 2024-06-27T18:16:30Z

druid/src/test/java/org/apache/calcite/test/DruidAdapter2IT.java

        + "filter=[AND("
        + "=($3, 'High Top Dried Mushrooms'), "
-        + "SEARCH($87, Sarg['Q2', 'Q3']:CHAR(2)), "
+        + "SEARCH($87, Sarg['Q2':VARCHAR, 'Q3':VARCHAR]:VARCHAR), "


I find this change surprising.

I was surprised too ; however this is the same kind of expressions SqlToRel would produce if the input would have been a set of comparisison ; so the IN rewrite now matches that

core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java

mihaibudiu · 2024-06-28T17:02:08Z

Let's wait a bit to see whether anyone else has comments.

…rrect simplifications Conversion path for comparisions generated from IN expressions was handling types differently. This may have lead to some over-simplification in some cases. Altered the conversion to do the full SqlToRex conversion steps for these generated nodes as well. Added an extra safeguard check to RexSimplify to prevent the bug from being triggered.

sonarqubecloud · 2024-06-29T07:07:14Z

Quality Gate passed

Issues
7 New issues
0 Accepted issues

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

See analysis details on SonarCloud

YiwenWu · 2024-06-29T07:57:44Z

I think it can be simpler to modify the judgment of RexSimplify#simplifyAnd2ForUnknownAsFalse?

if (prevLiteral != null && !literal.equalsWithTypeName(prevLiteral)) {
     return rexBuilder.makeLiteral(false);
 }

where RexLiteral#equalsWithTypeName compares to TypeName instead of Type in RexLiteral#equals . In this case, varchar Sebastian and char Sebastian can be equivalent.

kgyrtkirk · 2024-06-29T09:20:03Z

I think it can be simpler to modify the judgment of RexSimplify#simplifyAnd2ForUnknownAsFalse?
if (prevLiteral != null && !literal.equalsWithTypeName(prevLiteral)) {
     return rexBuilder.makeLiteral(false);
 }
where RexLiteral#equalsWithTypeName compares to TypeName instead of Type in RexLiteral#equals . In this case, varchar Sebastian and char Sebastian can be equivalent.

I don't see any such method in
RexLiteral did I missed it?

I would be kinda against of the introduction of such method because it would place custom type comparision logic inside a Rex implementation...

I might have not mentioned this here...but
weakening these in RexSimplify has no adverse effects because predicate based simplification can do this too(and more) ; I think these map based things can even be removed....

YiwenWu · 2024-06-30T08:33:04Z

core/src/main/java/org/apache/calcite/rex/RexSimplify.java

          continue;
        }
-        if (!literal1.equals(literal2)) {
+        if (literal1.getType().equals(literal2.getType()) && !literal1.equals(literal2)) {


I'm puzzled about the necessity of adding extra type equals checks. What specific issues might arise without them in SQL simplification?

YiwenWu · 2024-06-30T08:37:33Z

core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java

-                        // TODO: remove requireNonNull when checkerframework issue resolved
-                        ensureSqlType(requireNonNull(pair.left, "pair.left").getType(),
-                            bb.convertExpression(pair.right)))));
+            RexUtil.composeConjunction(


Is the focus solely on the SQL IN case, or might there be a need for adjustments elsewhere too?

not really; this conjunction is there for complex expressions; which will be there for NOT IN as well:

(a,b) NOT IN ((1,2), (3,4)) NOT ((a=1 && b=2) || (a=3 && b=4) )

YiwenWu · 2024-06-30T09:07:10Z

I think it can be simpler to modify the judgment of RexSimplify#simplifyAnd2ForUnknownAsFalse?
if (prevLiteral != null && !literal.equalsWithTypeName(prevLiteral)) {
     return rexBuilder.makeLiteral(false);
 }
where RexLiteral#equalsWithTypeName compares to TypeName instead of Type in RexLiteral#equals . In this case, varchar Sebastian and char Sebastian can be equivalent.
I don't see any such method in RexLiteral did I missed it?

I would be kinda against of the introduction of such method because it would place custom type comparision logic inside a Rex implementation...

I might have not mentioned this here...but weakening these in RexSimplify has no adverse effects because predicate based simplification can do this too(and more) ; I think these map based things can even be removed....

Currently, there is no RexLiteral#equalsWithTypeName method.

It seems two distinct approaches. This PR handles CHAR to VARCHAR conversion early in SqlToRelConverter , enabling direct conjunction in the simplification stage. Alternatively, refine the simplifyAnd2ForUnknownAsFalse method to bypass false judgments for RexLiteral with identical value and typeName(SqlTypeName).

There is no problem based on the approach of this PR. Only two small points that need to be confirmed:

Addressing range concerns, the CHAR to VARCHAR conversion is specific to IN conditions?
Is an update to simplifyAnd2ForUnknownAsFalse necessary, what scenarios would benefit from this change?

kgyrtkirk · 2024-07-06T09:43:17Z

Addressing range concerns, the CHAR to VARCHAR conversion is specific to IN conditions?

the issue arised from the fact that SqlToRel had a different route to produce = checks when IN was rewritten to a set of OR conditions; this PR only makes that extra route go away - and shows the converter the full = expression to do the same conversion as it would have done in case it would have been part of the original query

Is an update to simplifyAnd2ForUnknownAsFalse necessary, what scenarios would benefit from this change?

not mandatory for the current bug ; as 1 solves that ; but I would rather restrict the cases in which RexSimplify could over-simplify things; as its a quite frequently called part.

caicancai

LGTM

kgyrtkirk force-pushed the literal-type-mismtach3 branch from b1d979c to 84069b2 Compare June 27, 2024 17:19

mihaibudiu approved these changes Jun 27, 2024

View reviewed changes

kgyrtkirk force-pushed the literal-type-mismtach3 branch from 84069b2 to f08ddbf Compare June 28, 2024 07:27

kgyrtkirk force-pushed the literal-type-mismtach3 branch from f08ddbf to 0a4a974 Compare June 29, 2024 06:45

YiwenWu reviewed Jun 30, 2024

View reviewed changes

kgyrtkirk added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Jul 9, 2024

caicancai approved these changes Jul 9, 2024

View reviewed changes

kgyrtkirk merged commit 73846cc into apache:main Jul 11, 2024

Conversation

kgyrtkirk commented Jun 27, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YiwenWu Jun 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mihaibudiu commented Jun 28, 2024

Uh oh!

sonarqubecloud bot commented Jun 29, 2024

Quality Gate passed

Uh oh!

YiwenWu commented Jun 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kgyrtkirk commented Jun 29, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YiwenWu commented Jun 30, 2024

Uh oh!

kgyrtkirk commented Jul 6, 2024

Uh oh!

caicancai left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

YiwenWu Jun 29, 2024 •

edited

Loading

YiwenWu commented Jun 29, 2024 •

edited

Loading