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

Avoid duplicate index selection in Mango #807

Merged
merged 2 commits into from Sep 12, 2017

Conversation

Projects
None yet
2 participants
@willholley
Member

willholley commented Sep 12, 2017

Overview

Previously, index selection for a given query was run twice for each request - once to add
a warning in case a full database scan would be performed and then again when the query was executed.

This moves the warning generation so that it occurs at the end of the query processing and we can use the existing index context to decide whether to add a warning or not.

Whilst only a minor optimisation (which also assumes we don't have cached query plans etc), it
at least moves index selection to where one might expect it to happen i.e. during query planning.

Testing recommendations

Added a unit test to cover that a warning is appropriately generated. The text index tests should also be run manually.

GitHub issue number

Related Pull Requests

Checklist

  • Code is written and works correctly;
  • Changes are covered by tests;
  • Documentation reflects the changes;
@garrensmith

This comment has been minimized.

Show comment
Hide comment
@garrensmith

garrensmith Sep 12, 2017

Member

I like the idea of this. I don't really like the fact that mango_cursor is calling the http module to send the warning. I dont think the mango_cursor module should know about the http layer. It might be better to do this via the handle_doc({row}) callback.

Member

garrensmith commented Sep 12, 2017

I like the idea of this. I don't really like the fact that mango_cursor is calling the http module to send the warning. I dont think the mango_cursor module should know about the http layer. It might be better to do this via the handle_doc({row}) callback.

@willholley

This comment has been minimized.

Show comment
Hide comment
@willholley

willholley Sep 12, 2017

Member

@garrensmith I kinda agree. However, it's no different to the pattern used by bookmarks or execution stats (i.e. JSON serialisation happens in the maybe_ function rather than in httpd). Would you be more comfortable if the maybe_add_warning lived in a different module e.g. mango_idx?

Member

willholley commented Sep 12, 2017

@garrensmith I kinda agree. However, it's no different to the pattern used by bookmarks or execution stats (i.e. JSON serialisation happens in the maybe_ function rather than in httpd). Would you be more comfortable if the maybe_add_warning lived in a different module e.g. mango_idx?

willholley added some commits Sep 12, 2017

Avoid duplicate index selection in Mango
Previously, index selection for a given query
was run twice for each request - once to add
a warning in case a full database scan would be
performed and then again when the query was executed.

This moves the warning generation so that it occurs
at the end of the query processing and we can use
the existing index context to decide whether to
add a warning or not.

Whilst only a minor optimisation (which also assumes
we don't have cached query plans etc), it
at least moves index selection to where you'd expect
it to happen (query planning).
Use unittest assert in index selection tests
Replace use of native assert with unittest.assertX.
This ensures we return descriptive errors when assertions
fail.
@garrensmith

Awesome

@willholley willholley merged commit bc43efb into apache:master Sep 12, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@willholley willholley deleted the willholley:mango_avoid_duplicate_index_selection branch Sep 12, 2017

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