search

moremore0812 / cqengine

Automatically exported from code.google.com/p/cqengine
0 stars 0 forks source link

Support to order by combinations of attributes, ascending/descending #16

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago 
QueryFactory.orderBy and orderByDescending methods signature return type are 
not the specific implementation OrderByOption/DeduplicationOption but 
QueryOption.

This leads to use of instanceof operator when validating the Map dynamic 
population when some of the implementations (DeduplicationOption) are not 
desired.

By the way... thank you for publishing this great project. You saved me a lot 
of time!

Original issue reported on code.google.com by rsgonzal...@gmail.com on 8 May 2013 at 7:43

GoogleCodeExporter commented 9 years ago 
You mean change the API from this:

    QueryOption<Car> deduplicate = deduplicate(DeduplicationStrategy.LOGICAL_ELIMINATION);
    QueryOption<Car> order = orderBy(Car.PRICE, Car.DOORS);
    ResultSet<Car> results = cars.retrieve(lessThan(Car.PRICE, 5000.0), queryOptions(deduplicate, order));

To this?:

    DeduplicationOption<Car> deduplicate = deduplicate(DeduplicationStrategy.LOGICAL_ELIMINATION);
    OrderByOption<Car> order = orderBy(Car.PRICE, Car.DOORS);
    ResultSet<Car> results = cars.retrieve(lessThan(Car.PRICE, 5000.0), queryOptions(deduplicate, order));

The query engine will need to use the instanceof operator internally anyway, 
because all QueryOptions (of various types) get bundled into a map by the 
queryOptions method.

Note I am thinking of overhauling Ordering support in a later release anyway. 
For example it currently lack support to order by one attribute ascending, and 
another descending (without defining a dummy attribute just for ordering). Let 
me know what you would like to see and I'll try to accommodate it. Thanks!

Original comment by ni...@npgall.com on 8 May 2013 at 1:24

GoogleCodeExporter commented 9 years ago 
Yes.

    DeduplicationOption<Car> deduplicate = deduplicate(DeduplicationStrategy.LOGICAL_ELIMINATION);
    OrderByOption<Car> order = orderBy(Car.PRICE, Car.DOORS);
    ResultSet<Car> results = cars.retrieve(lessThan(Car.PRICE, 5000.0), queryOptions(deduplicate, order));

it's what I had in mind.

First of all, I think this is not a defect, but a enhancement. It is a design 
decision which change does not affect backwards compatibility.

And why i was suggesting the change?

Let me explain. Unfortunately I was not thinking in the cqengine internals but 
a class of my domain (CachedIndexedCollection) which wraps 
cqengine.IndexedCollection and where it's needed the operator instanceof in 
order to filter (and allow) orderByOption object (and reject the other one). My 
apologizes for the mess.

This is the piece of code:
com.googlecode.cqengine.resultset.ResultSet queryResult = 
CachedIndexedCollection.retrieve(query, queryOptions); // needs internal check
com.googlecode.cqengine.resultset.ResultSet queryResult = 
CachedIndexedCollection.retrieve(query, orderOptions);

But my thoughts about the cqengine API design were: DeduplicationOption 
(single) are related to the ResultSet, whereas that OrderOptions (could be more 
than one) are related to Query. And the operator instanceof usage in 
queryEngine could offer a clue for that. 

Probably this humble opinion on the nature of queryOptions objects are better 
to discuss in the forum and Ordering support improvement you are evaluating is 
the best way for changing QueryOption interface. Let me time to read cqengine 
code and think about until weekend and then, if you agree, I will open a new 
topic there.

Original comment by rsgonzal...@gmail.com on 9 May 2013 at 8:01

GoogleCodeExporter commented 9 years ago 
Feel free to open a topic in the forum. I think I see what you would like to 
do. I will rename this issue to "Consider combining Query with QueryOptions for 
caching purposes" - i.e. you would like to associate result sets with a query + 
query options, to retrieve them later. Let's put the requirements on hold 
(shelved temporarily) until we have a plan from the forum.

Original comment by ni...@npgall.com on 9 May 2013 at 2:29

GoogleCodeExporter commented 9 years ago 
[deleted comment]
GoogleCodeExporter commented 9 years ago 
Re-opening this based on Roberto's patch from the forum, attached. It adds 
support to order results with combinations of ascending and descending 
attributes. Will try to integrate later in the week.

Original comment by ni...@npgall.com on 9 Jun 2013 at 11:50

Attachments:

GoogleCodeExporter commented 9 years ago 
[deleted comment]
GoogleCodeExporter commented 9 years ago

Original comment by ni...@npgall.com on 9 Jun 2013 at 11:52

GoogleCodeExporter commented 9 years ago 
This is done, released as CQEngine 1.2.0.

Roberto thanks for the patch, I have integrated it. I slimmed down the patch as 
I didn't want to maintain two APIs to order results going forward (the previous 
way, and this new way). So this way replaces the old approach. Actually it 
reduces the amount of code involved overall.

I integrated it with QueryFactory so there is a new syntax for ordering results:

ResultSet<Car> results = cars.retrieve(query, 
queryOptions(orderBy(descending(Car.PRICE), ascending(Car.DOORS))));

Thanks again for this contribution.

Original comment by ni...@npgall.com on 20 Aug 2013 at 10:30