Skip to content

[CALCITE-3433] EQUALS operator between date/timestamp types returns false if the type is nullable#1540

Closed
DonnyZone wants to merge 1 commit intoapache:masterfrom
DonnyZone:CALCITE-3433
Closed

[CALCITE-3433] EQUALS operator between date/timestamp types returns false if the type is nullable#1540
DonnyZone wants to merge 1 commit intoapache:masterfrom
DonnyZone:CALCITE-3433

Conversation

@DonnyZone
Copy link
Contributor

@DonnyZone DonnyZone commented Oct 28, 2019 •
edited by danny0405
Loading

Currently, BinaryExpression generates all Equal/NotEqual operation as "=="/"!=".
However, when comparing two primitive boxing classes, we may get wrong result, such as the example described in CLACITE-3433.
This PR adopts Objects.equals to handle this problem.

danny0405
danny0405 reviewed Oct 28, 2019
View reviewed changes
linq4j/src/main/java/org/apache/calcite/linq4j/tree/BinaryExpression.java Outdated
Primitive boxPrimitive1 = Primitive.ofBox(expression1.getType());
if (boxPrimitive0 != null && boxPrimitive1 != null
&& !isConstantNullExpression(expression0)
&& !isConstantNullExpression(expression1)) {
Copy link
Contributor

@danny0405 danny0405 Oct 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should move this logic into [1]

[1]

calcite/linq4j/src/main/java/org/apache/calcite/linq4j/tree/BinaryExpression.java

Line 103 in 08dfd17

return expression0.evaluate(evaluator)

Copy link
Contributor Author

@DonnyZone DonnyZone Oct 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean that we should keep the evaluation logic and codegen logic as same?

Copy link
Contributor

@danny0405 danny0405 Oct 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, i misunderstood, sorry for that ~

@yanlin-Lynn
Copy link
Contributor

yanlin-Lynn commented Oct 29, 2019

There is already a eq function in SqlFunctions.
How about call it when it's non primitive types.

https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java#L619

For this case, the generated code may like this

public Object current() {
              final Object[] current = (Object[]) inputEnumerator.current();
              final Long inp0_ = (Long) current[0];
              final Long inp1_ = (Long) current[1];
              return new Object[] {
                  inp0_ == null || inp1_ == null ? (Boolean) null : Boolean.valueOf(org.apache.calcite.runtime.SqlFunctions.eq(inp0_, inp1_)),
                  inp0_ == null || inp1_ == null ? (Boolean) null : Boolean.valueOf(org.apache.calcite.runtime.SqlFunctions.ne(inp0_, inp1_))};
            }

          };

@DonnyZone
Copy link
Contributor Author

DonnyZone commented Oct 29, 2019

@yanlin-Lynn Thanks for good advice! We can fall back to call backupMethodName in SqlFunctions.

@DonnyZone
Copy link
Contributor Author

DonnyZone commented Oct 29, 2019

@danny0405 @yanlin-Lynn, I update the fix. Could you take a look when you have time?

danny0405
danny0405 reviewed Oct 29, 2019
View reviewed changes
core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java Outdated
if (EQUALS_OPERATORS.contains(op)
&& boxPrimitive0 != null && boxPrimitive1 != null
&& !isConstantNullExpression(expressions.get(0))
&& !isConstantNullExpression(expressions.get(1))) {
Copy link
Contributor

@danny0405 danny0405 Oct 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about if one operand is Long and another is Integer but with the same value ?

Copy link
Contributor Author

@DonnyZone DonnyZone Oct 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In RexImpTable, when defining BinaryImplementor, there is a harmonize function called to ensure that operands have identical type.

Copy link
Contributor

@danny0405 danny0405 Oct 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, the defineBinary indeed is with harmonize as true, thanks for the explanation.

yanlin-Lynn
yanlin-Lynn reviewed Oct 29, 2019
View reviewed changes
core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java Outdated
}
ConstantExpression ce = (ConstantExpression) expression;
return ce.value == null;
}
Copy link
Contributor

@yanlin-Lynn yanlin-Lynn Oct 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think isConstantNullExpression is unnecessary, it seems null value is processed in
https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java#L1083.
You can check again.

Copy link
Contributor Author

@DonnyZone DonnyZone Oct 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yanlin-Lynn Yes, we can remove this check. Comments addressed, thanks!

@danny0405 danny0405 added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Oct 29, 2019
@danny0405 danny0405 closed this in 65043f2 Nov 1, 2019
XuQianJin-Stars pushed a commit to XuQianJin-Stars/calcite that referenced this pull request Nov 17, 2019
@DonnyZone @XuQianJin-Stars
[CALCITE-3433] EQUALS operator between date/timestamp types returns f…
7c509ce 
…alse if the type is nullable (DonnyZone)

When checking equals or not-equals on two primitive boxing classes
(i.e. Long x, Long y), we should fall back to call `SqlFunctions.eq(x, y)`
and `SqlFunctions.ne(x, y)`, rather than `x == y`.

close apache#1540
XuQianJin-Stars pushed a commit to XuQianJin-Stars/calcite that referenced this pull request Nov 20, 2019
@DonnyZone @XuQianJin-Stars
[CALCITE-3433] EQUALS operator between date/timestamp types returns f…
fe5e437 
…alse if the type is nullable (DonnyZone)

When checking equals or not-equals on two primitive boxing classes
(i.e. Long x, Long y), we should fall back to call `SqlFunctions.eq(x, y)`
and `SqlFunctions.ne(x, y)`, rather than `x == y`.

close apache#1540
XuQianJin-Stars pushed a commit to XuQianJin-Stars/calcite that referenced this pull request Nov 20, 2019
@DonnyZone @XuQianJin-Stars
[CALCITE-3433] EQUALS operator between date/timestamp types returns f…
c4febd6 
…alse if the type is nullable (DonnyZone)

When checking equals or not-equals on two primitive boxing classes
(i.e. Long x, Long y), we should fall back to call `SqlFunctions.eq(x, y)`
and `SqlFunctions.ne(x, y)`, rather than `x == y`.

close apache#1540
wangxlong pushed a commit to wangxlong/calcite that referenced this pull request Feb 13, 2020
@DonnyZone @danny0405
[CALCITE-3433] EQUALS operator between date/timestamp types returns f…
70d3664 
…alse if the type is nullable (DonnyZone)

When checking equals or not-equals on two primitive boxing classes
(i.e. Long x, Long y), we should fall back to call `SqlFunctions.eq(x, y)`
and `SqlFunctions.ne(x, y)`, rather than `x == y`.

close apache#1540
jamesstarr pushed a commit to jamesstarr/calcite that referenced this pull request Aug 28, 2025
@DonnyZone @sbroeder
[CALCITE-3433] EQUALS operator between date/timestamp types returns f…
7f4e58f 
…alse if the type is nullable (DonnyZone)

When checking equals or not-equals on two primitive boxing classes
(i.e. Long x, Long y), we should fall back to call `SqlFunctions.eq(x, y)`
and `SqlFunctions.ne(x, y)`, rather than `x == y`.

close apache#1540

Change-Id: I2acb25a189c607f03e8aab61ad3a592f7ce61550
jamesstarr pushed a commit to jamesstarr/calcite that referenced this pull request Mar 16, 2026
@DonnyZone @jamesstarr
[CALCITE-3433] EQUALS operator between date/timestamp types returns f…
799e33f 
…alse if the type is nullable (DonnyZone)

When checking equals or not-equals on two primitive boxing classes
(i.e. Long x, Long y), we should fall back to call `SqlFunctions.eq(x, y)`
and `SqlFunctions.ne(x, y)`, rather than `x == y`.

close apache#1540

Change-Id: I2acb25a189c607f03e8aab61ad3a592f7ce61550
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@danny0405 danny0405 danny0405 left review comments

@yanlin-Lynn yanlin-Lynn yanlin-Lynn left review comments

Assignees

No one assigned

Labels

LGTM-will-merge-soon Overall PR looks OK. Only minor things left.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@DonnyZone @yanlin-Lynn @danny0405