Add dbref field support for mongodb connector;#8549
Conversation
There was a problem hiding this comment.
Thanks for working on this!
Please add a test to TestMongoIntegrationSmokeTest. The test should create DBRef using MongoDB Java library and validate the result in Trino SQL.
CI failure cause is
2021-07-14T06:17:29.7767740Z [ERROR] src/main/java/io/trino/plugin/mongodb/MongoPageSource.java:[84] (regexp) RegexpMultiline: Multiple consecutive blank lines
2021-07-14T06:17:29.7769978Z [ERROR] src/main/java/io/trino/plugin/mongodb/MongoSession.java:[656,69] (whitespace) ParenPad: '(' is followed by whitespace.
If you haven't yet configured codestyle in IntelliJ, I would recommend setting https://github.com/trinodb/trino/blob/master/DEVELOPMENT.md#code-style.
Also, please remove ; from the commit message.
plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoSession.java
Outdated
Show resolved
Hide resolved
|
@ebyhr Please have a look. Made the changes and added the test case. :) Test case description: The test case ensures that the column of dbref type is poppulated as well as has a value parsed. This is done by asserting that the count of documents where the dbref column is not null is equal to 1, as we have inserted only one document in the test case. If column was not found or value was not parsed, the test case would fail. The test case passes and works and confirms that the dbref value is getting poppulated. |
plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoSession.java
Outdated
Show resolved
Hide resolved
plugin/trino-mongodb/src/test/java/io/trino/plugin/mongodb/TestMongoIntegrationSmokeTest.java
Outdated
Show resolved
Hide resolved
plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoSession.java
Outdated
Show resolved
Hide resolved
|
Thanks for the review. Let me fix them and get back in a day or two. |
ebyhr
left a comment
There was a problem hiding this comment.
Almost good to me. Please squash commits into one and update the commit title:
Add DBRef field support for MongoDB connector
plugin/trino-mongodb/src/test/java/io/trino/plugin/mongodb/TestMongoIntegrationSmokeTest.java
Outdated
Show resolved
Hide resolved
plugin/trino-mongodb/src/test/java/io/trino/plugin/mongodb/TestMongoIntegrationSmokeTest.java
Outdated
Show resolved
Hide resolved
plugin/trino-mongodb/src/test/java/io/trino/plugin/mongodb/TestMongoIntegrationSmokeTest.java
Outdated
Show resolved
Hide resolved
plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoPageSource.java
Outdated
Show resolved
Hide resolved
plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoSession.java
Outdated
Show resolved
Hide resolved
plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoSession.java
Outdated
Show resolved
Hide resolved
plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoPageSource.java
Outdated
Show resolved
Hide resolved
90ed9c9 to
7037eca
Compare
ebyhr
left a comment
There was a problem hiding this comment.
Looks good except for minor comments.
plugin/trino-mongodb/src/test/java/io/trino/plugin/mongodb/TestMongoIntegrationSmokeTest.java
Outdated
Show resolved
Hide resolved
plugin/trino-mongodb/src/test/java/io/trino/plugin/mongodb/TestMongoIntegrationSmokeTest.java
Outdated
Show resolved
Hide resolved
7037eca to
a93a712
Compare
|
@ebyhr all good in this one now ? |
|
Merged, thanks! |
|
I'm testing latest version of trino (docker image 371). He dosn't support BinData as DBRef $id example |
|
@messaoudi-mounir It will be supported in #11122. |
Fixes #3134.
Description: Added explicit case for handling if the type of a field is DBRef. In such case, we extract the id, database name and collection name from the object and convert that into a row. This change enables support at the MongoSession level. With this change, the column starts showing up in the information schema. However, all the values come as null because the writeBlock() method still needs to add the support.
To do this, we need add support at the MongoPageSource. This is done by aadding a case in the writeBlock() method.