-
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
Add support for case sensitive identifiers #2350
Add support for case sensitive identifiers #2350
Conversation
bcd9eb1
to
844afe7
Compare
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.
Some initial comments
presto-main/src/main/java/io/prestosql/metadata/MetadataUtil.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/io/prestosql/metadata/MetadataUtil.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/io/prestosql/metadata/QualifiedObjectName.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/io/prestosql/metadata/QualifiedObjectName.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/io/prestosql/metadata/QualifiedObjectName.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/io/prestosql/metadata/QualifiedObjectName.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/io/prestosql/metadata/QualifiedObjectName.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/io/prestosql/metadata/NameCanonicalizer.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/io/prestosql/metadata/NameCanonicalizer.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/io/prestosql/sql/analyzer/Analysis.java
Outdated
Show resolved
Hide resolved
af6c013
to
9b32865
Compare
9b32865
to
cd797bf
Compare
cd797bf
to
c194d5c
Compare
@martint Thanks for your insights. Have applied the comments |
I have this problem, and, any update about this? |
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 squash all commits together. It is not clear what is the boundary between them and that way it will be easier for you to iterate.
import static java.util.Locale.ENGLISH; | ||
import static java.util.Objects.requireNonNull; | ||
|
||
public class NamePart |
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.
Can you please implement toString()
in a way it throws an exception.
@JsonCreator | ||
public NamePart(@JsonProperty("value") String value, @JsonProperty("delimited") boolean delimited) | ||
{ | ||
checkArgument(!value.isEmpty(), "value is empty"); |
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.
rnn(value)
import static java.util.Locale.ENGLISH; | ||
import static java.util.Objects.requireNonNull; | ||
|
||
public class NamePart |
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.
Identifier?
{ | ||
Optional<CatalogMetadata> metadata = getOptionalCatalogMetadata(session, catalogName); | ||
if (metadata.isPresent()) { | ||
return (identifier, delimited) -> metadata.get().getMetadata().canonicalize(session.toConnectorSession(metadata.get().getCatalogName()), identifier, delimited); |
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 this is functional? I would simply convert getNameCanonicalizer
to canonicalize
* Canonicalizes the provided SQL identifier according to connector-specific rules | ||
* for the purpose of providing the name in metadata APIs | ||
*/ | ||
default String canonicalize(ConnectorSession session, String identifier, boolean delimited) |
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 we can skip passing session here. The way how remote data source is typically static and it should not change at runtime.
Also this will make this change easier. If we ever hit a use case where session is needed we can always add it then, it should be easy change.
@@ -67,7 +67,7 @@ public void setUp() | |||
{ |
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.
Please add plenty of tests to BaseConnectorTest
.
Things to cover:
- whether output column aliases for remain their case
- CREATE TABLE with case sensitive names
- CREATE TABLE with columns that differs only with casing, do select, insert, update, delete from from that table
- The similar like above with columns, but for schemas and catalogs.
% method and classes renames and other mechanical refactorings. |
@Praveen2112 and @kokosing - this PR has become inactive. If you're still interested in working on it, please let us know. We're working on closing out old and inactive PRs, so if you're too busy or this has too many merge conflicts to be worth picking back up, we'll be making another pass to close it out in a few weeks. |
Relates to #17.
Supersedes #1302.