First draft of Mongodb plugin #3337

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
8 participants
@miniway
Contributor

miniway commented Jul 23, 2015

No description provided.

@electrum

This comment has been minimized.

Show comment
Hide comment
@electrum

electrum Jul 23, 2015

Member

This looks awesome! Very complete docs, too!

Nit: the preferred capitalization, from looking at their website, seems to be "MongoDB"

Member

electrum commented Jul 23, 2015

This looks awesome! Very complete docs, too!

Nit: the preferred capitalization, from looking at their website, seems to be "MongoDB"

+ {
+ super(session, workers);
+
+ server = new MongoServer(new SyncMemoryBackend());

This comment has been minimized.

@electrum

electrum Jul 23, 2015

Member

Nice! An in-memory Java MongoDB server.

@electrum

electrum Jul 23, 2015

Member

Nice! An in-memory Java MongoDB server.

@electrum

View changes

presto-docs/src/main/sphinx/connector.rst
@@ -18,3 +18,4 @@ from different data sources.
connector/postgresql
connector/system
connector/tpch
+ connector/mongodb

This comment has been minimized.

@electrum

electrum Jul 23, 2015

Member

Please keep these sorted

@electrum

electrum Jul 23, 2015

Member

Please keep these sorted

This comment has been minimized.

@miniway

miniway Jul 27, 2015

Contributor

moved to the sorted position, right before mysql

@miniway

miniway Jul 27, 2015

Contributor

moved to the sorted position, right before mysql

@electrum

View changes

presto-mongodb/pom.xml
+ <dependency>
+ <groupId>io.airlift</groupId>
+ <artifactId>bootstrap</artifactId>
+ <scope>provided</scope>

This comment has been minimized.

@electrum

electrum Jul 23, 2015

Member

Take a look at the latest POMs for the other connectors such as example-http. They have a section <!-- Presto SPI --> which has provided dependencies that are part of the SPI. Everything else including Airlift classes should not be provided since they aren't part of the SPI. This was fixed in all the connectors relatively recently.

@electrum

electrum Jul 23, 2015

Member

Take a look at the latest POMs for the other connectors such as example-http. They have a section <!-- Presto SPI --> which has provided dependencies that are part of the SPI. Everything else including Airlift classes should not be provided since they aren't part of the SPI. This was fixed in all the connectors relatively recently.

This comment has been minimized.

@miniway

miniway Jul 27, 2015

Contributor

cleaned up pom.xml based on example-http plugin

@miniway

miniway Jul 27, 2015

Contributor

cleaned up pom.xml based on example-http plugin

@electrum

View changes

presto-mongodb/src/main/java/com/facebook/presto/mongodb/MongoSession.java
+ }
+ }
+ // If rangeConjuncts is null, then the range was ALL, which should already have been checked for
+ checkState(!rangeConjuncts.isEmpty());

This comment has been minimized.

@electrum

electrum Jul 23, 2015

Member

Guava recently added a verify() (in Verify) for these types of runtime assertions.

@electrum

electrum Jul 23, 2015

Member

Guava recently added a verify() (in Verify) for these types of runtime assertions.

This comment has been minimized.

@miniway

miniway Jul 27, 2015

Contributor

changed into Verify.verify

@miniway

miniway Jul 27, 2015

Contributor

changed into Verify.verify

@electrum

View changes

...ngodb/src/main/java/com/facebook/presto/mongodb/MongoHandleResolver.java
+ private final String connectorId;
+
+ @Inject
+ public MongoHandleResolver(@Named("connectorId") String connectorId)

This comment has been minimized.

@electrum

electrum Jul 23, 2015

Member

Rather than @Named, create a class like RaptorConnectorId

@electrum

electrum Jul 23, 2015

Member

Rather than @Named, create a class like RaptorConnectorId

This comment has been minimized.

@miniway

miniway Jul 27, 2015

Contributor

MongoConnectorId has been added.

@miniway

miniway Jul 27, 2015

Contributor

MongoConnectorId has been added.

@akshatnair

View changes

presto-docs/src/main/sphinx/connector/mongodb.rst
+``mongodb.schema-collection``
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+As the MongoDB is a document database, there's no fixed schema information in the system. So a special collection in each MongoDB database should defines the schema of all tables. Please refer the Table definitions section for the details.

This comment has been minimized.

@akshatnair

akshatnair Oct 14, 2015

Member

You can use the :ref: tag for referencing Table definitions section

@akshatnair

akshatnair Oct 14, 2015

Member

You can use the :ref: tag for referencing Table definitions section

This comment has been minimized.

@miniway

miniway Oct 19, 2015

Contributor

Thanks for the tip.

@miniway

miniway Oct 19, 2015

Contributor

Thanks for the tip.

@akshatnair

View changes

presto-docs/src/main/sphinx/connector/mongodb.rst
+
+As the MongoDB is a document database, there's no fixed schema information in the system. So a special collection in each MongoDB database should defines the schema of all tables. Please refer the Table definitions section for the details.
+
+At startup, this plugin try guessing fields' types, but it might not correct for your collection. In that case, you need to modify it manually. ``CREATE TABLE`` and ``CREATE TABLE AS SELECT`` will create an entry for you.

This comment has been minimized.

@akshatnair

akshatnair Oct 14, 2015

Member

this plugin tries
it might not be

@akshatnair

akshatnair Oct 14, 2015

Member

this plugin tries
it might not be

@akshatnair

View changes

presto-docs/src/main/sphinx/connector/mongodb.rst
+^^^^^^^^^^^^^^^^^^^^^^^^^
+
+The maximum wait time in milliseconds that a thread may wait for a connection to become available.
+A value of 0 means that it will not wait. A negative value means to wait indefinitely.y wait for a connection to become available.

This comment has been minimized.

@akshatnair

akshatnair Oct 14, 2015

Member

wait indefinitely for

@akshatnair

akshatnair Oct 14, 2015

Member

wait indefinitely for

@akshatnair

View changes

presto-docs/src/main/sphinx/connector/mongodb.rst
+``mongodb.required-replica-set``
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Limits the number of elements returned in one batch. A cursor typically fetches a batch of result objects and store them locally.

This comment has been minimized.

@akshatnair

akshatnair Oct 14, 2015

Member

and stores them

@akshatnair

akshatnair Oct 14, 2015

Member

and stores them

@akshatnair

View changes

presto-docs/src/main/sphinx/connector/mongodb.rst
+
+.. note::
+ There's no way for the plugin to detect a collection is deleted.
+ You need to delete the entry by ``db.getCollection("_schema").remove( { table: deleted_table_name })``. Or please drop a collection by ``drop table table_name`` at Presto shell.

This comment has been minimized.

@akshatnair

akshatnair Oct 14, 2015

Member

If this db.getCollection() is something users do not care about then we should remove this.

@akshatnair

akshatnair Oct 14, 2015

Member

If this db.getCollection() is something users do not care about then we should remove this.

This comment has been minimized.

@miniway

miniway Oct 19, 2015

Contributor

User might wonder why a table is still shown at show tables after he have deleted a collection through his mongo shell. This is a node for this case.

@miniway

miniway Oct 19, 2015

Contributor

User might wonder why a table is still shown at show tables after he have deleted a collection through his mongo shell. This is a node for this case.

+=============== ========= ========= =============================
+``name`` required string Name of the column in the Presto table.
+``type`` required string Presto type of the column.
+``hidden`` optional boolean Hides the column from ``DESCRIBE <table name>`` and ``SELECT *``. Defaults to ``false``.

This comment has been minimized.

@akshatnair

akshatnair Oct 14, 2015

Member

Any special use case for a column to be made hidden?

@akshatnair

akshatnair Oct 14, 2015

Member

Any special use case for a column to be made hidden?

This comment has been minimized.

@miniway

miniway Oct 19, 2015

Contributor

Basically, users might not want to expose some columns of a table. I guess why Presto has hidden property in the ColumnMetadata

@miniway

miniway Oct 19, 2015

Contributor

Basically, users might not want to expose some columns of a table. I guess why Presto has hidden property in the ColumnMetadata

+ try {
+ columns.put(tableName, getTableMetadata(session, tableName).getColumns());
+ }
+ catch (NotFoundException e) {

This comment has been minimized.

@akshatnair

akshatnair Oct 15, 2015

Member

May be log this error as a debug message.

@akshatnair

akshatnair Oct 15, 2015

Member

May be log this error as a debug message.

This comment has been minimized.

@miniway

miniway Oct 19, 2015

Contributor

Thanks for the comment, I've add a debug logging

@miniway

miniway Oct 19, 2015

Contributor

Thanks for the comment, I've add a debug logging

@akshatnair

This comment has been minimized.

Show comment
Hide comment
@akshatnair

akshatnair Oct 15, 2015

Member

looks good

Member

akshatnair commented Oct 15, 2015

looks good

@sl33nyc

View changes

...mongodb/src/main/java/com/facebook/presto/mongodb/ObjectIdFunctions.java
+import static com.facebook.presto.metadata.OperatorType.EQUAL;
+import static com.facebook.presto.metadata.OperatorType.NOT_EQUAL;
+
+public class ObjectIdFunctions

This comment has been minimized.

@sl33nyc

sl33nyc Oct 16, 2015

FYI, I merged the changes in this PR with tag 0.122 and attempted to run on my MongoDB instance. The server threw:

2015-10-14T16:09:54.669-0400    ERROR   main    com.facebook.presto.server.PrestoServer [GREATER_THAN, BETWEEN, LESS_THAN_OR_EQUAL, GREATER_THAN_OR_EQUAL, HASH_CODE, LESS_THAN] missing for ObjectId
java.lang.IllegalStateException: [GREATER_THAN, BETWEEN, LESS_THAN_OR_EQUAL, GREATER_THAN_OR_EQUAL, HASH_CODE, LESS_THAN] missing for ObjectId
        at com.facebook.presto.metadata.MetadataManager.verifyComparableOrderableContract(MetadataManager.java:227)
        at com.facebook.presto.server.PluginManager.loadPlugins(PluginManager.java:157)
        at com.facebook.presto.server.PrestoServer.run(PrestoServer.java:111)
        at com.facebook.presto.server.PrestoServer.main(PrestoServer.java:62)

I added the missing ObjectId operators to ObjectIdFunctions and now have a running presto server.

@sl33nyc

sl33nyc Oct 16, 2015

FYI, I merged the changes in this PR with tag 0.122 and attempted to run on my MongoDB instance. The server threw:

2015-10-14T16:09:54.669-0400    ERROR   main    com.facebook.presto.server.PrestoServer [GREATER_THAN, BETWEEN, LESS_THAN_OR_EQUAL, GREATER_THAN_OR_EQUAL, HASH_CODE, LESS_THAN] missing for ObjectId
java.lang.IllegalStateException: [GREATER_THAN, BETWEEN, LESS_THAN_OR_EQUAL, GREATER_THAN_OR_EQUAL, HASH_CODE, LESS_THAN] missing for ObjectId
        at com.facebook.presto.metadata.MetadataManager.verifyComparableOrderableContract(MetadataManager.java:227)
        at com.facebook.presto.server.PluginManager.loadPlugins(PluginManager.java:157)
        at com.facebook.presto.server.PrestoServer.run(PrestoServer.java:111)
        at com.facebook.presto.server.PrestoServer.main(PrestoServer.java:62)

I added the missing ObjectId operators to ObjectIdFunctions and now have a running presto server.

This comment has been minimized.

@miniway

miniway Oct 19, 2015

Contributor

Thanks for catching this. I've added missing operators GREATER_THAN, BETWEEN, LESS_THAN_OR_EQUAL, GREATER_THAN_OR_EQUAL, HASH_CODE, LESS_THAN

@miniway

miniway Oct 19, 2015

Contributor

Thanks for catching this. I've added missing operators GREATER_THAN, BETWEEN, LESS_THAN_OR_EQUAL, GREATER_THAN_OR_EQUAL, HASH_CODE, LESS_THAN

@miniway

This comment has been minimized.

Show comment
Hide comment
@miniway

miniway Oct 19, 2015

Contributor

I've rebased on 0.123-SNAPSHOT and fixed various typos in the document.

Contributor

miniway commented Oct 19, 2015

I've rebased on 0.123-SNAPSHOT and fixed various typos in the document.

@gonewandering

This comment has been minimized.

Show comment
Hide comment
@gonewandering

gonewandering Jan 20, 2016

@electrum, apologies if I'm missing something. Is there a merge plan for this?

@electrum, apologies if I'm missing something. Is there a merge plan for this?

@martint

This comment has been minimized.

Show comment
Hide comment
@martint

martint Jan 20, 2016

Contributor

@gonewandering yes, we're super backlogged processing pull requests, but this is definitely on our list.

Contributor

martint commented Jan 20, 2016

@gonewandering yes, we're super backlogged processing pull requests, but this is definitely on our list.

@gonewandering

This comment has been minimized.

Show comment
Hide comment
@gonewandering

gonewandering Jan 21, 2016

@martint, awesome. Thanks. Looking forward

@martint, awesome. Thanks. Looking forward

@milespoindexter

This comment has been minimized.

Show comment
Hide comment
@milespoindexter

milespoindexter Feb 9, 2016

Hi, I am using this as a plugin, with my current version of PrestoDB, and may have found a bug. When creating a schema for a collection, the connector fails with error whenever the db collection has a field name with a mixture or lower-case and upper-case characters. If I rename the field in the collection to be all lower-case, the queries then work fine. This has to be tested with a MongoDB server hosted on a case-sensitive O/S like Linux. The problem does not happen when I use a MongoDB on OSX, which has a case-insensitive file system.

Hi, I am using this as a plugin, with my current version of PrestoDB, and may have found a bug. When creating a schema for a collection, the connector fails with error whenever the db collection has a field name with a mixture or lower-case and upper-case characters. If I rename the field in the collection to be all lower-case, the queries then work fine. This has to be tested with a MongoDB server hosted on a case-sensitive O/S like Linux. The problem does not happen when I use a MongoDB on OSX, which has a case-insensitive file system.

@martint

This comment has been minimized.

Show comment
Hide comment
@martint

martint Apr 15, 2016

Contributor

Can you rebase on latest master?

Contributor

martint commented Apr 15, 2016

Can you rebase on latest master?

@martint

This comment has been minimized.

Show comment
Hide comment
@martint

martint Apr 22, 2016

Contributor

Merged, thanks!

Contributor

martint commented Apr 22, 2016

Merged, thanks!

@martint martint closed this Apr 22, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment