Skip to content

[CALCITE-3520] Type cast from primitive to box is not correct#1609

Closed
DonnyZone wants to merge 2 commits intoapache:masterfrom
DonnyZone:CALCITE-3520
Closed

[CALCITE-3520] Type cast from primitive to box is not correct#1609
DonnyZone wants to merge 2 commits intoapache:masterfrom
DonnyZone:CALCITE-3520

Conversation

@DonnyZone
Copy link
Copy Markdown
Contributor

@DonnyZone DonnyZone commented Nov 27, 2019

Current optimization:

int x;
Long.valueOf((long) x) -> Long.valueOf(x)

However, it is not always safe to eliminate primitive cast when converting from primitive to box.

boolean a = false;
byte b = 0;
char c = 'c';
short d = 1;
int e = 0;
long f = 0L;
float g = 1.1f;
double h = 2.2D;

Correct Statement:
==============================================================
Boolean.valueOf(a);
Byte.valueOf(b);
Character.valueOf(c);
Short.valueOf(b); Short.valueOf(d);
Integer.valueOf(b); Integer.valueOf(c); Integer.valueOf(d); Integer.valueOf(e);
Long.valueOf(b); Long.valueOf(c); Long.valueOf(d); Long.valueOf(e); Long.valueOf(f);
Float.valueOf(b); Float.valueOf(c); Float.valueOf(d); Float.valueOf(e); Float.valueOf(f); Float.valueOf(g);
Double.valueOf(b); Double.valueOf(c); Double.valueOf(d); Double.valueOf(e); Double.valueOf(f); Double.valueOf(g); Double.valueOf(h);

If the case is:

int x;
Byte.valueOf((byte) x) -> Byte.valueOf(x)

We will get the exception below.

java.lang.RuntimeException: while resolving method 'valueOf[int]' in class class java.lang.Byte
	at org.apache.calcite.linq4j.tree.Types.lookupMethod(Types.java:323)
	at org.apache.calcite.linq4j.tree.Expressions.call(Expressions.java:445)
	at org.apache.calcite.linq4j.tree.Expressions.call(Expressions.java:457)
	at org.apache.calcite.linq4j.tree.Expressions.box(Expressions.java:1437)
	at org.apache.calcite.adapter.enumerable.EnumUtils.convert(EnumUtils.java:404)
	at org.apache.calcite.adapter.enumerable.EnumUtils.convert(EnumUtils.java:306)
	......
Caused by: java.lang.NoSuchMethodException: java.lang.Byte.valueOf(int)
	at java.base/java.lang.Class.getMethod(Class.java:2108)
	at org.apache.calcite.linq4j.tree.Types.lookupMethod(Types.java:314)
	... 72 more

Moreover, we cannot figure out queries to hit this code path. Because the codegen framework will conduct unbox operation when necessary.

amaliujia
amaliujia reviewed Nov 27, 2019
View reviewed changes
core/src/main/java/org/apache/calcite/adapter/enumerable/EnumUtils.java
|| Primitive.of(una.getType()) == toBox) {
return Expressions.box(una.expression, toBox);
}
}
Copy link
Copy Markdown
Contributor

@amaliujia amaliujia Nov 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

If you check the following lines, there is

      // E.g., from "int" to "Byte".
      // Convert it first and generate "Byte.valueOf((byte)x)"
      // Because there is no method "Byte.valueOf(int)" in Byte
      return Expressions.box(
          Expressions.convert_(operand, toBox.primitiveClass),
          toBox);

So the case was aware of already, and it just seems like cases should have skipped this optimization fail to do so.

Copy link
Copy Markdown
Contributor Author

@DonnyZone DonnyZone Nov 28, 2019

Choose a reason for hiding this comment

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

Yes, this line of code for general case is also added by me in CALCITE-3414 .
But the optimization targets on the pattern like
(byte)(int)->Byte
In which the operand is UnaryExpression with ExpressionType.Convert

@jinxing64
Copy link
Copy Markdown
Contributor

jinxing64 commented Nov 28, 2019

LGTM

@danny0405 danny0405 force-pushed the master branch 2 times, most recently from 80f411d to ca27fe9 Compare November 30, 2019 07:52
danny0405
danny0405 reviewed Nov 30, 2019
View reviewed changes
core/src/test/java/org/apache/calcite/adapter/enumerable/EnumUtilsTest.java
}

@Test public void testTypeFromPrimitiveToBox() {
final Expression intVariable =
Copy link
Copy Markdown
Contributor

@danny0405 danny0405 Nov 30, 2019

Choose a reason for hiding this comment

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

It would be nice if we can add more test cases to cover all the integer data types conversion.

Copy link
Copy Markdown
Contributor Author

@DonnyZone DonnyZone Dec 2, 2019

Choose a reason for hiding this comment

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

Ok, done.

@danny0405 danny0405 added the accepted-and-would-rework-soon This patch is overall ok, but need some refactoring by the committers. label Dec 2, 2019
@DonnyZone
Copy link
Copy Markdown
Contributor Author

DonnyZone commented Dec 3, 2019 •
edited
Loading

I found there is a function assignableFrom in Primitive. It conducts fine-grained check on primitive type's assignment. Therefore, we can add a condition and keep this optimization.

danny0405
danny0405 reviewed Dec 8, 2019
View reviewed changes
core/src/main/java/org/apache/calcite/adapter/enumerable/EnumUtils.java
|| Primitive.of(una.getType()) == toBox) {
return Expressions.box(una.expression, toBox);
&& Primitive.of(una.getType()) == toBox) {
Primitive origin = Primitive.of(una.expression.type);
Copy link
Copy Markdown
Contributor

@danny0405 danny0405 Dec 8, 2019

Choose a reason for hiding this comment

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

Why change || to &&.

Copy link
Copy Markdown
Contributor Author

@DonnyZone DonnyZone Dec 9, 2019

Choose a reason for hiding this comment

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

According to the comment, "Eliminate primitive casts like Long.valueOf((long) x), Generate Long.valueOf(x)", we should satiesfy all these conditions.

Copy link
Copy Markdown
Contributor

@danny0405 danny0405 Dec 12, 2019

Choose a reason for hiding this comment

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

Thanks for the explanation.

@danny0405 danny0405 added LGTM-will-merge-soon Overall PR looks OK. Only minor things left. and removed accepted-and-would-rework-soon This patch is overall ok, but need some refactoring by the committers. labels Dec 12, 2019
@danny0405 danny0405 closed this in d951844 Dec 12, 2019
wangxlong pushed a commit to wangxlong/calcite that referenced this pull request Feb 13, 2020
@DonnyZone @danny0405
[CALCITE-3520] Type cast from primitive to box is not correct (DonnyZ…
49602ff 
…one)

* Always cast the primitive first when converted to box type;
* Remain the optimization with fine-grained type check conditions.

close apache#1609
@michaelmior michaelmior removed the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Feb 14, 2020
jamesstarr pushed a commit to jamesstarr/calcite that referenced this pull request Aug 28, 2025
@DonnyZone @sbroeder
[CALCITE-3520] Type cast from primitive to box is not correct (DonnyZ…
46bddae 
…one)

* Always cast the primitive first when converted to box type;
* Remain the optimization with fine-grained type check conditions.

close apache#1609

Change-Id: I37b369b95b8b446caf34269ef64237aacb56af26
jamesstarr pushed a commit to jamesstarr/calcite that referenced this pull request Mar 16, 2026
@DonnyZone @jamesstarr
[CALCITE-3520] Type cast from primitive to box is not correct (DonnyZ…
144bf81 
…one)

* Always cast the primitive first when converted to box type;
* Remain the optimization with fine-grained type check conditions.

close apache#1609

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

Reviewers

@amaliujia amaliujia amaliujia left review comments

@danny0405 danny0405 danny0405 left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@DonnyZone @jinxing64 @amaliujia @danny0405 @michaelmior