-
Notifications
You must be signed in to change notification settings - Fork 16
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 CreateTableOperation identifier issue, add several table ITs. #1348
Conversation
|
||
CreateTableStart create = | ||
createTable(commandContext.schemaObject().name.keyspace(), tableName).ifNotExists(); | ||
String keyspaceQuoted = "\"%s\"".formatted(commandContext.schemaObject().name.keyspace()); |
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 we not have helper method(s) in CqlIdentifier
to handle quoting? Instead of manually surrounding with double quotes.
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 did not find any method in driver that gives us CreateTableStart back and handle quote automatically.
@NonNull
public static CreateTableStart createTable(@NonNull String tableName) {
return createTable(CqlIdentifier.fromCql(tableName));
}
@NonNull
public static CreateTableStart createTable(@Nullable String keyspace, @NonNull String tableName) {
return createTable(keyspace == null ? null : CqlIdentifier.fromCql(keyspace), CqlIdentifier.fromCql(tableName));
}
I think they design this by the tableName/keyspace as cql representation. Which means if we want quoted string, we need to add by ourselves.
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.
That seems very odd -- there's nothing that takes CqlIdentifier
? We should pass
CqlIdentifier.fromInternal(tableName)
to ensure quoting is used if (and only if) necessary. But I agree, if only String
is allowed then we must quote.
.../java/io/stargate/sgv2/jsonapi/api/v1/tableIntegrationTest/AddTableIndexIntegrationTest.java
Outdated
Show resolved
Hide resolved
@Yuqi-Du let's get this merged to make it easier to add other Table ITs! |
createTable(commandContext.schemaObject().name.keyspace(), tableName).ifNotExists(); | ||
CqlIdentifier keyspaceIdentifier = | ||
CqlIdentifier.fromInternal(commandContext.schemaObject().name.keyspace()); | ||
CqlIdentifier tableIdentifier = CqlIdentifier.fromInternal(tableName); |
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.
Ok good!
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 👍
So in previous implementation of CreateTableOperation,
we are using createTable method from CQL Driver, which uses fromCql method to resolve identifier. This is a problematic, because if things are not quoted, driver will make them case-insensitive, to lower case.
e.g.
keyspace in DB:
Yuqi_Keyspace
because of this string is not quoted, then CQL Driver decides to convert it to
yuqi_keyspace
, which does not exist.So to be consistent with what we did for collection, I just added quote for keyspace and tableName in createTableCommand.
Enable table feature in test resource
add AbstractTableIntegrationTestBase, add few ITs for table.
Checklist