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.

Sign up for GitHub

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

Jump to bottom

Support MongoDB case insensitive collection name #1915

Closed
wants to merge 1 commit into from
Closed

Support MongoDB case insensitive collection name #1915

wants to merge 1 commit into from

Conversation

tchunwei
Copy link
Member

@tchunwei tchunwei commented Oct 31, 2019 •
edited by ebyhr
Loading

Fixes #1102

Currently any MongoDB collection with uppercase name like "Hello" could not be queried.
This PR makes it possible by supporting case insensitivity for collection name, providing solution for case 2 mentioned here #1102 (comment), it doesn't help case 1 though.

@cla-bot
Copy link

cla-bot bot commented Oct 31, 2019

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@prestosql.io. For more information, see https://github.com/prestosql/cla.

1 similar comment
@cla-bot
Copy link

cla-bot bot commented Oct 31, 2019

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@prestosql.io. For more information, see https://github.com/prestosql/cla.

@tchunwei tchunwei mentioned this pull request Oct 31, 2019
Closed
@cla-bot
Copy link

cla-bot bot commented Nov 1, 2019

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@prestosql.io. For more information, see https://github.com/prestosql/cla.

1 similar comment
@cla-bot
Copy link

cla-bot bot commented Nov 1, 2019

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@prestosql.io. For more information, see https://github.com/prestosql/cla.

@martint
Copy link
Member

martint commented Nov 4, 2019

@cla-bot check

@cla-bot cla-bot bot added the cla-signed label Nov 4, 2019
@cla-bot
Copy link

cla-bot bot commented Nov 4, 2019

The cla-bot has been summoned, and re-checked this pull request!

@tchunwei tchunwei requested review from findepi and removed request for findepi November 19, 2019 05:29
@findepi
Copy link
Member

findepi commented Nov 19, 2019

@tchunwei could you please rebase on current master and squash commits? Ideally, there should be just one commit after that.

@tchunwei
Copy link
Member Author

@findepi thanks for guiding, has rebased, made it into single commit and force pushed. Kindly let me know if I did it incorrectly, thanks again.

@ebyhr ebyhr self-assigned this Nov 20, 2019
ebyhr
ebyhr reviewed Nov 24, 2019
View reviewed changes
Copy link
Member

@ebyhr ebyhr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tchunwei Sorry for my late response. Could you try focusing on the actual issue (case insensitive database & collection name) before starting the detailed review? It seems the current commit contains unrelated changes for that.

@ebyhr ebyhr assigned tchunwei and unassigned ebyhr Dec 10, 2019
@tchunwei tchunwei closed this Dec 17, 2019
@buffcode
Copy link

@tchunwei Why was this closed?

@tchunwei
Copy link
Member Author

@buffcode that was done unintentionally, was rebasing my code then accidentally wiped my commit, will re-open later

@tchunwei tchunwei reopened this Dec 17, 2019
@tchunwei
Copy link
Member Author

Thank you @ebyhr for your effort guiding me and reviewing the code. Have updated the code based on the feedback for your further review.

@ebyhr ebyhr self-requested a review December 24, 2019 02:05
ebyhr
ebyhr reviewed Jan 21, 2020
View reviewed changes
Copy link
Member

@ebyhr ebyhr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for my late response. Left some comments. The program behavior itself looks fine as you added the test.

presto-mongodb/src/test/java/io/prestosql/plugin/mongodb/MongoQueryRunner.java
MongoClient client = new MongoClient(server.getAddress().getHost(), server.getAddress().getPort());
MongoCollection<Document> collection = client.getDatabase("CamelDB").getCollection("camelTable");

collection.insertOne(new Document(ImmutableMap.of("Name", "asdf", "Value", 1)));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TestMongoIntegrationSmokeTest already has MongoClient object. Please move this body to testCaseInsensitive. Also, I would recommend renaming Camel to TestCaseInsensitive or something.

presto-mongodb/src/test/java/io/prestosql/plugin/mongodb/TestMongoIntegrationSmokeTest.java
assertQuery("SELECT name, value FROM cameldb.cameltable", "SELECT 'asdf', 1");
assertUpdate("INSERT INTO cameldb.cameltable VALUES('qwer', 2)", 1);

assertQuery("SELECT value FROM cameldb.cameltable where name = 'qwer'", "SELECT 2");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add drop table in Presto and drop collection in MongoDB.

presto-mongodb/src/main/java/io/prestosql/plugin/mongodb/MongoInsertTableHandle.java
private final List<MongoColumnHandle> columns;

@JsonCreator
public MongoInsertTableHandle(
@JsonProperty("schemaTableName") SchemaTableName schemaTableName,
@JsonProperty("table") MongoTableHandle table,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add String schemaName (case sensitive) and String tableName (case sensitive) instead of MongoTableHandle. MongoInsertTableHandle is also the same.

presto-mongodb/src/main/java/io/prestosql/plugin/mongodb/MongoMetadata.java
@@ -215,17 +217,16 @@ public boolean usesLegacyTableLayouts()
}

@Override
public ConnectorTableProperties getTableProperties(ConnectorSession session, ConnectorTableHandle table)
public ConnectorTableProperties getTableProperties(ConnectorSession session, ConnectorTableHandle tableHandle)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revert

presto-mongodb/src/main/java/io/prestosql/plugin/mongodb/MongoSession.java
continue;
}

builder.add(name.toLowerCase(ENGLISH));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revert and add .map(name -> name.toLowerCase(ENGLISH)).

presto-mongodb/src/main/java/io/prestosql/plugin/mongodb/MongoSession.java
return databaseName;
}

private String getTableName(String databaseName, String tableName)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is unused in this commit. I suppose this should be called from loadTable method.

Suggested change
private String getTableName(String databaseName, String tableName)
private String getCaseSensitiveTableName(String databaseName, String tableName)

presto-mongodb/src/main/java/io/prestosql/plugin/mongodb/MongoTableHandle.java
private final TupleDomain<ColumnHandle> constraint;

public MongoTableHandle(SchemaTableName schemaTableName)
public MongoTableHandle(SchemaTableName table)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's change the argument to (String databaseName, String tableName). Otherwise, the above comment about case sensitivity is a little misleading.

presto-mongodb/src/main/java/io/prestosql/plugin/mongodb/MongoTableHandle.java
}

@Override
public String toString()
{
return schemaTableName.toString();
return String.format("%s.%s", databaseName, collectionName);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Consistency as ConnectorTableHandle class. Basically, using String.format function to concatinate string is the right choice. Also, we can use static import for String.format.

Suggested change
return String.format("%s.%s", databaseName, collectionName);
return databaseName + ":" + collectionName;

presto-mongodb/src/test/java/io/prestosql/plugin/mongodb/TestMongoSession.java
@@ -35,8 +35,8 @@

public class TestMongoSession
{
private static final MongoColumnHandle COL1 = new MongoColumnHandle("col1", BIGINT, false);
private static final MongoColumnHandle COL2 = new MongoColumnHandle("col2", createUnboundedVarcharType(), false);
private static final MongoColumnHandle COL1 = new MongoColumnHandle("Col1", BIGINT, false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep col1 as a lowercase.

presto-mongodb/src/test/java/io/prestosql/plugin/mongodb/TestMongoTableHandle.java
@@ -26,11 +26,13 @@
@Test
public void testRoundTrip()
{
MongoTableHandle expected = new MongoTableHandle(new SchemaTableName("schema", "table"));
MongoTableHandle expected = new MongoTableHandle("Schema", "Table", TupleDomain.all());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's call another constructor. Also, it would be better to add a test case instead of replacement to avoid unexpected regression.

@zifer123
Copy link

any updated about this?

@tchunwei
Copy link
Member Author

I am sorry I do not have much time for this and this is consider low priority for me so will put this aside first. Also, this PR will be a temporary solution until #2350 is ready. So I think it would be better to wait for #2350 since it is a better approach?

@zifer123
Copy link

I am sorry I do not have much time for this and this is consider low priority for me so will put this aside first. Also, this PR will be a temporary solution until #2350 is ready. So I think it would be better to wait for #2350 since it is a better approach?

ok, thanks

@ebyhr ebyhr mentioned this pull request Apr 16, 2020
Merged
@d4rth-v4d3r
Copy link

Any updates of this?

@ebyhr
Copy link
Member

ebyhr commented May 5, 2020

Covered by #3453

@ebyhr ebyhr closed this May 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@findepi findepi findepi left review comments

@ebyhr ebyhr ebyhr left review comments

Assignees

@tchunwei tchunwei

Labels
cla-signed
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

MongoDB collection name case-sensitivity
7 participants
@tchunwei @martint @findepi @buffcode @zifer123 @d4rth-v4d3r @ebyhr