Conversation
The engine feature this drove (opensearch-project/sql#5657) is being removed: returning a knowingly incomplete aggregation to make a query faster is not a good default to offer, and the rerun affordance implied a remedy that only ever applied to that one narrowing case. Removed: the explore:enablePartialResults advanced setting, the partial_result field sent on PPL queries, the disablePartialResults per-execution override and the bucket-count carve-out it existed for, useRerunWithoutPartialResults, and the "Rerun without partial results" button on the warning callout. Kept: the warnings channel itself -- warnings flowing from the PPL search strategy onto dataFrame.meta.warnings, the QueryWarning type, and QueryWarningsCallout rendering one banner per warning with its detail behind "Show more". The callout still titles a PARTIAL_RESULT warning "Partial results" and anything else "Warning", so a backend that attaches a warning keeps its banner with no further change here. Companion to opensearch-project/sql#5814. opensearch-project/sql#5807 is the next producer of warnings and relies only on the kept path. Signed-off-by: Kai Huang <ahkcs@amazon.com>
PR Reviewer Guide 🔍(Review updated until commit 313ab4c)Here are some key observations to aid the review process:
|
|
Persistent review updated to latest commit 27be75e |
✅ All unit and integration tests passing
|
|
Persistent review updated to latest commit f4402ef |
The comments and test fixtures around the warnings banner still described the
removed mapping-conflict mode ("Results exclude 1 of 2 indices due to a mapping
conflict"). Point them at a search that reached only some of its shards, the
case opensearch-project/sql#5807 reports under PARTIAL_RESULT.
Signed-off-by: Kai Huang <ahkcs@amazon.com>
|
Persistent review updated to latest commit e9a5d80 |
There was a problem hiding this comment.
Thanks for the change @ahkcs . One high level thing need to confirm with you. It seems like there should be a follow up work after this change. Due to the removal of opensearch-project/sql#5814, plus this change, my understanding is that this banner will not be triggering at all at the moment.
May need to update the description with the info of your follow up work. But in general this is looking good to me.
| dataset: schema.nullable(schema.object({}, { unknowns: 'allow' })), | ||
| format: schema.string(), | ||
| profile: schema.maybe(schema.boolean()), | ||
| partial_result: schema.maybe(schema.boolean()), |
There was a problem hiding this comment.
query has no unknowns option so it defaults to forbid, which means dropping this key makes a body that still carries it fail validation:
[partial_result]: definition for this key is missing
And the old client sent it on every PPL query, not just when the setting was on — the spread was keyed on language === 'PPL'. So a tab still running the old bundle 400s on every query until it reloads.
There was a problem hiding this comment.
Confirmed. It 400s on every PPL query when the bundle and server report the same version (across a version bump, OSD's osd-version check rejects the stale tab first). Fixed in 943735a: partial_result is accepted and ignored, the rest of query stays strict, with tests for both.
| requiresCapability: 'explore.logsQueryBuilderEnabled', | ||
| schema: schema.boolean(), | ||
| }, | ||
| [PARTIAL_RESULTS_SETTING]: { |
There was a problem hiding this comment.
This setting ships in 3.9.0, which releases on Sept 29. OSD 3.9 has #12481 and SQL 3.9 has opensearch-project/sql#5657, while opensearch-project/sql#5814 is only on main. So this PR removes an Advanced Setting that users will already have, which seems worth calling out in the description. If a 3.9.x backport comes up, taking this and opensearch-project/sql#5814 together keeps the two sides consistent. The documentation website has no mention of explore:enablePartialResults, so the docs item can be marked n/a.
There was a problem hiding this comment.
Thanks, good call. Added an upgrade note to the description covering the 3.9 → 3.10 removal and pairing any 3.9.x backport with sql#5814, and marked the docs item n/a.
| // The rerun action dispatches, which needs a store. This suite covers the tab's rendering; the | ||
| // banner's own tests cover the action. | ||
| jest.mock('../../application/hooks', () => ({ | ||
| useRerunWithoutPartialResults: () => jest.fn(), | ||
| })); | ||
| jest.mock('../../application/hooks', () => ({})); |
There was a problem hiding this comment.
Nothing in StatisticsTab imports ../../application/hooks after this change, so this mock and the comment above it, which still describes the rerun action, can both go. The suite passes with the three lines removed.
There was a problem hiding this comment.
Good catch, removed in 313ab4c. The suite passes without it.
Explore bundles built before this change send partial_result on every PPL query, and the route schema forbids unknown keys, so an open tab would get a 400 on every query after an upgrade until it reloads. Accept the key and drop it. Signed-off-by: Kai Huang <ahkcs@amazon.com>
|
Persistent review updated to latest commit 943735a |
1 similar comment
|
Persistent review updated to latest commit 943735a |
PR Code Suggestions ✨Latest suggestions up to 313ab4c Explore these optional code suggestions:
Previous suggestionsSuggestions up to commit 943735a
Suggestions up to commit 943735a
|
Yes, no banner will be triggered for now, but we want to keep the warning channel for later follow-up PRs I've updated the description on #12815. It now has a Follow-up section:
I also fixed a stale line: the description said |
StatisticsTab no longer imports application/hooks, so the mock and its comment about the rerun action are dead. Signed-off-by: Kai Huang <ahkcs@amazon.com>
|
Persistent review updated to latest commit 313ab4c |
Description
Companion to opensearch-project/sql#5814, which removes the engine-side opt-in partial-result mode for mapping conflicts (added in opensearch-project/sql#5657). This removes the Dashboards half added in #12481 — and keeps the warnings banner, which a different engine change still needs.
Why. The feature returned a knowingly incomplete aggregation to make a query faster: on a field mapped inconsistently across indices it aggregated over only the aggregatable subset and labelled the result. That is not a good default to offer, and the "Rerun without partial results" button implied a remedy that applied only to that one narrowing case — it does nothing for any other reason a result might be incomplete.
Removed
explore:enablePartialResultsPARTIAL_RESULTS_SETTINGconstantpartial_resulton the querydata/common/query/types.tsand thefacet.tspassthrough. The PPL route schema still accepts it and drops it, since older Explore bundles send it on every PPL querydisablePartialResultsexecuteQueries/executeHistogramQuery/executeTabQuery, plus the bucket-count carve-out that existed only to stop the denominator being undercounteduseRerunWithoutPartialResultsonRerunWithoutPartialResultsprop throughQueryWarningsCallout→ExploreDataTable/StatisticsTabKept
The warnings channel, end to end:
ppl_search_strategy.tsstill copiesdata.warningsontodataFrame.meta.warningsQueryWarningstays in the results sliceQueryWarningsCalloutstill renders one banner per warning, message always visible,detailbehind "Show more", titlingPARTIAL_RESULTas "Partial results" and anything else as "Warning"So a backend that attaches a warning keeps its banner with no further Dashboards change.
Follow-up
With opensearch-project/sql#5814 merged, nothing produces a warning yet, so the banner won't show until these land (no further Dashboards change needed):
PARTIAL_RESULT, which the banner titles "Partial results".Upgrade note
explore:enablePartialResultsships in OSD 3.9.0 (#12481), paired with the engine side in opensearch-project/sql#5657. This PR and opensearch-project/sql#5814 remove it in 3.10, so users upgrading from 3.9 lose the setting:partial_result, so the cluster default (off) applies. OSD 3.9 against SQL 3.10 still sends it, and the engine ignores it.Testing
node scripts/jest.json the affected areas: 34 suites / 535 tests forstate_management/actionsandquery_enhancements/server, plus 20 tests acrossquery_warnings_callout,explore_data_tableandstatistics_tab— all passing. The callout's three rerun-specific cases are replaced by one asserting an unrecognised type still gets the generic "Warning" title.i18n-checkandeslintclean. Two route tests check that a body carryingpartial_resultis accepted and that other unknown keys are still rejected.Check List
--signoffexplore:enablePartialResultsisn't on the documentation websiteBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.