Skip to content

Conversation

romseygeek
Copy link
Contributor

I'll open a separate PR against 6.x to issue a deprecation warning instead of throwing an exception.

@romseygeek romseygeek added >enhancement >breaking :Search Foundations/Mapping Index mappings, including merging and defining field types v6.4.0 labels Jun 4, 2018
@romseygeek romseygeek self-assigned this Jun 4, 2018
@romseygeek romseygeek requested a review from jimczi June 4, 2018 09:53
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

now include entire geohash cell, instead of just geohash center.

* Attempts to generate multi-term phrase queries against non-indexed fields
will now throw an exception
Copy link
Contributor

Choose a reason for hiding this comment

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

Only if you force an analyzer for all fields in the query ? This limits the scope of the breaking change so I think it's important to mention it.

Copy link
Contributor

Choose a reason for hiding this comment

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

also maybe "non-indexed" should be "non-text" since eg. dates are conceptually indexed, just differently. Or alternatively "against fields that do not index positions"

Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

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

I left another comment about the docs.

now include entire geohash cell, instead of just geohash center.

* Attempts to generate multi-term phrase queries against non-indexed fields
will now throw an exception
Copy link
Contributor

Choose a reason for hiding this comment

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

also maybe "non-indexed" should be "non-text" since eg. dates are conceptually indexed, just differently. Or alternatively "against fields that do not index positions"

@romseygeek
Copy link
Contributor Author

We need to handle MultiMatch queries here too, which means ignoring exceptions if lenient=true

@jimczi
Copy link
Contributor

jimczi commented Jun 4, 2018

We need to handle MultiMatch queries here too, which means ignoring exceptions if lenient=true

Good catch. MatchQuery handles all full text query parsers so we just need to catch exceptions inside MatchQuery#MatchQueryBuilder#analyze(Multi)Phrase. Sorry I missed this in #30450.

@romseygeek
Copy link
Contributor Author

Failure above is because I changed MatchQuery#checkForPositions to throw IllegalArgumentException rather than IllegalStateException. I still think this makes sense, but I'll do it in a follow up so we can argue about it there separately.

@romseygeek romseygeek merged commit 852df12 into elastic:master Jun 4, 2018
@romseygeek romseygeek deleted the unindexed-phrases branch June 4, 2018 18:12
dnhatn added a commit that referenced this pull request Jun 4, 2018
* master:
  Match phrase queries against non-indexed fields should throw an exception (#31060)
  In the internal highlighter APIs, use the field type as opposed to the mapper. (#31039)
  [DOCS] Removes duplicated authentication pages
  Enable customizing REST tests blacklist (#31074)
  Make sure KeywordFieldMapper#clone preserves split_queries_on_whitespace. (#31049)
  [DOCS] Moves machine learning overview to stack-docs
  [ML] Add secondary sort to ML events (#31063)
  [Rollup] Specialize validation exception for easier management (#30339)
  Adapt bwc versions after backporting #31045 to 6.3
  Remove usage of explicit type in docs (#29667)
  Share common readFrom/writeTo code in AcknowledgeResponse (#30983)
  Adapt bwc versions after backporting #31045 to 6.x
  Mute MatchPhrase*QueryBuilderTests
  [Docs] Fix typo in watcher conditions documentation (#30989)
  Remove wrong link in index phrases doc
  Move pipeline APIs to ingest namespace (#31027)
  [DOCS] Fixes accounting setting names (#30863)
  [DOCS] Rewords _field_names documentation (#31029)
  Index phrases (#30450)
  Remove leftover debugging from PTCMDT
  Fix PTCMDT#testMinVersionSerialization
  Make Persistent Tasks implementations version and feature aware (#31045)
@jimczi jimczi added the v7.0.0 label Jun 4, 2018
@romseygeek romseygeek removed the v6.4.0 label Jul 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>breaking >enhancement :Search Foundations/Mapping Index mappings, including merging and defining field types v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants