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

Fix range queries on _type field for singe type indices (#31756) #32161

Merged
merged 2 commits into from Jul 23, 2018
Merged

Fix range queries on _type field for singe type indices (#31756) #32161

merged 2 commits into from Jul 23, 2018

Conversation

cbuescher
Copy link
Member

Backport of #31756, mostly opening to get CI run but also to get some feedback about support for multiple types indices in 6.x.

Fix range queries on _type field for singe type indices (elastic#…
0a6259d 
…31756)

With the introduction of single types in 6.x, the `_type` field is no longer
indexed, which leads to certain queries that were working before throw errors
now. One such query is the `range` query, that, if performed on a single typer
index, currently throws an IAE since the field is not indexed.
This change adds special treatment for this case in the TypeFieldMapper,
comparing the range queries lower and upper bound to the one existing type and
either returns a MatchAllDocs or a MatchNoDocs query.

Relates to elastic#31632
Closes elastic#31476
@cbuescher cbuescher added review :Search/Mapping Index mappings, including merging and defining field types v6.4.0 labels Jul 18, 2018
@cbuescher cbuescher requested a review from jpountz July 18, 2018 09:30
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

cbuescher
cbuescher commented Jul 18, 2018
View reviewed changes
server/src/main/java/org/elasticsearch/index/mapper/TypeFieldMapper.java Outdated
public Query rangeQuery(Object lowerTerm, Object upperTerm, boolean includeLower, boolean includeUpper, QueryShardContext context) {
if (hasDocValues()) {
return new TermRangeQuery(name(), lowerTerm == null ? null : indexedValueForSearch(lowerTerm),
upperTerm == null ? null : indexedValueForSearch(upperTerm), includeLower, includeUpper);
Copy link
Member Author

Choose a reason for hiding this comment

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

@jpountz could you take a look at this backport also, please? This was one of my open questions in #31756, whether we need this for cases where we have old 5.x indices with multiple types.

Copy link
Contributor

Choose a reason for hiding this comment

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

we need to take care of 5.x indices indeed, but the condition in the if statement should be context.getIndexSettings().isSingleType() == false

server/src/main/java/org/elasticsearch/index/mapper/TypeFieldMapper.java Outdated
DEPRECATION_LOGGER.deprecatedAndMaybeLog("range_single_type",
"Running [range] query on [_type] field for an index with a single type. As types are deprecated, this functionality will be removed in future releases.");
Query result = new MatchAllDocsQuery();
Collection<String> types = context.getMapperService().types();
Copy link
Member Author

Choose a reason for hiding this comment

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

context.getMapperService().documentMapper() is missing in 6.x, the existing method requires knowing the type. Should types() always return a 1-element collection in the single-type case on 6.x?

Copy link
Contributor

Choose a reason for hiding this comment

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

It may return no types if no mapping has been put, but indeed never more than 1 type. FYI the right way to check is context.getIndexSettings().isSingleType() rather than indexCreatedVersion >= 6.0

jpountz
jpountz requested changes Jul 23, 2018
View reviewed changes
server/src/main/java/org/elasticsearch/index/mapper/TypeFieldMapper.java Outdated
public Query rangeQuery(Object lowerTerm, Object upperTerm, boolean includeLower, boolean includeUpper, QueryShardContext context) {
if (hasDocValues()) {
return new TermRangeQuery(name(), lowerTerm == null ? null : indexedValueForSearch(lowerTerm),
upperTerm == null ? null : indexedValueForSearch(upperTerm), includeLower, includeUpper);
Copy link
Contributor

Choose a reason for hiding this comment

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

we need to take care of 5.x indices indeed, but the condition in the if statement should be context.getIndexSettings().isSingleType() == false

server/src/main/java/org/elasticsearch/index/mapper/TypeFieldMapper.java Outdated
DEPRECATION_LOGGER.deprecatedAndMaybeLog("range_single_type",
"Running [range] query on [_type] field for an index with a single type. As types are deprecated, this functionality will be removed in future releases.");
Query result = new MatchAllDocsQuery();
Collection<String> types = context.getMapperService().types();
Copy link
Contributor

Choose a reason for hiding this comment

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

It may return no types if no mapping has been put, but indeed never more than 1 type. FYI the right way to check is context.getIndexSettings().isSingleType() rather than indexCreatedVersion >= 6.0

@cbuescher
Copy link
Member Author

@jpountz thanks, I made the change and also added two unit tests to TypeFieldTypeTests. Can you take another look if this look okay to you now?

jpountz
jpountz approved these changes Jul 23, 2018
View reviewed changes
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.

LGTM!

@cbuescher cbuescher added the >bug label Jul 23, 2018
@cbuescher cbuescher merged commit 864711b into elastic:6.x Jul 23, 2018
dnhatn added a commit that referenced this pull request Jul 25, 2018
@dnhatn
Merge branch '6.x' into ccr-6.x
eef3abf 
* 6.x:
  Security: revert to old way of merging automata (#32254)
  Fix a test bug in RangeQueryBuilderTests introduced in the field aliases backport.
  Introduce Application Privileges with support for Kibana RBAC (#32309)
  Undo a debugging change that snuck in during the field aliases merge.
  [test] port linux package packaging tests (#31943)
  Painless: Update More Methods to New Naming Scheme (#32305)
  Tribe: Add error with secure settings copied to tribe (#32298)
  Add V_6_3_3 version constant
  Add ERR to ranking evaluation documentation (#32314)
  [DOCS] Added link to 6.3.2 RNs
  [DOCS] Updates 6.3.2 release notes with PRs from ml-cpp repo (#32334)
  [Kerberos] Add Kerberos authentication support (#32263)
  [ML] Extract persistent task methods from MlMetadata (#32319)
  Backport - Add Snapshots Status API to High Level Rest Client (#32295)
  Make release notes ignore the `>test-failure` label. (#31309)
  [DOCS] Adds release highlights for search for 6.4 (#32095)
  Allow Integ Tests to run in a FIPS-140 JVM (#32316)
  Add support for field aliases to 6.x. (#32184)
  Register ERR metric with NamedXContentRegistry (#32320)
  fixes broken build for third-party-tests (#32315) Relates #31918 / Closes infra/issues/6085
  [DOCS] Rollup Caps API incorrectly mentions GET Jobs API (#32280)
  Rest HL client: Add put watch action (#32026) (#32191)
  Add WeightedAvg metric aggregation (#31037)
  Consistent encoder names (#29492)
  Switch monitoring to new style Requests (#32255)
  specify subdirs of lib, bin, modules in package (#32253)
  Rename ranking evaluation `quality_level` to `metric_score` (#32168)
  Add new permission for JDK11 to load JAAS libraries (#32132)
  Switch x-pack:core to new style Requests (#32252)
  Watcher: Store username on watch execution (#31873)
  Silence SSL reload test that fails on JDK 11
  Painless: Clean up add methods in PainlessLookup (#32258)
  CCE when re-throwing "shard not available" exception in TransportShardMultiGetAction (#32185)
  Fail shard if IndexShard#storeStats runs into an IOException (#32241)
  Fix `range` queries on `_type` field for singe type indices (#31756) (#32161)
  AwaitsFix RecoveryIT#testHistoryUUIDIsGenerated
  Add new fields to monitoring template for Beats state (#32085) (#32273)
  [TEST] improve REST high-level client naming conventions check (#32244)
  Check that client methods match API defined in the REST spec (#31825)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpountz jpountz jpountz approved these changes

Assignees
No one assigned
Labels
>bug :Search/Mapping Index mappings, including merging and defining field types v6.4.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@cbuescher @elasticmachine @jpountz