-
Notifications
You must be signed in to change notification settings - Fork 5.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Do not review][Native] Add e2e tests for UUID type #23301
base: master
Are you sure you want to change the base?
Conversation
assertQuery("SELECT typeof(array_constructor(uuid(), uuid()))"); | ||
|
||
// Valid UUID. | ||
assertQuery("SELECT cast('33355449-2c7d-43d7-967a-f53cd23215ad' AS uuid)"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these queries run on coordinator or worker?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am following up with tests for CTAS + INSERTs etc. These SQL are largely co-ordinator.
The review went out too soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mbasmanova : UUID would need a few more changes to have a better user experience. It has equality comparison, but lt, lte, gt, gte, between comparisons are not available. So authoring SQL has limitations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aditi-pandit Aditi, thank you for sharing these findings. Will you be adding missing functionality to Velox?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mbasmanova : Created issues facebookincubator/velox#10584 and facebookincubator/velox#10585. We will follow up there.
...ecution/src/test/java/com/facebook/presto/nativeworker/AbstractTestNativeGeneralQueries.java
Show resolved
Hide resolved
"WHERE a = b", tmpTableName, tmpTableName)); | ||
|
||
// Velox missing lt for UUID. | ||
assertQueryFails(format("SELECT CAST(a AS VARCHAR), CAST(b AS VARCHAR) FROM " + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Query failures for lt, lte, gt, gte, between
// Invalid cast on both Presto Java and Native. | ||
assertQueryFails(format("SELECT CAST(CAST(c_uuid as uuid) AS INTEGER) FROM %s", tmpTableName), ".*Cannot cast uuid to integer.*"); | ||
// Cast from UUID->VARBINARY not supported on Velox. | ||
assertQueryFails(format("SELECT CAST(CAST(c_uuid AS uuid) AS VARBINARY) FROM %s", tmpTableName), ".*Cannot cast UUID to VARBINARY.*"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cast to Varbinary failure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! (docs)
Pull branch, local docs build, review the built doc. Thanks!
Description
Velox supports UUID type already.
Add e2e tests for SQL using UUID and remove the restriction documented.