add support to lucene module for converting a query to a Lucene QueryWrapperFilter #491

Closed
mirkosertic opened this Issue Sep 7, 2013 · 6 comments

Comments

Projects
None yet
2 participants
@mirkosertic

It would be great to have an ability to convert a TypedQuery to a Lucene QueryWrapperFilter. This would enable us to convert queries with a "constant" and a "dynamic" part to a Filter for the "constant" conditions and reuse it between different filters. This would greatly improve query performance in Lucene4. The AbstractLuceneQuery. createQuery() Method is private, so i am not able to implement this functionality by subclassing or delegation.

@mirkosertic

This comment has been minimized.

Show comment
Hide comment
@mirkosertic

mirkosertic Sep 7, 2013

Another option could be to add something like a "const" method to a filter predicate. Then we can use the dsl to express the static and dynamic parts and QueryDSL could internally create a query with the corresponding filter.

Another option could be to add something like a "const" method to a filter predicate. Then we can use the dsl to express the static and dynamic parts and QueryDSL could internally create a query with the corresponding filter.

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Sep 8, 2013

Member

AbstractLuceneQuery.createQuery() is private. Maybe filter access could be provided on the AbstractLuceneQuery level instead?

Something like this maybe

public Filter asFilter() {
    return new QueryWrapperFilter(createQuery());
}

or alternatively just make the access of createQuery public?

Member

timowest commented Sep 8, 2013

AbstractLuceneQuery.createQuery() is private. Maybe filter access could be provided on the AbstractLuceneQuery level instead?

Something like this maybe

public Filter asFilter() {
    return new QueryWrapperFilter(createQuery());
}

or alternatively just make the access of createQuery public?

@mirkosertic

This comment has been minimized.

Show comment
Hide comment
@mirkosertic

mirkosertic Sep 8, 2013

The asFilter implementation looks pretty good. From a Lucene point of view we should only use filters if we want to reuse them, so probably it would make sense to always return a CachingWrapperFilter instance. For further enhancements it would be cool to have the createQuery() method with protected visibility, so subclasses can access it.

By the way: what would be the best way to create unit tests for TypedQueries? Should i rely on toString() or should i assert the created Lucene query?

The asFilter implementation looks pretty good. From a Lucene point of view we should only use filters if we want to reuse them, so probably it would make sense to always return a CachingWrapperFilter instance. For further enhancements it would be cool to have the createQuery() method with protected visibility, so subclasses can access it.

By the way: what would be the best way to create unit tests for TypedQueries? Should i rely on toString() or should i assert the created Lucene query?

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Sep 8, 2013

Member

By the way: what would be the best way to create unit tests for TypedQueries? Should i rely on toString() or should i assert the created Lucene query?

Probably assert the created Lucene query.

Member

timowest commented Sep 8, 2013

By the way: what would be the best way to create unit tests for TypedQueries? Should i rely on toString() or should i assert the created Lucene query?

Probably assert the created Lucene query.

@mirkosertic

This comment has been minimized.

Show comment
Hide comment
@mirkosertic

mirkosertic Sep 8, 2013

So protected acces for createQuery() makes perfect sense, or even public access...

So protected acces for createQuery() makes perfect sense, or even public access...

timowest added a commit that referenced this issue Sep 8, 2013

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Oct 20, 2013

Member

Released in 3.2.4

Member

timowest commented Oct 20, 2013

Released in 3.2.4

@timowest timowest closed this Oct 20, 2013

@timowest timowest added this to the 3.2.4 milestone Apr 13, 2014

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