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

ISPN-3917 Implement projections for unindexed filters #2567

Closed
wants to merge 1 commit into from

Conversation

anistor
Copy link
Member

@anistor anistor commented May 19, 2014

Also refactor the Matcher interface and the interface for single filters (no callback)

Jira: https://issues.jboss.org/browse/ISPN-3917

@Sanne
Copy link
Member

Sanne commented May 19, 2014

I've started to look but couldn't finish.

@gustavonalle could you review this too? Good to familiarize with this functionality.

@anistor
Copy link
Member Author

anistor commented May 20, 2014

rebased.

@anistor
Copy link
Member Author

anistor commented May 20, 2014

@wburns Please have a look too, since you'll be the first user of the Matcher/ObjectFilter api :)

@anistor
Copy link
Member Author

anistor commented May 20, 2014

Updated tests.

@wburns
Copy link
Member

wburns commented May 20, 2014

First pass seems fine to me. Seems like the API is a bit nicer now thanks 👍

@gustavocoding
Copy link

Looks nice!

I found issues with a couple of expressions:

from Person p where p.age > 10 and p.age < 30  => matches even if p is 40 years old
from Person p where p.license is null => throws NPE (same for p.license = null)

@anistor
Copy link
Member Author

anistor commented May 21, 2014

Thanks @wburns and @gustavonalle! I noticed the comment only now. Will try to fix asap.

@anistor
Copy link
Member Author

anistor commented May 22, 2014

Thanks for spotting those issues. I fixed them.

@gustavonalle I think you added comments directly on the commit rather that in the 'conversation' or 'changed files' tab. github has the nasty habit of occasionally removing such comments when the commit in question is updated, which just happened. So I'll answer here:

  1. re FilterResult, unless @wburns needs it I would avoid adding an extra object to represent the result for now. The whole object-filter module is internal, so we can easily change the API whenever we feel the need.

  2. re FilterQuery and Matcher, yes, we plan to have a single Query interface for both indexed and non-indexed search. But the factory is different, and the returned instances are not useable interchangeably. FilterQuery is an implementation detail that the user should not see.

@gustavocoding
Copy link

Makes sense!

Re the issues, sorry for not being specific, but the second use case

from Person p where p.license is null

is for an existing property, and I'm afraid it is still throwing NPE

@anistor
Copy link
Member Author

anistor commented May 23, 2014

@gustavonalle I've just found a problem with != operator and fixed it and added test for it. Also added tests for 'is null' for both existing and missing property but could not find the NPE you mention. I'm surely not doing it right. Could you please try again on the new version and post a stacktrace or the test code that fails? Thanks!

@gustavocoding
Copy link

The NPE happens when an existing property of the instance obj has null value:

java.lang.NullPointerException
    at org.infinispan.objectfilter.impl.predicateindex.ReflectionMatcherEvalContext.processAttributes(ReflectionMatcherEvalContext.java:20)
    at org.infinispan.objectfilter.impl.predicateindex.ReflectionMatcherEvalContext.processAttribute(ReflectionMatcherEvalContext.java:40)
    at org.infinispan.objectfilter.impl.predicateindex.ReflectionMatcherEvalContext.processAttributes(ReflectionMatcherEvalContext.java:33)

@anistor
Copy link
Member Author

anistor commented May 23, 2014

Got it now! That was easily fixed for the reflection based cased. But running the same test for a protobuf encoded entity leads to another issue, https://issues.jboss.org/browse/ISPN-4319. I'll solve that separately.

Also refactor the Matcher interface and the interface for single filters (no callback)
@gustavocoding
Copy link

Integrated!

@anistor anistor deleted the t_3917_revisited branch May 23, 2014 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants