Issue Details (XML | Word | Printable)

Key: NUCMONGODB-65
Type: Improvement Improvement
Status: Closed Closed
Resolution: Fixed
Priority: Major Major
Assignee: Unassigned
Reporter: Chris Rued
Votes: 0
Watchers: 0
Operations

If you were logged in you would be able to see more operations.
DataNucleus Store MongoDB

Support parameters in MongoDB queries with basic types and simple relationship types (patch)

Created: 20/Mar/12 07:38 AM   Updated: 06/Apr/12 05:52 PM   Resolved: 20/Mar/12 07:23 PM
Component/s: None
Affects Version/s: None
Fix Version/s: 3.1.0.m2

File Attachments: 1. File mongodb.patch.20120320 (17 kB)


Forum Thread URL: http://www.datanucleus.org/servlet/forum/viewthread_thread,7083
Datastore: MongoDB


 Description  « Hide
For prior discussion see forum...

I've only actually implemented a narrow set of cases, but they seem to suit my current needs.

Much of this is "code by analogy" from RDBMS support, so I may not understand the implications of some bits of it...other bits were left in because they *looked* right, and seemed useful here if they were useful elsewhere...but I haven't tested them thoroughly.

Is there a comprehensive test suite I can run against or should be augmenting?

Let me know if you need more.


Sort Order: Ascending order - Click to sort in descending order
Chris Rued added a comment - 20/Mar/12 07:40 AM
Patch to support some basic query parameters.

Created with the command:

    svn diff > ../../../mongodb.patch

from the directory:

    datanucleus\platform\store.mongodb\trunk\src


Chris Rued added a comment - 20/Mar/12 08:12 AM
(updated to remove an e.printStackTrace() accidentally left behind)

Andy Jefferson added a comment - 20/Mar/12 08:55 AM
Thx.

Patch refers to MongoParameterLiteral yet doesn't add it; seems from your patch like you renamed MongoLiteral then changed it. Sadly this is in the patch like that ... so you intend MongoLiteral not to exist?

You introduce use of "JavaTypeMapping" for MongoDB, this is for RDBMS only. I don't understand why you need anything more than MongoLiteral since in the datastore there is only a literal (i.e it doesn't support parameters).

End result is the patch won't apply.

Andy Jefferson added a comment - 20/Mar/12 09:22 AM
I've committed some minimal changes to SVN trunk that shows (I hope) what I meant for the simple parameter support (and maybe you had this at some point but the RDBMS plugin confused you). Have a look and hopefully we can build on that for other types of parameters.

The RDBMS plugin is different in many ways (not just the use of org.datanucleus.store.mapped package), but also that its query generator supports the full range of JDOQL/JPQL syntax ... whereas MongoDB will never do that, so it will only be catering for specifics really. I'd use it only to get ideas from, but ignore any of the JavaTypeMapping stuff.



About tests :
Look at http://datanucleus.svn.sourceforge.net/viewvc/datanucleus/test/accessplatform/trunk/

The idea here is that *one day* all test suites (test.jdo.general, test.jdo.application, test.jdo.datastore, test.jdo.orm.application etc) will run on all datastores. The reality is somewhat different due to resource, so I typically just run "test.jdo.mongodb" (for mongodb) which has some simple tests. Like this

mvn clean test -Pmongodb

I added a simple parameter test to it (in SVN trunk) and it passes.

Chris Rued added a comment - 20/Mar/12 04:29 PM - edited
Thanks for your detailed feedback and careful review:

I'm not sure what happened with the Literal/ParameterLiteral: in my source I have both files, and MongoParameterLiteral extends MongoLiteral. In any case, it appears to be unnecessary. I did become a bit confused in dealing with relations which is when some of the JavaTypeMapping stuff came over, I think...


What you have partly matches my intention.

You handle the basic parameters much more simply than I had (thanks for the cleanup :-) ).

What's missing is:
1) Support for simple relations (in cases where a join would not be necessary). The important changes to support this are in getFieldNameForPrimary() and in processEqExpression().

2) Support for a some additional literals: I modified processLiteral() to support Boolean literals and NULL literals.

Chris Rued added a comment - 20/Mar/12 06:40 PM
A more focused set of changes for implementing the following:

1. Simple relation queries, like:

       SELECT a FROM Entity1 a WHERE a.relatedEntity = :related

   Where Entity1 is is related (via relatedEntity) to some other entity and is the owning-side.

   The main changes to support this are in QueryToMongoDBMapper.getFieldNameForPrimary (I've changed the signature to take the expression directly, since the tuples weren't used anywhere else). It now returns the field name in cases where the relationType is supported and the candidate class is the owning side.

   This also required a change to QueryToMongoDBMapper.processParameterExpression() so that it is aware of persistable objects and creates appropriate MongoLiterals with the id (I think it's OK to use String.valueOf(...getIdForObject(paramValue)) for these, right?).

   (I had originally just stored the related entity directly in a MongoLiteral and did the ID extraction in processEqExpression, but I think it's better to pull the ID out sooner...)

2. Added support for Boolean and NULL literals (added to QueryToMongoDBMapper.processLiteral()).

3. Added support for Boolean and NULL as parameter values(added to QueryToMongoDBMapper.processParameterExpression()).

4. Added support for $and in MongoDB queries (in MongoBooleanExpression). I think it may have been unimplemented because it wasn't available for an older version of mongo???

5. Remaining changes are cosmetic (@Override annotation, and some ordering of imports done by my IDE that seemed good to keep).


(note: the confusing MongoParameterLiteral is now removed from my code ... things are getting a little clearer :-) )

Andy Jefferson added a comment - 20/Mar/12 07:23 PM
Thx. Your patch is now in SVN trunk, and I added a simple detection on whether the parameter is actually set at the point of compile (with JDO you can compile before parameters are passed in at execution), and also to cater for positional parameters (copied from the HBase plugin).

Chris Rued added a comment - 21/Mar/12 11:48 PM
Thanks for all your help on this.

I noticed a problem with JPA queries with multiple parameters: The first parameter triggers the compilation of the expression while the second parameter is unset. This leaves precompilable set to "true". When the second setParameter is called, the old compilation is still used and the parameter is only stored in a map. Because we're not substituting the parameters into the pre-compiled expression, this leaves no choice but to execute in memory.

I've come up with a quick fix that causes precompilable to be set to false when an unset parameter is encountered:


Index: QueryToMongoDBMapper.java
===================================================================
--- QueryToMongoDBMapper.java (revision 14441)
+++ QueryToMongoDBMapper.java (working copy)
@@ -573,6 +579,8 @@
                 return lit;
             }
             // TODO Support other parameter value types
+ } else {
+ precompilable = false;
         }


I'm not sure this patch will apply cleanly, since I'm only pasting in the portion relevant to this comment; I have some other changes in progress. Basically, it's adding an else clause to the if (paramValueSet) statement which sets precompilable to false if an unset parameter is encountered.


Andy Jefferson added a comment - 22/Mar/12 10:46 AM
I made a change to processParameter() and changed the log messages. Perhaps that caters for your case

Chris Rued added a comment - 22/Mar/12 01:27 PM - edited
I just had a look at your latest changes. It looks good to me, I'll update and confirm.

Thanks again!