Skip to content
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 of ANSI SQL reserved key word 'key' in JdbcUtil.java #4952

Closed
sungwy-backup opened this issue Jun 3, 2022 · 5 comments
Closed

Use of ANSI SQL reserved key word 'key' in JdbcUtil.java #4952

sungwy-backup opened this issue Jun 3, 2022 · 5 comments

Comments

@sungwy-backup
Copy link

The word 'key' is used as a column name for the metadata table 'iceberg_namespace_properties' in the current implementation of Iceberg's JDBC Catalog. This is a reserved keyword according to ANSI SQL standards, and hence such schema would cause issues in ANSI SQL compliant databases.

static final String NAMESPACE_PROPERTY_KEY = "key";

@sungwy-backup
Copy link
Author

@felixYyu @nssalian @rdblue
Hello folks! Just wanted to tag you to see if you had any thoughts on this issue.

At the very least I think we could expect companies using managed SQL infrastructures with varying degrees in strictness of lint checks getting unnecessary noise or failures from having to deal with a reserved key word as a column.

It doesn't look like this change has made it to a release yet, but other open source projects are already looking into onboarding this implementation, so it would be good to understand whether this schema will be staying for good, or it might be worth changing this now.

https://github.com/trinodb/trino/pull/11772/files#diff-e4db49a223035ceaad88fc346e7558b55ff83485ac36bf92e97409b36ceb9717R102

@rdblue
Copy link
Contributor

rdblue commented Jun 8, 2022

@sungwy, if this hasn't been released yet, then let's fix it. Can you open a PR?

@sungwy-backup
Copy link
Author

Sure thing - I am doing this work as a part of a company, so I will raise a ticket for Open Source contribution, and open the PR soon on the company account. Will keep you posted

@rdblue
Copy link
Contributor

rdblue commented Jun 8, 2022

Great, thank you for doing this!

@kbendick
Copy link
Contributor

As #5017 has been merged and the field has been renamed, I’m going to close this issue.

Please create a new issue if there’s a new question, or reopen this issue if the follow up relates directly to the field name (or potentially open a new isssue that references this one).

Thank you for reporting @sungwy and thank you for the patch @syun64!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants