You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2021/09/20 12:34:50 UTC
[GitHub] [calcite] gierlachg opened a new pull request #2530: [CALCITE-4532] Incorrect code generated for primitive-object ConstantExpression
gierlachg opened a new pull request #2530:
URL: https://github.com/apache/calcite/pull/2530
[CALCITE-4532](https://issues.apache.org/jira/browse/CALCITE-4532)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] gierlachg commented on a change in pull request #2530: [CALCITE-4532] Correct code generated for primitive-object ConstantExpression
Posted by GitBox <gi...@apache.org>.
gierlachg commented on a change in pull request #2530:
URL: https://github.com/apache/calcite/pull/2530#discussion_r714729470
##########
File path: linq4j/src/main/java/org/apache/calcite/linq4j/tree/ConstantExpression.java
##########
@@ -136,6 +136,10 @@ private static ExpressionWriter write(ExpressionWriter writer,
write(writer, value, primitive2.primitiveClass);
return writer.append(")");
}
+ Primitive primitive3 = Primitive.ofBox(value.getClass());
+ if (Object.class.equals(type) && primitive3 != null) {
Review comment:
Wanted to stay consistent with `primitive2` couple of lines above.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] DonnyZone merged pull request #2530: [CALCITE-4532] Correct code generated for primitive-object ConstantExpression
Posted by GitBox <gi...@apache.org>.
DonnyZone merged pull request #2530:
URL: https://github.com/apache/calcite/pull/2530
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] DonnyZone commented on pull request #2530: [CALCITE-4532] Correct code generated for primitive-object ConstantExpression
Posted by GitBox <gi...@apache.org>.
DonnyZone commented on pull request #2530:
URL: https://github.com/apache/calcite/pull/2530#issuecomment-926529060
Hi @gierlachg, could you please squash the commits? So that I can merge your fix through Github UI quickly.
My local environment has encountered some token problems today.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] zinking commented on a change in pull request #2530: [CALCITE-4532] Correct code generated for primitive-object ConstantExpression
Posted by GitBox <gi...@apache.org>.
zinking commented on a change in pull request #2530:
URL: https://github.com/apache/calcite/pull/2530#discussion_r714724829
##########
File path: linq4j/src/main/java/org/apache/calcite/linq4j/tree/ConstantExpression.java
##########
@@ -136,6 +136,10 @@ private static ExpressionWriter write(ExpressionWriter writer,
write(writer, value, primitive2.primitiveClass);
return writer.append(")");
}
+ Primitive primitive3 = Primitive.ofBox(value.getClass());
+ if (Object.class.equals(type) && primitive3 != null) {
Review comment:
doesn't primitive just mean unboxed java type ? can you call it just boxed ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] DonnyZone commented on a change in pull request #2530: [CALCITE-4532] Correct code generated for primitive-object ConstantExpression
Posted by GitBox <gi...@apache.org>.
DonnyZone commented on a change in pull request #2530:
URL: https://github.com/apache/calcite/pull/2530#discussion_r715425762
##########
File path: linq4j/src/test/java/org/apache/calcite/linq4j/test/ExpressionTest.java
##########
@@ -1357,6 +1357,25 @@ public void checkBlockBuilder(boolean optimizing, String expected) {
Expressions.toString(Expressions.constant(12.34, BigDecimal.class)));
}
+ @Test void testObjectConstantExpression() {
+ assertEquals("true",
+ Expressions.toString(Expressions.constant(true, Object.class)));
+ assertEquals("(byte)100",
+ Expressions.toString(Expressions.constant((byte) 100, Object.class)));
+ assertEquals("(char)100",
+ Expressions.toString(Expressions.constant((char) 100, Object.class)));
+ assertEquals("(short)100",
+ Expressions.toString(Expressions.constant((short) 100, Object.class)));
+ assertEquals("100",
+ Expressions.toString(Expressions.constant(100, Object.class)));
+ assertEquals("100L",
+ Expressions.toString(Expressions.constant(100L, Object.class)));
+ assertEquals("100.0F",
+ Expressions.toString(Expressions.constant(100F, Object.class)));
+ assertEquals("100.0D",
+ Expressions.toString(Expressions.constant(100D, Object.class)));
+ }
+
Review comment:
The following two test case also pass without this commit. We can remove them.
```
assertEquals("true",
Expressions.toString(Expressions.constant(true, Object.class)));
assertEquals("100",
Expressions.toString(Expressions.constant(100, Object.class)));
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] DonnyZone commented on pull request #2530: [CALCITE-4532] Correct code generated for primitive-object ConstantExpression
Posted by GitBox <gi...@apache.org>.
DonnyZone commented on pull request #2530:
URL: https://github.com/apache/calcite/pull/2530#issuecomment-926292389
> @DonnyZone any chance you could take a quick look at this one?
Sure, I will take a look today.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] DonnyZone commented on pull request #2530: [CALCITE-4532] Correct code generated for primitive-object ConstantExpression
Posted by GitBox <gi...@apache.org>.
DonnyZone commented on pull request #2530:
URL: https://github.com/apache/calcite/pull/2530#issuecomment-926494006
LGTM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on pull request #2530: [CALCITE-4532] Correct code generated for primitive-object ConstantExpression
Posted by GitBox <gi...@apache.org>.
rubenada commented on pull request #2530:
URL: https://github.com/apache/calcite/pull/2530#issuecomment-925808430
@DonnyZone any chance you could take a quick look at this one?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] gierlachg commented on pull request #2530: [CALCITE-4532] Correct code generated for primitive-object ConstantExpression
Posted by GitBox <gi...@apache.org>.
gierlachg commented on pull request #2530:
URL: https://github.com/apache/calcite/pull/2530#issuecomment-926530672
Of course. Just did it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] gierlachg commented on a change in pull request #2530: [CALCITE-4532] Correct code generated for primitive-object ConstantExpression
Posted by GitBox <gi...@apache.org>.
gierlachg commented on a change in pull request #2530:
URL: https://github.com/apache/calcite/pull/2530#discussion_r715440122
##########
File path: linq4j/src/test/java/org/apache/calcite/linq4j/test/ExpressionTest.java
##########
@@ -1357,6 +1357,25 @@ public void checkBlockBuilder(boolean optimizing, String expected) {
Expressions.toString(Expressions.constant(12.34, BigDecimal.class)));
}
+ @Test void testObjectConstantExpression() {
+ assertEquals("true",
+ Expressions.toString(Expressions.constant(true, Object.class)));
+ assertEquals("(byte)100",
+ Expressions.toString(Expressions.constant((byte) 100, Object.class)));
+ assertEquals("(char)100",
+ Expressions.toString(Expressions.constant((char) 100, Object.class)));
+ assertEquals("(short)100",
+ Expressions.toString(Expressions.constant((short) 100, Object.class)));
+ assertEquals("100",
+ Expressions.toString(Expressions.constant(100, Object.class)));
+ assertEquals("100L",
+ Expressions.toString(Expressions.constant(100L, Object.class)));
+ assertEquals("100.0F",
+ Expressions.toString(Expressions.constant(100F, Object.class)));
+ assertEquals("100.0D",
+ Expressions.toString(Expressions.constant(100D, Object.class)));
+ }
+
Review comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org