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.

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

Support for Filter method String.contains(). #37

Open
hopecee opened this issue Jan 9, 2018 · 42 comments
Open

Support for Filter method String.contains(). #37

hopecee opened this issue Jan 9, 2018 · 42 comments

Comments

@hopecee
Copy link

hopecee commented Jan 9, 2018

Hi!
We already have the following Filter methods,
.eq() => equals to
.ne() => not equal to
.startsWith() => begins with
.matches() => exact value

Can we have method to search for string which contains a string value?
For instance, if a string like Appletogether, searching for toget is true.

I have not seen the method for that.

@andyjefferson
Copy link
Member

If you want a feature adding you need to fork the code and contribute the feature via a GitHub pull request.

@hopecee
Copy link
Author

hopecee commented Jan 10, 2018

I need to learn that now. You can put me through. To do this I need to fork both datanucleus-neo4j and datanucleus-core.

@andyjefferson
Copy link
Member

FYI Valid JDOQL methods are shown on this link http://www.datanucleus.org:15080/products/accessplatform_5_1/jdo/query.html#jdoql_methods
No idea why you would need to fork datanucleus-core, because that then affects ALL datastores. The only possible addition to datanucleus-core would be to add the METHOD "contains" for String values, since JAVA has a method String.contains(...).

@hopecee
Copy link
Author

hopecee commented Jan 11, 2018

If the String.contains() is added it will help.

This Cypher code can be used to check for the String that contains a variable value;

MATCH (p:Person)
WHERE p.name =~ '.*Apple.*'
RETURN p

OR

MATCH (p:Person)
WHERE p.name =~ '(?i).*apple.*' //case-insensitive
RETURN p

I have trying to look for how to add the method by adding code to these files;
9ed0bce
7faeb30
11a5a40

5d09a07

I am still looking and studying for where the actual method is added so that I can test it.
To add the method some files have to be modified , like;
https://github.com/datanucleus/datanucleus-api-jdo/blob/master/src/main/java/org/datanucleus/api/jdo/query/ExpressionImpl.java

@andyjefferson
Copy link
Member

The JDOQL is
stringField.contains(someValue)

which maps on to a generic compilation of InvokeExpression. InvokeExpression is what you should process in the neo4j plugin. i.e in QueryToCypherMapper.java

@hopecee
Copy link
Author

hopecee commented Jan 11, 2018

Sorry, Please how do I get that?
I have opened the file and trying to wrap my head on what to do here.
https://github.com/datanucleus/datanucleus-neo4j/blob/master/src/main/java/org/datanucleus/store/neo4j/query/QueryToCypherMapper.java

@andyjefferson
Copy link
Member

processInvokeExpression in that class I just quoted! just look at other plugins that do querying, and they all handle methods

@hopecee
Copy link
Author

hopecee commented Jan 11, 2018

I am having small issue on how to release on GitHub. The Zip file doesn't look like the normal jar file.
I need to know how to bring out the jar file so I can test the changes.

@andyjefferson
Copy link
Member

No idea what that means. To contribute on GitHub you provide a PULL REQUEST, via the Pull Requests option on the project.

@hopecee
Copy link
Author

hopecee commented Jan 11, 2018

I will make PULL REQUEST, but wont it be to test the commit before making PULL REQUEST?
I am learning.

@andyjefferson
Copy link
Member

It is for YOU to test the proposed changes, before submitting the PULL REQUEST. Hence it is in your local codebase, so you test it locally

@hopecee
Copy link
Author

hopecee commented Jan 11, 2018

Yes I test it locally. Shouldn't I extract the jar and run it on my IDE? Or is there any way to test it on GitHub?

@andyjefferson
Copy link
Member

GitHub is a repository. It doesn't test anything. You test it. There are some simply tests under the repository "tests", navigating to "jdo/neo4j", as per what the documentation says.

Also, to correct your earlier statement, there is no "startsWith" or "matches" supported by this plugin, as you can see from the processInvokeExpression method. No methods are currently supported.

@andyjefferson
Copy link
Member

To give an idea of how methods can be added (and since we didnt support any on this plugin before) I've added support for String toUpperCase/toLowerCase in issue #38 . Use the code added there as a guide to adding "contains" support for String fields

@hopecee
Copy link
Author

hopecee commented Jan 13, 2018

I followed another example before you come up with that.
This what I did but I have not been able to test it.
8d93f4a
d7a59ac
738ac85
cd9abf9

f01cb9f
32b506d

Is this another way of doing it? Am I taking the wrong way?

@andyjefferson
Copy link
Member

andyjefferson commented Jan 13, 2018

You seem to have tried to handle it directly as an operator (=~). JDOQL/JPQL do NOT have any OPERATOR called "CONTAINS" or indeed "=~". "contains" is a METHOD. You should not be going anywhere near datanucleus-core. JDOQL allows METHODS to be defined as per what Java allows, and as I've already said, the equivalent JDOQL that you are proposing here is
stringField.contains(someValue)

which is converted into a generic compilation of
InvokeExpression{[PrimaryExpression{this.stringField}].contains({arg})}

@hopecee
Copy link
Author

hopecee commented Jan 13, 2018

Ok. I got you. It is under InvokeExpression. I was thinking it should be under Neo4jBooleanExpression.

@hopecee
Copy link
Author

hopecee commented Jan 14, 2018

Following your footsteps, I have done this c3b6fb6. My problem is that I have not been able to see where those changes reflected.

@andyjefferson
Copy link
Member

That change makes no sense to me. You have a JDOQL query of

SELECT FROM mydomain.Person WHERE this.name.contains('so')

mapping on to Cypher clause of

contains(this.name)

I'm pretty sure there is no Cypher function for Strings called "contains" http://neo4j.com/docs/developer-manual/current/cypher/functions/#header-query-functions-string
so what is the Cypher supposed to be?! After all you said this was possible.

@hopecee
Copy link
Author

hopecee commented Jan 14, 2018

I thought we could get something like this, not that I was sure;

List<Product> prods = null;

JDOPersistenceManager jdopm = (JDOPersistenceManager) pm;
QProduct cand = QProduct.candidate();
JDOQLTypedQuery<Product> tq = jdopm.newJDOQLTypedQuery(Product.class);

prods = tq.filter(cand.contains(value)).executeList();

@hopecee
Copy link
Author

hopecee commented Jan 14, 2018

Since that method is not available, I used this code;

Query<Product> tq2 = jdopm.newQuery("SELECT  FROM " + Product.class.getName() + " WHERE :name.contains("+value+")");

I got this stack trace;

[filter:InvokeExpression{[ParameterExpression{name}].contains(VariableExpression{amp})}]
[symbols: amp type=unknown, this type=com.hopecee.tublex.neo4j.jdo.model.Product, name type=unknown]
2018-01-14 20:11:24.759 [qtp69673470-83] Query - JDOQL Query : Compile (datastore) of "SELECT   FROM com.hopecee.tublex.neo4j.jdo.model.Product WHERE :name.contains(amp)"
[INFO ] 2018-01-14 20:11:24.759 [qtp69673470-83] General - >> exception in filter
org.datanucleus.exceptions.NucleusException: Parameter ParameterExpression{name} is not currently set, so cannot complete the compilation

When I changed to;

Query<Product> tq2 = jdopm.newQuery("SELECT  FROM " + Product.class.getName() + " WHERE name.contains("+value+")");


[filter:InvokeExpression{[PrimaryExpression{name}].contains(VariableExpression{amp})}]
org.datanucleus.exceptions.NucleusException: Invoke expression is not supported by this mapper

@hopecee
Copy link
Author

hopecee commented Jan 14, 2018

It seems that this String method is not supported yet. They could only support .contains() in Collection Methods.

Is there any way to do this?

@andyjefferson
Copy link
Member

Only you know wtf 'value' is so nothing to say; you get a variableexpression so must be inputting something weird. You cannot use jdoqltyped queries with string.contains ... Obviously since there is no method in the jdo spec, hence why i post string-based jdoql. Jdoql method invocation is present in 'tests' project jdo/neo4j and all work for me.

@hopecee
Copy link
Author

hopecee commented Jan 14, 2018

Ok. I have list of names for the Product name, like "Apple", "Man", "AppleMan", "Orange"
I am trying to search for Product with name that contains variable "apple" .

This is why I am having this headache. How can I be able to accomplish that?

Let me have the link to the 'tests' project jdo/neo4j

@hopecee
Copy link
Author

hopecee commented Jan 14, 2018

I have seen it. https://github.com/datanucleus/tests/blob/master/jdo/neo4j/src/test/org/datanucleus/test/JDOQLTest.java.
Is this the best way to go? Is it not too cumbersome?

@andyjefferson
Copy link
Member

Cumbersome? It's a TEST. Any issue for adding a feature needs a test. Should be the first thing you do.

As for the overall "I am trying to search for Product with a name containing", JDOQL has "matches" allowing regexp. You could implement that. Or you could implement "contains".

@hopecee
Copy link
Author

hopecee commented Jan 15, 2018

JDOQL has "matches" allowing regexp. Thanks for This. It works perfectly well. I tried it before, I did not know it could be used for regexp. I used string value directly on it before but it did not work as I wanted.

These code works;

tq.filter(can.matches("(?i).*"+value+".*")).executeList(); //case-insensitive

Thanks again.

@hopecee
Copy link
Author

hopecee commented Jan 15, 2018

JDOQL has "matches" allowing regexp. Thanks for This. It works perfectly well. I tried it before, I did not know it could be used for regexp. I used string value directly on it before but it did not work as I wanted.

These code works;

String value = "appl";
tq.filter(cand.matches("(?i).*"+value+".*")).executeList(); // case-insensitive

Thanks again.

@andyjefferson
Copy link
Member

As already said, that is NOT implemented to execute IN THE DATASTORE; it executes in-memory! You have to implement it to work as a method in the datastore

@hopecee
Copy link
Author

hopecee commented Jan 15, 2018

Please give an example on how to use the methods you add to QueryToCypherMapper. Let me see what is going on.

@andyjefferson
Copy link
Member

I already have, IN THE TESTS. That does toUpperCase. These examples are the same as you can find in the DN docs, or JDO spec

@hopecee
Copy link
Author

hopecee commented Jan 17, 2018

Look at what I have come up with here,
7eca91a
b38fd46

Query q1 = jdopm.newQuery("SELECT FROM " + Product.class.getName() + "  WHERE this.name.contains('apple')");
prods = (List<Product>) q1.execute();

Is the execution still in in-memory?

@andyjefferson
Copy link
Member

An Operator is an INTERNAL JDOQL/JPQL Operator. There is NO "=~" operator in JDOQL/JPQL and so there should not be here either. You should simply generate the Cypher query you think that JDOQL maps on to. Can't get simpler

Whether it is in-memory or in-datastore is for YOU to see ... because the LOG tells you!

@hopecee
Copy link
Author

hopecee commented Jan 17, 2018

It gives me Invalid compilation when I use Neo4jStringExpression() or Neo4jLiteral().

 String propName = invokedFieldExpr.getFieldName();
 String value = literalValue.getLiteral().toString();
 String cypherText = propName + " =~ '(?i).*" + value + ".*'";
 Neo4jExpression neo4jExpr = new Neo4jStringExpression(cypherText);
 System.out.println("invokedFiecypherText.neo4jExpr : " + neo4jExpr.getCypherText());
 stack.push(neo4jExpr);
 return neo4jExpr;

That is my reason for using Neo4jBooleanExpression(). I am yet to find out why Neo4jStringExpression() gives Invalid compilation. Does it only meant for cypher function?

@andyjefferson
Copy link
Member

Why would you try to use Neo4jStringExpression????? It would be mindless. As JRE javadocs tell you, String.contains(CharSequence) returns a Boolean. Hence Neo4jBooleanExpression is what you use!!

If you can't work it out from that then frankly it is your problem. I have other things to do.

@hopecee
Copy link
Author

hopecee commented Jan 17, 2018

Sorry, I am here to learn. This my commit works. Is anything wrong with it?
7eca91a
b38fd46

Query q1 = jdopm.newQuery("SELECT FROM " + Product.class.getName() + "  WHERE this.name.contains('apple')");
prods = (List<Product>) q1.execute();

I have tested it. It works like the normal

MATCH (p:Person)
WHERE p.name =~ '(?i).*apple.*' //case-insensitive
RETURN p

As documented here, http://neo4j.com/docs/developer-manual/current/cypher/clauses/where/#_regular_expressions

@andyjefferson
Copy link
Member

Anything wrong with it ? I have already told you on multiple occasions that you should not be touching OPERATIONs, since those are internal JDOQL/JPQL operations and there is no such "=~" in JDOQL/JPQL. So remove that!! You simply need to form the Cypher query equivalent for "String.contains(arg)" and return it in a Neo4jBooleanExpression ...

@hopecee
Copy link
Author

hopecee commented Jan 18, 2018

I did not understand you before. My eyes did not go towards that constructor;

public Neo4jBooleanExpression(String cypher) {
    cypherText = cypher;
}

I have done these;
11d6b58
ffe9e6d

It is still working as expected, but there is one issue with Literal.
Literal with less than two letters does not work here.

@andyjefferson
Copy link
Member

You are ignoring "neo4jExprArgs" which is the processed form of the "args" from earlier in that method; use those and not "args".
You should check that the literal value is of type String (or Character).
You should check if the argument is a parameter, and use the parameter value.
Can Neo4j Cypher allow the argument to be a property? i.e WHERE p.name =~ '(?i).*' + p.otherName + '.*' ? in which case you need to support the arg being a PrimaryExpression.
You need to provide a test or tests that demonstrate this (to the "tests" project jdo/neo4j).

You are ignoring DataNucleus code conventions http://www.datanucleus.org/documentation/development.html#coding_standards

@hopecee
Copy link
Author

hopecee commented Jan 19, 2018

I do not think it can allow the argument to be a Parameter unless the parameter name is converted to String.
You know, =~ is use to match regular expressions which is in String form.

=~ 'regexp'

The documentation is here, http://neo4j.com/docs/developer-manual/current/cypher/clauses/where/#_regular_expressions

@hopecee
Copy link
Author

hopecee commented Jan 19, 2018

Added Support for String.matches(), To except REGEXP
e1b13f8

Using appropriate Operator CONTAINS
9451f2b
Added Support for String.startsWith() and .endsWith()
b417653

@andyjefferson andyjefferson changed the title Support for Filter method .contains(). Support for Filter method String.contains(). May 6, 2018
@andyjefferson
Copy link
Member

I don't see anything here that isn't in the current codebase

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

No branches or pull requests

2 participants