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

Do NOT allow termvectors on nested fields #32728

Merged
merged 2 commits into from
Aug 23, 2018
Merged

Do NOT allow termvectors on nested fields #32728

merged 2 commits into from
Aug 23, 2018

Conversation

mayya-sharipova
Copy link
Contributor

Requesting _termvectors on a nested field or any sub-fields of a nested field
returns empty results.

Closes #21625, #32652

@mayya-sharipova
Do not allow termvectors on nested fields
1387c18 
Requesting _termvectors on a nested field or any sub-fields of a nested field
returns empty results.

Closes elastic#21625
jimczi
jimczi requested changes Aug 9, 2018
View reviewed changes
Copy link
Contributor

@jimczi jimczi left a comment

Choose a reason for hiding this comment

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

Thanks @mayya-sharipova , I left one comment

server/src/main/java/org/elasticsearch/index/termvectors/TermVectorsService.java
@@ -169,6 +170,15 @@ private static boolean isValidField(MappedFieldType fieldType) {
if (fieldType.indexOptions() == IndexOptions.NONE) {
return false;
}
// and must not be under nested field
Copy link
Contributor

Choose a reason for hiding this comment

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

We should check all levels in the path, for instance in foo.bar.baz, foo can be a simple object field and foo.bar a nested field.

@colings86 colings86 added >bug :Search/Search Search-related issues that do not fall into other categories labels Aug 10, 2018
@mayya-sharipova
Copy link
Contributor Author

@jimczi I have addressed you feedback. Can you please continue the review when you have time.

jimczi
jimczi approved these changes Aug 23, 2018
View reviewed changes
Copy link
Contributor

@jimczi jimczi left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @mayya-sharipova

@mayya-sharipova mayya-sharipova merged commit fdff8f3 into elastic:master Aug 23, 2018
martijnvg added a commit that referenced this pull request Aug 24, 2018
@martijnvg
Merge remote-tracking branch 'es/master' into ccr
82592dd 
* es/master: (62 commits)
  [DOCS] Add docs for Application Privileges (#32635)
  Add versions 5.6.12 and 6.4.1
  Do NOT allow termvectors on nested fields (#32728)
  [Rollup] Return empty response when aggs are missing (#32796)
  [TEST] Add some ACL yaml tests for Rollup (#33035)
  Move non duplicated actions back into xpack core (#32952)
  Test fix - GraphExploreResponseTests should not randomise array elements Closes #33086
  Use `addIfAbsent` instead of checking if an element is contained
  TESTS: Fix Random Fail in MockTcpTransportTests (#33061)
  HLRC: Fix Compile Error From Missing Throws (#33083)
  [DOCS] Remove reload password from docs cf. #32889
  HLRC: Add ML Get Buckets API (#33056)
  Watcher: Improve error messages for CronEvalTool (#32800)
  Search: Support of wildcard on docvalue_fields (#32980)
  Change query field expansion (#33020)
  INGEST: Cleanup Redundant Put Method (#33034)
  SQL: skip uppercasing/lowercasing function tests for AZ locales as well (#32910)
  Fix the default pom file name (#33063)
  Switch ml basic tests to new style Requests (#32483)
  Switch some watcher tests to new style Requests (#33044)
  ...
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Aug 24, 2018
@jasontedor
Merge branch 'ccr' into ccr-fetch-exceptions
3efdf20 
* ccr: (71 commits)
  Make CCR QA tests build again (elastic#33113)
  Add hook to skip asserting x-content equivalence (elastic#33114)
  Muted testListenersThrowingExceptionsDoNotCauseOtherListenersToBeSkipped
  [Rollup] Move getMetadata() methods out of rollup config objects (elastic#32579)
  fixed not returning response instance
  Muted testEmptyAuthorizedIndicesSearchForAllDisallowNoIndices
  Update Google Cloud Storage Library for Java (elastic#32940)
  Remove unsupported Version.V_5_* (elastic#32937)
  Required changes after merging in master branch.
  [DOCS] Add docs for Application Privileges (elastic#32635)
  Add versions 5.6.12 and 6.4.1
  Do NOT allow termvectors on nested fields (elastic#32728)
  [Rollup] Return empty response when aggs are missing (elastic#32796)
  [TEST] Add some ACL yaml tests for Rollup (elastic#33035)
  Move non duplicated actions back into xpack core (elastic#32952)
  Test fix - GraphExploreResponseTests should not randomise array elements Closes elastic#33086
  Use `addIfAbsent` instead of checking if an element is contained
  TESTS: Fix Random Fail in MockTcpTransportTests (elastic#33061)
  HLRC: Fix Compile Error From Missing Throws (elastic#33083)
  [DOCS] Remove reload password from docs cf. elastic#32889
  ...
mayya-sharipova added a commit that referenced this pull request Aug 24, 2018
@mayya-sharipova
Revert "Do NOT allow termvectors on nested fields (#32728)"
6f1ee76 
This reverts commit fdff8f3.
jimczi pushed a commit that referenced this pull request Aug 24, 2018
@mayya-sharipova @jimczi
Revert "Do NOT allow termvectors on nested fields (#32728)"
9824bf9 
This reverts commit fdff8f3.
dnhatn added a commit that referenced this pull request Aug 26, 2018
@dnhatn
Merge branch 'master' into ccr
75304f4 
* master:
  Add proxy support to RemoteClusterConnection (#33062)
  TEST: Skip assertSeqNos for closed shards (#33130)
  TEST: resync operation on replica should acquire shard permit (#33103)
  Switch remaining x-pack tests to new style Requests (#33108)
  Switch remaining tests to new style Requests (#33109)
  Switch remaining ml tests to new style Requests (#33107)
  Build: Line up IDE detection logic
  Security index expands to a single replica (#33131)
  HLRC: request/response homogeneity and JavaDoc improvements (#33133)
  Checkstyle!
  [Test] Fix sporadic failure in MembershipActionTests
  Revert "Do NOT allow termvectors on nested fields (#32728)"
  [Rollup] Move toAggCap() methods out of rollup config objects (#32583)
  Fix race condition in scheduler engine test
@jimczi jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019
@BurntSushi
Copy link

BurntSushi commented Dec 16, 2020 •
edited
Loading

@mayya-sharipova Do you happen to know why this change was reverted? As far as I can tell, termvectors still doesn't work with nested documents.

@mayya-sharipova
Copy link
Contributor Author

mayya-sharipova commented Dec 16, 2020 •
edited
Loading

@BurntSushi We were a little hasty in disabling _termvectors request on nested docs, because this request works when term vectors are not explicitly stored, and disabling this request all together is a breaking change. That's why we had to revert the PR.
You are right, the request _termvectors still doesn't work if term vectors were explicitly stored and pre-calculated during indexing.
As a short fix, we were thinking to disable term_vector parameter in the mapping of sub-fields of a nested field, but overall we need to think how term vectors should work on nested docs.

@BurntSushi
Copy link

@mayya-sharipova Great, thanks for the clarification!

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

@jimczi jimczi jimczi approved these changes

Assignees
No one assigned
Labels
>bug :Search/Search Search-related issues that do not fall into other categories v6.5.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Term vectors query returns nothing in nested inner objects if term vectors are stored in the index
4 participants
@mayya-sharipova @BurntSushi @jimczi @colings86