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.

Sign up for GitHub

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

Jump to bottom

Allow terms query in _rollup_search #30973

Merged
merged 3 commits into from
Jun 5, 2018
Merged

Allow terms query in _rollup_search #30973

merged 3 commits into from
Jun 5, 2018

Conversation

jimczi
Copy link
Contributor

@jimczi jimczi commented May 30, 2018 •
edited
Loading

This change adds the terms query to the list of accepted queries
for the _rollup_search endpoint.

@jimczi
Allow match and terms query in _rollup_search
8e49949 
This change adds the `match` and `terms` query to the list of accepted queries
for the _rollup_search endpoint.
@jimczi jimczi added >enhancement v7.0.0 :StorageEngine/Rollup Turn fine-grained time-based data into coarser-grained data v6.4.0 labels May 30, 2018
@jimczi jimczi requested a review from polyfractal May 30, 2018 16:54
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

@epixa
Copy link
Contributor

epixa commented May 30, 2018

cc @jen-huang since this might impact your work on rollups in Kibana

colings86
colings86 reviewed May 31, 2018
View reviewed changes
Copy link
Contributor

@colings86 colings86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have the index side of rollup copy any normalisers from keywords fields into the rollup index so the match query will normalise the query text?

I realise this is makes this change bigger but it means that the match query will work the same across live and rollup indexes. With the change as it is now if I have a field that has a lowercasing normaliser on the live index and do a rollup search across live and rollup indexes for mySQL, it will find documents in the live index and return them but it will look like there are no matching documents in the rollup index because the same normalisation was not applied on the rollup index.

@colings86 colings86 mentioned this pull request May 31, 2018
Closed
colings86
colings86 approved these changes May 31, 2018
View reviewed changes
Copy link
Contributor

@colings86 colings86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jimczi and I discussed my concerns offline and we agreed that we would merge this PR as is into master and 6.4 and would open #30996 which is a blocker for 6.4 so the behaviour is consistent by the time 6.4 is released

@jimczi jimczi changed the title Allow match and terms query in _rollup_search Allow terms query in _rollup_search Jun 5, 2018
@jimczi
Copy link
Contributor Author

jimczi commented Jun 5, 2018

I updated the pr to handle terms queries only. match queries would not bring anything compared to term queries because rollup doesn't accept text field and normalizers are not copied in the rollup index. We can revisit the limitation if #30996 is implemented.

@polyfractal
Copy link
Contributor

LGTM, thanks @jimczi!

@jimczi jimczi merged commit 7f850bb into elastic:master Jun 5, 2018
@jimczi jimczi deleted the rollup_search_queries branch June 5, 2018 14:51
jimczi added a commit that referenced this pull request Jun 5, 2018
@jimczi
Allow terms query in _rollup_search (#30973)
3cefd40 
This change adds the `terms` query to the list of accepted queries
for the _rollup_search endpoint.
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jun 5, 2018
@jasontedor
Merge remote-tracking branch 'elastic/master' into feature-aware-check
3b9a1a9 
* elastic/master:
  Add cors support to NioHttpServerTransport (elastic#30827)
  [DOCS] Fixes security example (elastic#31082)
  Allow terms query in _rollup_search (elastic#30973)
  Removing erroneous repeat
  Adapt bwc versions after backporting elastic#30983 to 6.4
jasontedor added a commit that referenced this pull request Jun 5, 2018
@jasontedor
Merge remote-tracking branch 'elastic/master' into ccr
6e109e9 
* elastic/master:
  [DOCS] Creates rest-api folder in docs
  [Rollup] Disallow index patterns that match the rollup index (#30491)
  Add cors support to NioHttpServerTransport (#30827)
  [DOCS] Fixes security example (#31082)
  Allow terms query in _rollup_search (#30973)
jasontedor added a commit that referenced this pull request Jun 5, 2018
@jasontedor
Merge remote-tracking branch 'elastic/6.x' into ccr-6.x
5bf32af 
* elastic/6.x:
  [Rollup] Disallow index patterns that match the rollup index (#30491)
  Revert "Fixing MixedClusterClientYamlTestSuiteIT"
  Fixing MixedClusterClientYamlTestSuiteIT
  Add get mappings support to high-level rest client (#30889)
  [DOCS] Fixes security example (#31082)
  Allow terms query in _rollup_search (#30973)
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jun 5, 2018
@jasontedor
Merge branch 'ccr' into follower-primary-term
9627c60 
* ccr:
  [DOCS] Creates rest-api folder in docs
  [Rollup] Disallow index patterns that match the rollup index (elastic#30491)
  Replace exact numDocs by soft-del count in SegmentInfo (elastic#31086)
  Upgrade to Lucene-7.4.0-snapshot-0a7c3f462f (elastic#31073)
  Add cors support to NioHttpServerTransport (elastic#30827)
  [DOCS] Fixes security example (elastic#31082)
  Allow terms query in _rollup_search (elastic#30973)
  Removing erroneous repeat
  Adapt bwc versions after backporting elastic#30983 to 6.4
  [Tests] Muting RatedRequestsTests#testXContentParsingIsNotLenient
  TEST:  Retry synced-flush if ongoing ops on primary (elastic#30978)
  Fix docs build.
  Only auto-update license signature if all nodes ready (elastic#30859)
  Add BlobContainer.writeBlobAtomic() (elastic#30902)
  Add a doc value format to binary fields. (elastic#30860)
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jun 5, 2018
@jasontedor
Merge branch 'ccr' into remove-deprecated-perform-request
c29678f 
* ccr:
  [DOCS] Creates rest-api folder in docs
  [Rollup] Disallow index patterns that match the rollup index (elastic#30491)
  Replace exact numDocs by soft-del count in SegmentInfo (elastic#31086)
  Upgrade to Lucene-7.4.0-snapshot-0a7c3f462f (elastic#31073)
  Add cors support to NioHttpServerTransport (elastic#30827)
  [DOCS] Fixes security example (elastic#31082)
  Allow terms query in _rollup_search (elastic#30973)
  Removing erroneous repeat
  Adapt bwc versions after backporting elastic#30983 to 6.4
  [Tests] Muting RatedRequestsTests#testXContentParsingIsNotLenient
  TEST:  Retry synced-flush if ongoing ops on primary (elastic#30978)
  Fix docs build.
  Only auto-update license signature if all nodes ready (elastic#30859)
  Add BlobContainer.writeBlobAtomic() (elastic#30902)
  Add a doc value format to binary fields. (elastic#30860)
@jimczi jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@polyfractal polyfractal polyfractal approved these changes

@colings86 colings86 colings86 approved these changes

Assignees
No one assigned
Labels
>enhancement :StorageEngine/Rollup Turn fine-grained time-based data into coarser-grained data v6.4.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jimczi @elasticmachine @epixa @polyfractal @colings86