-
Notifications
You must be signed in to change notification settings - Fork 220
Add an opt-in partial-result mode for aggregations on text/keyword mapping conflicts #5657
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
Open
ahkcs
wants to merge
22
commits into
opensearch-project:main
Choose a base branch
from
ahkcs:feature/ppl-partial-result-warning-channel
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
992a78c
Add non-fatal warning channel to PPL query response
ahkcs 33aed50
Return a partial result instead of exhausting PIT on a mapping conflict
ahkcs c354738
Gate partial results on a warning-capable response format
ahkcs 70c344c
Flatten per-index mappings when partitioning for partial results
ahkcs 12867b2
Refine partial-result index selection and warning wording
ahkcs ff7845f
Truncate the excluded-index list in the partial-result warning
ahkcs 7585190
Fix partial-result warning wording to reflect the pushdown criterion
ahkcs bd883d8
Extract partial-result partitioning into its own class with unit tests
ahkcs 95d9ac6
Allow a per-request override for partial-result mode
ahkcs 14a35cb
Simplify partial-result warning to name the excluded indices and the fix
ahkcs 01a2436
Do not resolve response format for explain requests
ahkcs 4b04df9
Cover the null-warnings branch in QueryResult to satisfy protocol cov…
ahkcs 806a07a
Address review: rename the partial-result setting and fold the fallba…
ahkcs 0d4c8c0
Reuse the already-fetched index mappings for partial-result partitioning
ahkcs 6379c85
Fix formatting of the renamed partial-result setting key
ahkcs 40dad6e
Revert the index-mapping reuse optimization: it exposed a merge-mutat…
ahkcs f5be912
Reuse the already-fetched index mappings, and stop the merge mutating…
ahkcs 7631342
Clear the per-request partial-result state after each query
ahkcs 0018315
Update explain golden files for the changed OpenSearchDataType serial…
ahkcs b860306
Decide partial-result mode before pushdown analysis, not after it fails
ahkcs 43e5668
Document the partial-result-on-mapping-conflict setting
ahkcs 4bb2d5f
Tighten inline comments on the partial-result path
ahkcs File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
34 changes: 34 additions & 0 deletions
34
core/src/main/java/org/opensearch/sql/executor/Warning.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| /* | ||
| * Copyright OpenSearch Contributors | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| package org.opensearch.sql.executor; | ||
|
|
||
| import lombok.Data; | ||
|
|
||
| /** | ||
| * A non-fatal notice attached to an otherwise-successful query response. Carried through the | ||
| * response path so consumers can distinguish a correct-but-noteworthy result (e.g. a partial result | ||
| * over a subset of indices) from a plain success, without turning it into an error. | ||
| */ | ||
| @Data | ||
| public class Warning { | ||
|
|
||
| /** | ||
| * The result is complete for the indices it covers but omits one or more indices that could not | ||
| * be served (e.g. a mapping conflict that prevents aggregation pushdown). This is a cross-surface | ||
| * contract: consumers such as OpenSearch Dashboards branch on this {@code type} value, so it must | ||
| * not change without coordinating those consumers. | ||
| */ | ||
| public static final String TYPE_PARTIAL_RESULT = "PARTIAL_RESULT"; | ||
|
|
||
| /** Machine-readable category, e.g. {@link #TYPE_PARTIAL_RESULT}. */ | ||
| private final String type; | ||
|
|
||
| /** Short human-readable summary. */ | ||
| private final String message; | ||
|
|
||
| /** Optional longer explanation with the specifics and remedy; may be null. */ | ||
| private final String detail; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -236,6 +236,53 @@ Result set:: | |
| } | ||
| } | ||
|
|
||
| plugins.query.partial_result.on_mapping_conflict.enabled | ||
| ======================================================== | ||
|
|
||
| Version | ||
| ------- | ||
| 3.8 | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. since 3.9 |
||
|
|
||
| Description | ||
| ----------- | ||
|
|
||
| Controls how an aggregation behaves when its group-by field is mapped inconsistently across the queried indices -- for example ``keyword`` in some indices of a wildcard pattern and ``text`` (without a ``.keyword`` sub-field) in others. Such a field collapses to ``text``-without-``.keyword`` across the pattern, which has no doc values, so the aggregation cannot be pushed down natively and instead runs as a per-document script over ``_source`` -- correct, but a full scan of every document. | ||
|
|
||
| When this setting is ``false`` (the default), that complete-but-slow result is returned. When set to ``true``, the aggregation is pushed down over only the subset of indices where the field is aggregatable, and the response carries a ``PARTIAL_RESULT`` warning naming the excluded indices and the remedy (map the field as ``keyword`` everywhere). The result is therefore **partial** -- documents in the excluded indices are not counted -- so the setting is off by default and only takes effect for response formats that can surface the warning (the JSON format; CSV/raw/visualization responses fall through to the complete result rather than silently dropping data). | ||
|
|
||
| The behavior can also be overridden per request with the ``partial_result`` boolean field in the query body, which takes precedence over this cluster setting. Here is an example enabling it at the cluster level:: | ||
|
|
||
| >> curl -H 'Content-Type: application/json' -X PUT localhost:9200/_plugins/_query/settings -d '{ | ||
| "transient" : { | ||
| "plugins.query.partial_result.on_mapping_conflict.enabled" : true | ||
| } | ||
| }' | ||
|
|
||
| Result set:: | ||
|
|
||
| { | ||
| "acknowledged" : true, | ||
| "persistent" : { }, | ||
| "transient" : { | ||
| "plugins" : { | ||
| "query" : { | ||
| "partial_result" : { | ||
| "on_mapping_conflict" : { | ||
| "enabled" : "true" | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Per-request override example, opting a single query into a partial result regardless of the cluster setting:: | ||
|
|
||
| >> curl -H 'Content-Type: application/json' -X POST localhost:9200/_plugins/_ppl -d '{ | ||
| "query" : "source=logs-* | stats count() by service", | ||
| "partial_result" : true | ||
| }' | ||
|
|
||
| plugins.query.buckets | ||
| ===================== | ||
|
|
||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mark as experimental.