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

Adds SpanGapQueryBuilder. Feature #27862 #28636

Merged
merged 5 commits into from
Apr 13, 2018
Merged

Adds SpanGapQueryBuilder. Feature #27862 #28636

merged 5 commits into from
Apr 13, 2018

Conversation

Chandan83
Copy link
Contributor

@Chandan83 Chandan83 commented Feb 12, 2018 •
edited by javanna
Loading

SpanGapQueryBuilder is unlike any other QueryBuilder because it cannot be used to generate a SpanGapQuery which is private to SpanNearQuery. More details in the code comments. To the best of my intentions, I have adhered to all contributing guidelines except writing new unit tests. Before starting to write unit tests, I thought it will be useful to get some feedback on the general approach of the solution since it is little unusual compared to existing code.

Closes #27862

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

1 similar comment
@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@karmi
Copy link
Contributor

karmi commented Feb 12, 2018

Hi @Chandan83, we have found your signature in our records, but it seems like you have signed with a different e-mail than the one used in yout Git commit. Can you please add both of these e-mails into your Github profile (they can be hidden), so we can match your e-mails to your Github profile?

@Chandan83
Copy link
Contributor Author

@jimczi I had already signed CLA before submitting. Do you match anything other than github username?

@Chandan83
Copy link
Contributor Author

@karmi I have added the email which was used during commit to my github profile.

@Chandan83 Chandan83 closed this Feb 12, 2018
@Chandan83 Chandan83 reopened this Feb 12, 2018
@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

1 similar comment
@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@Chandan83 Chandan83 mentioned this pull request Feb 12, 2018
Closed
jimczi
jimczi reviewed Feb 14, 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 @Chandan83 !
The approach looks good to me. I'd remove the field parameter of the span_gap, it's needed for the other span_query but I think we can omit it for gaps.
I agree that it's not ideal that we need to wait to build the final Lucene query to fail a request that contains a span_gap in a wrong place but I can live with it.
@jpountz any thoughts about this leniency ?

@Chandan83
Copy link
Contributor Author

@jimczi If the approach is fine, I would start adding unit test cases.
Each clause of a span_near should have the same field (otherwise lucene throws error) and for the current change, I am using the field of the first clause as the field passed to SpanNearQuery.Builder(String field, boolen ordered). Since the first clause can be a span_gap, I felt having field parameter will be an uniform approach. Even the Lucene's SpanNearQuery uses field.
There is no harm in redundant sanity checks but I will go by what you suggest.

@jimczi
Copy link
Contributor

jimczi commented Feb 14, 2018

@jimczi If the approach is fine, I would start adding unit test cases.

Thanks

Since the first clause can be a span_gap, I felt having field parameter will be an uniform approach. Even the Lucene's SpanNearQuery uses field.

I think you're right, for consistency we should probably always require a field for a span_query so no need to remove it from the span_gap. We should probably make the change to SpanQueryBuilder to enforce it but it can be done in a follow up.

@javanna javanna added the :Search/Search Search-related issues that do not fall into other categories label Mar 16, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

@javanna
Copy link
Member

javanna commented Mar 16, 2018

@Chandan83 did you have a chance to get back to this?

@Chandan83
Copy link
Contributor Author

@javanna I could not find time to write the test cases. I might get to it in the first 2 weeks of April or most likely in May.

@Chandan83
Copy link
Contributor Author

@jimczi I have added the requires test cases.

@jimczi
Copy link
Contributor

jimczi commented Apr 9, 2018

jenkins, test this

@jimczi
Copy link
Contributor

jimczi commented Apr 10, 2018

jenkins, test this

@Chandan83
Copy link
Contributor Author

@jimczi The console log seems to suggest that the failure is unrelated to the commits. Please guide me here.

@jimczi
Copy link
Contributor

jimczi commented Apr 10, 2018

The pr is quite old so you should merge with master to pull the changes. Just do git merge master,
then push the changes here and I'll try the tests again.

@jimczi
Copy link
Contributor

jimczi commented Apr 11, 2018

jenkins, test this

jimczi
jimczi approved these changes Apr 11, 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.

Looks good to me. Thanks @Chandan83.
The tests are still failing but this pr is not the cause. Can you merge with master again and I'll merge if the tests pass.

@jimczi
Copy link
Contributor

jimczi commented Apr 13, 2018

jenkins retest this please

@jimczi jimczi merged commit 782517b into elastic:master Apr 13, 2018
@jimczi
Copy link
Contributor

jimczi commented Apr 13, 2018

Thanks @Chandan83 !

jimczi pushed a commit that referenced this pull request Apr 13, 2018
@Chandan83 @jimczi
Adds SpanGapQueryBuilder in the query DSL (#28636)
ba0eaab 
This change adds the support for a `span_gap` query inside the span query DSL.
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Apr 13, 2018
@jasontedor
Merge branch 'master' into java-home-sweet-java-home
062d1ae 
* master:
  Enable skipping fetching latest for BWC builds (elastic#29497)
  Add remote cluster client (elastic#29495)
  Ensure flush happens on shard idle
  Adds SpanGapQueryBuilder in the query DSL (elastic#28636)
  Control max size and count of warning headers (elastic#28427)
  Make index APIs work without types. (elastic#29479)
  Deprecate filtering on `_type`. (elastic#29468)
  Fix auto-generated ID example format (elastic#29461)
  Fix typo in max number of threads check docs (elastic#29469)
  Add primary term to translog header (elastic#29227)
  Add a helper method to get a random java.util.TimeZone (elastic#29487)
  Move TimeValue into elasticsearch-core project (elastic#29486)
martijnvg added a commit that referenced this pull request Apr 13, 2018
@martijnvg
Merge remote-tracking branch 'es/master' into ccr
9da3e73 
* es/master:
  Add remote cluster client (#29495)
  Ensure flush happens on shard idle
  Adds SpanGapQueryBuilder in the query DSL (#28636)
  Control max size and count of warning headers (#28427)
  Make index APIs work without types. (#29479)
  Deprecate filtering on `_type`. (#29468)
  Fix auto-generated ID example format (#29461)
  Fix typo in max number of threads check docs (#29469)
  Add primary term to translog header (#29227)
  Add a helper method to get a random java.util.TimeZone (#29487)
  Move TimeValue into elasticsearch-core project (#29486)
  Fix NPE in InternalGeoCentroidTests#testReduceRandom (#29481)
  Build: introduce keystoreFile for cluster config (#29491)
  test: Index more docs, so that it is less likely the search request does not time out.
martijnvg added a commit that referenced this pull request Apr 13, 2018
@martijnvg
Merge remote-tracking branch 'es/6.x' into ccr-6.x
8b3f218 
* es/6.x:
  Enable skipping fetching latest for BWC builds (#29497)
  Add remote cluster client (#29495)
  Ensure flush happens on shard idle
  Adds SpanGapQueryBuilder in the query DSL (#28636)
  Fix auto-generated ID example format (#29461)
  Fix typo in max number of threads check docs (#29469)
  Add primary term to translog header (#29227)
  Add a helper method to get a random java.util.TimeZone (#29487)
  Move TimeValue into elasticsearch-core project (#29486)
  Fix NPE in InternalGeoCentroidTests#testReduceRandom (#29481)
  Build: introduce keystoreFile for cluster config (#29491)
  test: Index more docs, so that it is less likely the search request does not time out.
@russcam
Copy link
Contributor

russcam commented Jul 9, 2018

This query is undocumented in the span queries documentation; it'll be exposed in the .NET client, so I'll base the implementation on how it's used in SpanGapQueryBuilderTests.java

@russcam russcam mentioned this pull request Jul 9, 2018
Closed
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
>enhancement :Search/Search Search-related issues that do not fall into other categories v6.3.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Chandan83 @elasticmachine @karmi @jimczi @javanna @russcam @colings86