-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Fix failure when creating a schema with non-lowercase username on Iceberg Glue catalog #16124
Conversation
35c3f64
to
c251041
Compare
name
in TrinoPrincipal
name
in TrinoPrincipal
when PrincipalType
is USER
f061439
to
7dd6bc9
Compare
CI hit #16137 |
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.
Fixes https://github.com/trinodb/trino/issues/16116
in the description.
I'm not sure of the impact of this change, and this impacts security commands, so I'm not comfortable approving right now. The |
Per the SQL spec, identifiers should only be canonicalized (to upper-case) when they are not "delimited" (i.e., in double quotes). Obviously, making such a change wholesale would break compatibility. Also, many systems have different rules. Therefore, the plan so far is to let plugins decide how to canonicalize identifiers (schemas, tables, columns, users, roles, etc) before the values are used in other connector/plugin API calls. The change proposed in this PR, as it stands, may break compatibility for plugins that are case-insensitive. |
Should we instead fix this for Glue by lowercasing the name in |
7dd6bc9
to
470794e
Compare
name
in TrinoPrincipal
when PrincipalType
is USER
@ebyhr can you pls rebase to address the conflicts? |
470794e
to
b58927e
Compare
Hi @ebyhr, Is anything remaining in this PR? |
@@ -253,7 +253,7 @@ public Optional<TrinoPrincipal> getNamespacePrincipal(ConnectorSession session, | |||
public void createNamespace(ConnectorSession session, String namespace, Map<String, Object> properties, TrinoPrincipal owner) | |||
{ | |||
checkArgument(owner.getType() == PrincipalType.USER, "Owner type must be USER"); | |||
checkArgument(owner.getName().equals(session.getUser()), "Explicit schema owner is not supported"); | |||
checkArgument(owner.getName().equalsIgnoreCase(session.getUser()), "Explicit schema owner is not supported"); |
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.
why would owner name get lowercased?
why not compare equality with session.getUser().toLower
?
public void testCreateSchemaWithNonLowercaseOwnerName() | ||
{ | ||
Session newSession = Session.builder(getSession()) | ||
.setIdentity(Identity.ofUser("User")) |
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.
Should we have a reusable (connector) (smoke) test?
bbe5ebf
to
1e189a8
Compare
1e189a8
to
03b6dc1
Compare
03b6dc1
to
2576094
Compare
/test-with-secrets sha=25760946b0a17393dcf1f1f6cdc0a6bda3fce2f4 |
The CI workflow run with tests that require additional secrets finished as failure: https://github.com/trinodb/trino/actions/runs/4550668692 |
TrinoPrincipal sets the owner name in lowercase.
2576094
to
b2f71c2
Compare
/test-with-secrets sha=b2f71c2b30a4ca8ef1b38baca593b3867352b405 |
The CI workflow run with tests that require additional secrets finished as failure: https://github.com/trinodb/trino/actions/runs/4565827240 |
Description
Don't lowercase
name
inTrinoPrincipal
whenPrincipalType
isUSER
.Fixes #16116
Release notes
(x) Release notes are required, with the following suggested text: