[CALCITE-7727] Comparing UUID <> '' always returns FALSE - #5191
Conversation
rubenada
left a comment
rubenada
left a comment
There was a problem hiding this comment.
LGTM
nit: I think there's another occurrence of UUID.fromString in RelWriterTest that for consistency should be replaced with the new SqlFunctions.stringToUuid
xuzifu666
left a comment
xuzifu666
left a comment
There was a problem hiding this comment.
LGTM, left some minor comments.
| Long.parseUnsignedLong(digits.substring(16), 16)); | ||
| } | ||
|
|
||
| private static boolean isHexDigit(char c) { |
There was a problem hiding this comment.
This seems to duplicate the SqlParserUtil.isHexDigit method.
There was a problem hiding this comment.
I will remove the duplicate
| * 123e4567-e89b-12d3-a456-42665544000 31 digits | ||
| * </pre></blockquote> | ||
| */ | ||
| public static UUID stringToUuid(String s) { |
There was a problem hiding this comment.
The handling of blank string does not appear to be clearly defined; it needs to be consistent with PostgreSQL.
There was a problem hiding this comment.
Will add a test for this
I will change this |
|
Since this is already approved, I will push an amended commit which performs all three suggested improvements and I will merge once CI passes. Thank you for the feedback! |
Signed-off-by: Mihai Budiu <mbudiu@feldera.com>
mihaibudiu
added
the
LGTM-will-merge-soon
|
4 participants
Jira Link
CALCITE-7727
Changes Proposed
I had mistakenly implemented type coercion between UUID and CHAR/BINARY by converting UUID to CHAR/BINARY.
Postgres does the opposite, and I think it's the right choice. The previous choice leads to the surprising behavior described by the current issue. The new behavior also parallels other type coercions, e.g. comparisons between INT and CHAR, which cast to INT.
The documentation was actually silent about this behavior, so we can claim it was undocumented. I have added this change of semantics to the release notes.
I have also made the cast from string to UUID behave closer to Postgres.