-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Use proper SQL identifier semantics for CALL arguments #11163
Use proper SQL identifier semantics for CALL arguments #11163
Conversation
assertUpdate(optimizeEnabledSession, "ALTER TABLE " + tableName + " EXECUTE \"OPTIMIZE\" (\"file_size_threshold\" => '10B')"); | ||
assertUpdate(optimizeEnabledSession, "ALTER TABLE " + tableName + " EXECUTE \"OPTIMIZE\" (\"FILE_SIZE_THRESHOLD\" => '10B')"); |
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.
This feels wrong. Only one of them should pass.
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.
It works like this:
- the distinction between delimited and not is first erased in
trino/core/trino-main/src/main/java/io/trino/metadata/TableProceduresPropertyManager.java
Line 90 in 2051e78
.map(entry -> new Property(new Identifier(entry.getKey()), entry.getValue())) - then the case is normalized again at
// property names are case-insensitive and normalized to lower case String propertyName = property.getName().getValue().toLowerCase(ENGLISH);
So
- if we want ALTER TABLE EXECUTE (...) properties to be case sensitive, both these places need to be changed/diverged
- do we want all properties to be case-sensitive? especially the table properties?
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.
I would say that we only allow case-insensitive arguments for procedures. I don't see a benefit to allowing case-sensitive.
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.
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.
I think that we should respect the proper identifier semantics.
Although it might not seem very beneficial to allow using case-sensitive arguments for procedures, it's about consistency with the spec, and consistency of behavior across Trino. When adding new features, we generally try to be consistent with the spec wrt identifiers. In the long term, we're going to apply per-spec identifiers everywhere.
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.
It's not just about case sensitivity, but also about allowing characters that are invalid unless it's a delimited (quoted) identifier. Once we support quoted identifiers, we should be consistent with the specification -- if an identifier is quoted, it needs to be treated as is (case conversions for non-english characters can be problematic, too, as they depend on locale)
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.
Filed #11326 to prevent scope creep in this PR.
9b3f81c
to
e7a215e
Compare
e7a215e
to
cff195b
Compare
Ready to review, even if we decide not to address #11163 (comment) within this PR. |
@@ -7891,6 +7891,13 @@ public void testOptimize() | |||
assertUpdate(optimizeEnabledSession, "ALTER TABLE " + tableName + " EXECUTE optimize(file_size_threshold => '10B')"); | |||
assertThat(getTableFiles(tableName)).hasSameElementsAs(compactedFiles); | |||
|
|||
// optimize with delimited procedure name |
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.
Could you please separate tests for procedure name from tests for arguments (put them in different methods)? It seems that they are distinct issues. Also, could you add tests with unquoted names in different case?
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.
This is just a smoke test coverage. I put the together because it's not a detailed test for ALTER TABLE EXECUTE itself. (such test doesn't exist, and I don't want to add it)
assertUpdate(optimizeEnabledSession, "ALTER TABLE " + tableName + " EXECUTE \"OPTIMIZE\" (\"file_size_threshold\" => '10B')"); | ||
assertUpdate(optimizeEnabledSession, "ALTER TABLE " + tableName + " EXECUTE \"OPTIMIZE\" (\"FILE_SIZE_THRESHOLD\" => '10B')"); |
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.
I think that we should respect the proper identifier semantics.
Although it might not seem very beneficial to allow using case-sensitive arguments for procedures, it's about consistency with the spec, and consistency of behavior across Trino. When adding new features, we generally try to be consistent with the spec wrt identifiers. In the long term, we're going to apply per-spec identifiers everywhere.
f41d71b
to
c4a1cce
Compare
Please re-review. |
The class has no shared state.
Co-authored-by: kasiafi <[email protected]>
Co-authored-by: Piotr Findeisen <[email protected]>
c4a1cce
to
4e578ce
Compare
(rebased) |
No description provided.