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

Enable compound indices and queries to work regardless of ordering #206

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ejbarrios
Copy link

Enable compound queries to use compound indices even if the attributes are specified in different order

@npgall
Copy link
Owner

npgall commented Aug 21, 2018

Thanks Eduardo!

I will try to find time to take a look at this in more detail soon, but things are quite busy at the moment in my day job!

I think your analysis is spot on, and I agree there are basically two issues at play here:

  1. There is no query normalizer. This would flatten or restructure complex queries while preserving logical equivalence.
  2. The CompoundIndex is not lenient enough at supporting and() queries whose:
    2a. equals() subqueries are not in its expected order, or
    2b. contain additional subqueries as well.

Could we split the PR into two parts along the lines above?

For 1 –
I’d prefer if the query normalizer was a first class citizen of the library, rather than it being buried in CompoundIndex.

I’d envisage a kind of helper utility like:

Query<O> normalized = QueryNormalizer.normalize(Query<O> input, QueryOptions queryOptions, IndexedCollection<O> collection)

(supplying the collection might not be strictly necessary, but it might be useful at some point in future)

For 2 –
We need to be careful to avoid allocating additional collections and data structures where possible in the request code path.

For some background – I remember when I introduced the transaction support a few years ago, I started allocating additional transient collections internally for each request, and people complained that latency had regressed. And it was true that earlier versions of CQEngine were faster at the time. Some people are using CQEngine in very latency and GC-sensitive applications (financial trading etc.).

I note you’ve tried to retain the current architecture of the engine itself by confining changes to the CompoundIndex, but this has necessitated allocating a Map and copying attributes into it etc.
Rather than that, I think we could change the engine to iterate the CompoundIndexes and ask each one if it supports each branch. This might simplify the task of the index checking it supports each and() query.

What do you think? Would you be happy to split this into two separate CRs that we could tackle independently?

@npgall npgall force-pushed the master branch 2 times, most recently from a7fb59c to 1ed900c Compare September 15, 2018 00:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants