[CALCITE-4510] RexLiteral can produce wrong digest for some user defined types #2356
Conversation
liyafan82
force-pushed
the
fly_0223_dg
branch
4 times, most recently
from
61396dd
to
3ce0f1f
Compare
liyafan82
changed the title
|
LGTM |
| /** | ||
| * Suffix for the digests of non-nullable types. | ||
| */ | ||
| String NON_NULLABLE_TYPE_DIGEST_SUFFIX = " NOT NULL"; |
There was a problem hiding this 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?
There was a problem hiding this comment.
Me too, i prefer the string constant " NOT NULL" directly because it is straight-forward.
There was a problem hiding this 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.
There was a problem hiding this 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).
|
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. |
@liyafan82 have you pushed your latest changes? I cannot see the test |
@rubenada Pushed just now. Please forgive me for my carelessness. |
| * Suffix for the digests of non-nullable types. | ||
| */ | ||
| public static final String NON_NULLABLE_TYPE_DIGEST_SUFFIX = " NOT NULL"; | ||
|
|
There was a problem hiding this 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 ?
There was a problem hiding this comment.
Is there any special reason that the variable
NON_NULLABLE_TYPE_DIGEST_SUFFIXbelongs 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?
liyafan82
added
the
LGTM-will-merge-soon
|
Many thanks to all reviewers for the feedback. |
|
Merging. Thanks. |
We find weird literals for some user defined non-nullable types. Some investigation shows that the problem lies in the
RexLiteral#toJavaStringmethod.In particular, it checks the type string suffix with an 8-character string:
However, it trims the last 9 characters from the end of the string: