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

Added fields for MultiTermVectors (#42232) #42877

Closed
wants to merge 1 commit into from

Conversation

ojasgulati
Copy link
Contributor

@ojasgulati ojasgulati commented Jun 4, 2019

Fixes #42232

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features

@gwbrown
Copy link
Contributor

gwbrown commented Jun 6, 2019

@elasticmachine test this please

It looks like this bug happened because the single termvectors API can take the list of fields as a URL parameter, as an alternative to the fields list in the body, and the URL parameter method is how the HLRC implements the single termvectors API, but the multi termvectors API can only take the list of fields in the body. This change, as is, will result in the HLRC putting the fields list in both the URL parameters and the request body for single-termvector requests - I'm not sure if that will cause a problem or not, we may see some failing tests.

We may end up having to change the HLRC implementation of the single termvectors API to not use the URL parameters since they will now show up in the body. If that's the case, we'll have to adjust this test case as well.

@mayya-sharipova I'm not very familiar with the term vectors API, is there a reason we decided to have the HLRC put the fields list as a URL parameter rather than in the body?

@mayya-sharipova
Copy link
Contributor

@ojasgulati @gwbrown Sorry for a late reply. There is no any particular reason to have fields list as a URL parameter, and we can move them into the the body and fix the tests.

Copy link
Contributor

@mayya-sharipova mayya-sharipova left a comment

Choose a reason for hiding this comment

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

@ojasgulati Thanks for submitting a PR. We may need to do several more things here that @gwbrown pointed out:

  • We don't need URL parameter of params.withFields(tvrequest.getFields()); in here as we now will pass fields in the body
  • We will need to fix the test pointed out by @gwbrown to remove all mentions of fields parameter
  • Add a test to test that MultiTermVectors indeed work well fields. We can enhance CrudIT::testMultiTermvectors for this.

@ojasgulati
Copy link
Contributor Author

@mayya-sharipova Thank you for your reply. I will make changes as you described in your comment.

@gwbrown
Copy link
Contributor

gwbrown commented Sep 3, 2019

@ojasgulati Are you still planning to work on this?

@gwbrown
Copy link
Contributor

gwbrown commented Oct 9, 2019

Given that this hasn't seen any activity for quite a while, I'm going to close this PR. Feel free to reopen this one or open a new one if you want to continue working on this.

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

Successfully merging this pull request may close these issues.

MTermVector Java High Level Client Misses Fields Property
9 participants