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/02/24 02:38:19 UTC
[GitHub] [calcite] liyafan82 opened a new pull request #2356: [CALCITE-4510] Weird digests for literals with some user defined types
liyafan82 opened a new pull request #2356:
URL: https://github.com/apache/calcite/pull/2356
We find weird literals for some user defined non-nullable types. Some investigation shows that the problem lies in the `RexLiteral#toJavaString` method.
In particular, it checks the type string suffix with an 8-character string:
```
if (!fullTypeString.endsWith("NOT NULL")) {
```
However, it trims the last 9 characters from the end of the string:
```
sb.append(fullTypeString, 0, fullTypeString.length() - 9);
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] danny0405 commented on a change in pull request #2356: [CALCITE-4510] RexLiteral can produce wrong digest for some user defined types
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #2356:
URL: https://github.com/apache/calcite/pull/2356#discussion_r583591659
##########
File path: core/src/main/java/org/apache/calcite/rel/type/RelDataType.java
##########
@@ -40,6 +40,11 @@
int SCALE_NOT_SPECIFIED = Integer.MIN_VALUE;
int PRECISION_NOT_SPECIFIED = -1;
+ /**
+ * Suffix for the digests of non-nullable types.
+ */
+ String NON_NULLABLE_TYPE_DIGEST_SUFFIX = " NOT NULL";
Review comment:
Me too, i prefer the string constant `" NOT NULL"` directly because it is stragit-forward.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on pull request #2356: [CALCITE-4510] RexLiteral can produce wrong digest for some user defined types
Posted by GitBox <gi...@apache.org>.
rubenada commented on pull request #2356:
URL: https://github.com/apache/calcite/pull/2356#issuecomment-786567467
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on pull request #2356: [CALCITE-4510] RexLiteral can produce wrong digest for some user defined types
Posted by GitBox <gi...@apache.org>.
vlsi commented on pull request #2356:
URL: https://github.com/apache/calcite/pull/2356#issuecomment-786572856
Can you please add the test to verify the change?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] danny0405 commented on a change in pull request #2356: [CALCITE-4510] RexLiteral can produce wrong digest for some user defined types
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #2356:
URL: https://github.com/apache/calcite/pull/2356#discussion_r583591659
##########
File path: core/src/main/java/org/apache/calcite/rel/type/RelDataType.java
##########
@@ -40,6 +40,11 @@
int SCALE_NOT_SPECIFIED = Integer.MIN_VALUE;
int PRECISION_NOT_SPECIFIED = -1;
+ /**
+ * Suffix for the digests of non-nullable types.
+ */
+ String NON_NULLABLE_TYPE_DIGEST_SUFFIX = " NOT NULL";
Review comment:
Me too, i prefer the string constant `" NOT NULL"` directly because it is straight-forward.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on pull request #2356: [CALCITE-4510] RexLiteral can produce wrong digest for some user defined types
Posted by GitBox <gi...@apache.org>.
rubenada commented on pull request #2356:
URL: https://github.com/apache/calcite/pull/2356#issuecomment-787756639
> Sure. I have added one to emulate the user defined type that has revealed the problem.
@liyafan82 have you pushed your latest changes? I cannot see the test
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] danny0405 commented on a change in pull request #2356: [CALCITE-4510] RexLiteral can produce wrong digest for some user defined types
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #2356:
URL: https://github.com/apache/calcite/pull/2356#discussion_r584559989
##########
File path: core/src/main/java/org/apache/calcite/rel/type/RelDataTypeImpl.java
##########
@@ -50,6 +50,12 @@
*/
public abstract class RelDataTypeImpl
implements RelDataType, RelDataTypeFamily {
+
+ /**
+ * Suffix for the digests of non-nullable types.
+ */
+ public static final String NON_NULLABLE_TYPE_DIGEST_SUFFIX = " NOT NULL";
+
Review comment:
Is there any special reason that the variable `NON_NULLABLE_TYPE_DIGEST_SUFFIX` belongs to this class and can we give it a shorter name ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2356: [CALCITE-4510] RexLiteral can produce wrong digest for some user defined types
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2356:
URL: https://github.com/apache/calcite/pull/2356#discussion_r583553701
##########
File path: core/src/main/java/org/apache/calcite/rel/type/RelDataType.java
##########
@@ -40,6 +40,11 @@
int SCALE_NOT_SPECIFIED = Integer.MIN_VALUE;
int PRECISION_NOT_SPECIFIED = -1;
+ /**
+ * Suffix for the digests of non-nullable types.
+ */
+ String NON_NULLABLE_TYPE_DIGEST_SUFFIX = " NOT NULL";
Review comment:
Frankly speaking, I do not like how this leaks to the public API.
Could you declare the constant in a Calcite-private location to prevent it accidental use in the client code?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] liyafan82 commented on a change in pull request #2356: [CALCITE-4510] RexLiteral can produce wrong digest for some user defined types
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #2356:
URL: https://github.com/apache/calcite/pull/2356#discussion_r584580953
##########
File path: core/src/main/java/org/apache/calcite/rel/type/RelDataTypeImpl.java
##########
@@ -50,6 +50,12 @@
*/
public abstract class RelDataTypeImpl
implements RelDataType, RelDataTypeFamily {
+
+ /**
+ * Suffix for the digests of non-nullable types.
+ */
+ public static final String NON_NULLABLE_TYPE_DIGEST_SUFFIX = " NOT NULL";
+
Review comment:
> Is there any special reason that the variable `NON_NULLABLE_TYPE_DIGEST_SUFFIX` belongs to this class and can we give it a shorter name ?
Sure. I have refactored the code to give it a shorter name.
I think the reason to place it in this class is that, it defines the common suffix for all Calcite internal types, and this class is the common base class for all Calcite internal types.
One alternative that I can think of is to put it into the `RelDataType` interface, and annotate it as an internal API.
However, I personally prefer to place it in class `RelDataTypeImpl`, as in this PR.
WDYT?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on a change in pull request #2356: [CALCITE-4510] RexLiteral can produce wrong digest for some user defined types
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2356:
URL: https://github.com/apache/calcite/pull/2356#discussion_r583594887
##########
File path: core/src/main/java/org/apache/calcite/rel/type/RelDataType.java
##########
@@ -40,6 +40,11 @@
int SCALE_NOT_SPECIFIED = Integer.MIN_VALUE;
int PRECISION_NOT_SPECIFIED = -1;
+ /**
+ * Suffix for the digests of non-nullable types.
+ */
+ String NON_NULLABLE_TYPE_DIGEST_SUFFIX = " NOT NULL";
Review comment:
I understand @vlsi 's concern. I guess it would be better to define this constant e.g. in RelDataTypeImpl rather than RelDataType.
@danny0405 I think it is cleaner to "centralize" this constant in one single place.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] liyafan82 commented on pull request #2356: [CALCITE-4510] RexLiteral can produce wrong digest for some user defined types
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on pull request #2356:
URL: https://github.com/apache/calcite/pull/2356#issuecomment-787623200
> Can you please add the test to verify the change?
Sure. I have added one to emulate the user defined type that has revealed the problem.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] liyafan82 merged pull request #2356: [CALCITE-4510] RexLiteral can produce wrong digest for some user defined types
Posted by GitBox <gi...@apache.org>.
liyafan82 merged pull request #2356:
URL: https://github.com/apache/calcite/pull/2356
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] liyafan82 commented on pull request #2356: [CALCITE-4510] RexLiteral can produce wrong digest for some user defined types
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on pull request #2356:
URL: https://github.com/apache/calcite/pull/2356#issuecomment-829743655
Merging. Thanks.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] liyafan82 commented on pull request #2356: [CALCITE-4510] RexLiteral can produce wrong digest for some user defined types
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on pull request #2356:
URL: https://github.com/apache/calcite/pull/2356#issuecomment-814806729
Many thanks to all reviewers for the feedback.
If there are no objections, I want to merge this PR in a few days.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] liyafan82 commented on pull request #2356: [CALCITE-4510] RexLiteral can produce wrong digest for some user defined types
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on pull request #2356:
URL: https://github.com/apache/calcite/pull/2356#issuecomment-787782702
> > Sure. I have added one to emulate the user defined type that has revealed the problem.
>
> @liyafan82 have you pushed your latest changes? I cannot see the test
@rubenada Pushed just now. Please forgive me for my carelessness.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] liyafan82 commented on a change in pull request #2356: [CALCITE-4510] RexLiteral can produce wrong digest for some user defined types
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #2356:
URL: https://github.com/apache/calcite/pull/2356#discussion_r584436381
##########
File path: core/src/main/java/org/apache/calcite/rel/type/RelDataType.java
##########
@@ -40,6 +40,11 @@
int SCALE_NOT_SPECIFIED = Integer.MIN_VALUE;
int PRECISION_NOT_SPECIFIED = -1;
+ /**
+ * Suffix for the digests of non-nullable types.
+ */
+ String NON_NULLABLE_TYPE_DIGEST_SUFFIX = " NOT NULL";
Review comment:
@vlsi 's concern sounds reasonable, so I have moved the literal to class `RelDataTypeImpl` according to @rubenada 's suggestion.
IMO, extracting a constant to a static final field makes the code more structured and maintainable, and it also helps avoiding some bugs and typos (like the one in this issue).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] danny0405 commented on a change in pull request #2356: [CALCITE-4510] RexLiteral can produce wrong digest for some user defined types
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #2356:
URL: https://github.com/apache/calcite/pull/2356#discussion_r586290925
##########
File path: core/src/main/java/org/apache/calcite/rel/type/RelDataTypeImpl.java
##########
@@ -50,6 +50,12 @@
*/
public abstract class RelDataTypeImpl
implements RelDataType, RelDataTypeFamily {
+
+ /**
+ * Suffix for the digests of non-nullable types.
+ */
+ public static final String NON_NULLABLE_TYPE_DIGEST_SUFFIX = " NOT NULL";
+
Review comment:
Fine with the change.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org