Conversation
PR Reviewer Guide 🔍(Review updated until commit 336be69)Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Latest suggestions up to 336be69 Explore these optional code suggestions:
Previous suggestionsSuggestions up to commit abd9510
Suggestions up to commit d676381
Suggestions up to commit c128ebd
Suggestions up to commit 2ee666e
Suggestions up to commit efcd23d
|
The schema-conflict half of Approach 1 that opensearch-project#5727 left open, implemented as Approach 3 of the design on opensearch-project#5698: a request-level time range, applied while the table is still being resolved. A wildcard index expression is expanded -- and the mapping of every index it matches merged -- before any predicate in the query text has been parsed, so nothing downstream of that resolution can narrow it. opensearch-project#5727 reads its range from the filter already sitting in the pushdown context, which is the broadest coverage available without an API change but runs too late for the merge. Declaring the range on the request puts it in hand before planning starts: start_time/end_time/time_field -> PPLQueryRequest.getTimeBounds -> AbstractPlan -> QueryService.buildFrameworkConfig (seeded once) -> OpenSearchSchema.registerTable -> OpenSearchStorageEngine.getTable(.., TimeBounds) -> IndexPruner -> OpenSearchIndex Following the design's shape: - Bounds are global, applied to every table the query resolves rather than attributed to one relation -- the scope Splunk's time range picker and ES|QL's request-level filter have. That is what lets them be a plain request parameter with no AST work, no visitor change and no ordering problem. - Narrowing happens as the table is built, via a new SupportsIndexPruning the storage engine implements, not by mutating a table afterwards: a table's row type is derived from the merge, so once one exists the cost is already paid. The interface keeps the probe in the module owning a client to probe with, and core free of OpenSearch types. - Bounds are carried as the strings the request sent and handed to _field_caps as-is, so OpenSearch's own date parser reads them and date math works. Re-interpreting them here risks a narrower window than the caller meant, which would drop an index that can match -- the divergence hazard the design flags for Approach 1. Two additions the design leaves implicit. time_field makes the field explicit rather than assuming @timestamp, because a Dashboards index pattern is routinely configured on another field and would otherwise never prune. And the probe's format list accepts a UTC wall clock alongside the OpenSearch defaults, since a client that also writes the bound into the query text produces the spelling PPL accepts there; without it such a bound fails to parse and pruning silently declines. A source whose time_field is not a date excludes itself, read off the same probe response, so a range that cannot prove an index disjoint prunes nothing. Unusable bounds are dropped rather than rejected: they only decide which indices are read, so failing a query over a parameter it does not need is the worse outcome. Gated by plugins.query.pruning.enabled, which opensearch-project#5759 turns on by default. Verified on a security-enabled cluster over eight monthly indices whose newest one renamed a field's shape from object to keyword. Without bounds the merge sees the object and charting by it fails; with them only the newest is resolved and the same query returns its rows. Row counts are identical either way, a range covering every index prunes none, and a range no index can match declines rather than pruning all. Signed-off-by: Kai Huang <ahkcs@amazon.com>
c16b768 to
c90cee5
Compare
|
Persistent review updated to latest commit c90cee5 |
Correctness An index that does not map the time field was pruned away with its rows. A range on an unmapped field is *disjoint* for that index, not unknown, so _field_caps omits it exactly as it omits an index whose values fall outside the window -- the two are indistinguishable in the response, and the previous type check saw only the date entry the mapping indices reported and let pruning proceed. Verified against a cluster: a wildcard over one index mapping `ts` and one not returns only the former from the probe. The gate now runs on its own unfiltered probe with include_unmapped, which is the only way to see the indices a filtered probe has already hidden, and declines when the field is unmapped anywhere, not a date anywhere, or mapped by nothing. Costs one metadata probe, paid before the filtered one so a declining query stops early. New IT keeps a row that lives only in an index without the field. The gate also ran before the candidate count, so a window matching no index reported "[ts] is not a date field" -- field-caps returns an empty field map along with an empty index list -- and the "0 of N indices matched" line could never fire. Reordered. analyze dropped the bounds on both phases, so `"analyze": true` merged every matched index's mapping and reported a plan over an expression the execution would not have used. Threaded, as explain already was. Broken surface removed The URL-parameter loop could not work: BaseRestHandler rejects parameters absent from responseParams(), so start_time in a query string was a 400, and a real GET carries no jsonContent for the loop to write into. Dropped rather than half-supported. Tests createRoleWithIndexAccess granted neither indices:admin/resolve/index nor indices:data/read/field_caps*, so under security both probes were denied, pruning declined silently, and TimeBoundsPruningSecurityIT's positive case could not pass. Granted, matching what ppl_full_access carries since 3.9. Both ITs pinned the pruning setting to false on teardown; since opensearch-project#5759 made it default true, that left every later class in the cluster running with pruning off. They clear the override instead. Registered CalciteTimeBoundsPruningIT in CalciteNoPushdownIT, which the repo requires and which matters here: request-level pruning is meant to be independent of pushdown. Accuracy Documented that these bounds are not free of effect. Pruning drops whole indices, so a query whose text already constrains the same field to the same window returns the same rows -- the intended use -- while one that does not returns fewer. The previous claim that a request is "answered identically" was false, and this PR's own subsearch IT asserts a row count changing. Corrected the claim that a bound is read by the index's own date parser: the probe's format list replaces the field's, so a field with some other custom format is not pruned on. Said so, and noted that declining costs the optimization and never a row. Stopped promising the unified query path reads these; it builds its own schema and ignores them. The "Request-level time bounds" heading was a section sibling, sweeping the setting's own disable/result-set examples under it. Demoted to prose. Renamed the leftover "hint" and "time_range" vocabulary in tests to the shipped start_time/end_time/TimeBounds, reused OpenSearchConstants.IMPLICIT_FIELD_TIMESTAMP instead of a second copy, and made a one-sided window warn rather than vanish. Signed-off-by: Kai Huang <ahkcs@amazon.com>
|
Persistent review updated to latest commit 1380ef7 |
…hose indices Reviewing this against the Dashboards side turned up two things the separate reviews could not see. The guard added for the unmapped-field case was applied to both pruning paths, which regressed opensearch-project#5727's own yaml case "Prunes an index that does not map the timestamp". The two paths are not alike. When the range is read out of the query's own pushed-down filter, an index that does not map the field cannot satisfy that filter either, so its documents are already excluded from the answer and dropping the index is lossless -- which is exactly what that test asserts. When the range arrives as a request parameter the query text need not mention the field at all, so those documents are still wanted. The guard now applies only to the bounds path; the filter path is untouched, and the yaml suite passes again. And rather than declining outright when the field is unmapped somewhere, the bounds path now keeps those indices and prunes among the rest. _field_caps already names them in the unmapped bucket of an include_unmapped probe, so this costs nothing and turns a case that gave up into one that prunes correctly -- worth having because a Dashboards index pattern spanning a stray index without the time field would otherwise never prune at all. Declining is kept only where nothing better is possible: no index maps the field, it is mapped as a non-date somewhere, or the probe does not name the unmapped indices. Signed-off-by: Kai Huang <ahkcs@amazon.com>
|
Persistent review updated to latest commit d7b6b4c |
Got integTestWithSecurity running locally for the first time on this branch, and the suite failed two of its four cases -- not on the handoff it was written for, but because of how it observed pruning. It asserted that charting by a field mapped as an object in one index and a keyword in another fails without bounds. Which side of that conflict wins the merge depends on hash iteration order, so it varies between JVMs: the earlier manual run on a standalone cluster returned the expected 400, this one returned 200 and the two negative cases failed. The assertion was never sound. Observes the resolved schema instead, as CalcitePPLTimeBounds... already does: a field that exists only in the out-of-range index either resolves or does not, which is decided by which indices the merge saw and nothing else. Adds a row-count parity case while here, since that is the property that matters most and the old shape could not express it. Confirmed the permission grant added earlier is load-bearing by taking it back out: the positive case then fails with "nothing was thrown", pruning having declined on a denied probe exactly as predicted. Restored, and 5/5 pass. Signed-off-by: Kai Huang <ahkcs@amazon.com>
|
Persistent review updated to latest commit c792001 |
Codecov Report❌ Patch coverage is ❌ Your project check has failed because the head coverage (62.88%) is below the target coverage (99.00%). You can increase the head coverage or adjust the target coverage.
Additional details and impacted files@@ Coverage Diff @@
## main #5766 +/- ##
=============================================
- Coverage 98.40% 62.88% -35.52%
- Complexity 2746 8801 +6055
=============================================
Files 266 938 +672
Lines 6758 40198 +33440
Branches 426 4520 +4094
=============================================
+ Hits 6650 25280 +18630
- Misses 107 14117 +14010
- Partials 1 801 +800
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…cannot match Reverts the retain-unmapped behaviour, and with it the mapping probe that existed to detect the case. Under a request-level time range a document with no time value is in no window, so the index holding it can be excluded on the same evidence as one whose values fall outside the range. That is what the pushed-down-filter path does -- opensearch-project#5727's own yaml case asserts it -- and what ES|QL's request filter and Splunk's picker do. Retaining such an index was the inconsistent special case, and it protected only a usage the manual already tells callers not to attempt: bounds sent without an equivalent predicate in the query text, where rows are dropped for in-range indices anyway. It also cost a warning. Keeping an index that maps none of the group fields gives the partial-result feature something to exclude, so a query that had no mapping conflict gained a "mapping conflict" banner naming a field that is merely absent, with a rerun prompt that would buy nothing. The probe goes with it. An unmapped time field now self-declines through the existing empty-candidate path -- every index reports as non-matching, so nothing is substituted -- which is one fewer blocking round trip per distinct source on the bounds path. The consequence is deliberate and now documented: pruning removes the index's schema along with its rows, so a query naming a field only that index maps fails with "Field [x] not found." rather than quietly returning nothing. The IT asserts the loud failure. Signed-off-by: Kai Huang <ahkcs@amazon.com>
The warning listed every group key rather than the fields that could not be aggregated, which made it both inaccurate and impossible to de-duplicate. Inaccurate because a key that is fine everywhere gets named as the problem: charting by a keyword field over a date produced "mapping conflict on [attributes.cluster, timestamp]" when the date was aggregatable in every index and nothing about it needed fixing. Impossible to de-duplicate because the planner raises this once per equivalent plan alternative -- which drainWarnings() collapses by value, and opensearch-project#5657 wrote it for exactly that -- but the group keys arrive in whatever order the alternative had them, so logically identical findings differed as strings and survived as separate banners. A single chart query showed three: Results exclude 1 of 2 indices due to a mapping conflict on [attributes.cluster, timestamp]. Results exclude 1 of 2 indices due to a mapping conflict on [timestamp, attributes.cluster]. Results exclude 1 of 2 indices due to a mapping conflict on [attributes.cluster]. Reporting only the offending fields makes those three identical, so the existing dedup collapses them to one, and the one that remains names what to fix. The list is sorted for the same reason the excluded-index list already is: so plan ordering cannot reach the text. Reproducible without index pruning involved -- a plain text/keyword conflict and `chart count() over ts by env` -- and verified against a cluster: three banners before, one after, naming [env] alone. Signed-off-by: Kai Huang <ahkcs@amazon.com>
|
Persistent review updated to latest commit 7a3d063 |
|
|
||
| Pruning is also skipped when it would not reduce the read, that is when no index is excluded. The query then uses the original wildcard expression and reads exactly the same indices. | ||
|
|
||
| **Request-level time bounds.** ``start_time`` and ``end_time`` declare the window the request is asking about, so the engine has it before it resolves the queried index expression. They are inclusive, and accept OpenSearch date math (``now-7d``) and absolute timestamps alike. The bounds are handed to the probe as sent rather than reinterpreted, so a relative range is resolved once, by OpenSearch. Absolute bounds are parsed as ``strict_date_optional_time``, epoch milliseconds, or ``yyyy-MM-dd HH:mm:ss.SSS``; a field declaring some other custom format is not pruned on, since the bound fails to parse and pruning then declines. ``time_field`` names the field they constrain, defaulting to ``@timestamp`` -- a caller whose index pattern is configured on another field has to say so, or nothing is pruned. Bounds that cannot be used are ignored rather than failing the query. |
There was a problem hiding this comment.
so the engine has it before it resolves the queried index expression.
This is user facing doc. Do not need to explain the engine details.
|
|
||
| **Request-level time bounds.** ``start_time`` and ``end_time`` declare the window the request is asking about, so the engine has it before it resolves the queried index expression. They are inclusive, and accept OpenSearch date math (``now-7d``) and absolute timestamps alike. The bounds are handed to the probe as sent rather than reinterpreted, so a relative range is resolved once, by OpenSearch. Absolute bounds are parsed as ``strict_date_optional_time``, epoch milliseconds, or ``yyyy-MM-dd HH:mm:ss.SSS``; a field declaring some other custom format is not pruned on, since the bound fails to parse and pruning then declines. ``time_field`` names the field they constrain, defaulting to ``@timestamp`` -- a caller whose index pattern is configured on another field has to say so, or nothing is pruned. Bounds that cannot be used are ignored rather than failing the query. | ||
|
|
||
| Their scope is the whole request: every source the query reads is narrowed, subsearches included, as with Splunk's time range picker and OpenSearch SQL's own PPL ``earliest``/``latest`` at request level. |
There was a problem hiding this comment.
What if JOIN? Applied to all indices?
|
|
||
| Their scope is the whole request: every source the query reads is narrowed, subsearches included, as with Splunk's time range picker and OpenSearch SQL's own PPL ``earliest``/``latest`` at request level. | ||
|
|
||
| They are not a filter, and they are not free of effect either. Pruning drops whole indices, so a query whose text already constrains the same field to the same window returns exactly the same rows -- that is the intended use, and how a client appending its own ``where`` should send them. A query whose text does not carry that constraint returns fewer rows: documents outside the window still count inside a retained index, while an index wholly outside it contributes nothing. Send bounds only for a window the query itself already restricts. |
There was a problem hiding this comment.
what is the intend of this section?
| "start_time" : "now-7d", | ||
| "end_time" : "now" |
There was a problem hiding this comment.
what are supported literal of start_time and end_time. The doc should clear on it.
| { | ||
| "query" : "source=logs-* | stats count() by span(@timestamp, 1h)", | ||
| "time_field" : "@timestamp", | ||
| "start_time" : "now-7d", |
There was a problem hiding this comment.
| // Say so: a one-sided window is almost always a typo in the other key, and dropping it in | ||
| // silence leaves no trace of why nothing was pruned. |
| package org.opensearch.sql.opensearch.storage.scan; | ||
|
|
||
| import java.util.ArrayList; | ||
| import java.util.Collection; |
There was a problem hiding this comment.
why change this file? seems not releated to this PR.
| * @param bounds request-level bounds | ||
| * @return expression to read, never null | ||
| */ | ||
| public IndexName prune(IndexName indexName, TimeBounds bounds) { |
There was a problem hiding this comment.
why add new interface? Instead, build a range query and reuse exsting prune API
@penghuo on opensearch-project#5766: this is a user-facing document and does not need to explain engine internals. Rewrote the section around what a caller does and gets. Gone: when the bounds arrive relative to index resolution, that they are handed to a probe rather than reinterpreted, that a relative range is resolved once by OpenSearch, and that a failure to parse makes pruning "decline" -- all true, none of it actionable from outside. The same lens applied to the two paragraphs above it, which had the same problem: the list of "ways the range can reach the pruner" described the mapping merge and the PIT rather than saying that a query filtering on @timestamp needs no configuration and everything else needs the parameters. Two fixes while there. The heading was underlined at the same level as Version and Description, making it their sibling and pulling the setting's own disable and result-set examples underneath it; it is now nested under Description. And the example sent bounds for a window its query did not restrict, contradicting the rule stated directly above it -- it now shows the intended shape, bounds repeating a where clause on the same field and range. Signed-off-by: Kai Huang <ahkcs@amazon.com>
|
Persistent review updated to latest commit dfaf49e |
Comments across the change were explaining at length what the code and the user manual already say. Trimmed from 276 lines to 184, keeping only what is not evident from reading: why narrowing happens as the table is built rather than after, why the probe's format list replaces the field's own, why the warning's field list is sorted, and why the security IT's role needs the two probe actions. Removed the restatements of the ordering argument, which appeared in five places and belongs in the PR and the manual rather than in every javadoc that touches it. No behaviour change; unit suites unchanged at 9119. Signed-off-by: Kai Huang <ahkcs@amazon.com>
|
Persistent review updated to latest commit 616e021 |
@penghuo on opensearch-project#5766: what if JOIN -- applied to all indices? Yes, and it changes the answer. Verified against a cluster: a join whose other side reads its own wildcard, matching one in-window and one out-of-window index, contributes 2 rows without bounds and 1 with them, the pruner reporting joinref-* -> joinref-new. The outer query never names that pattern. Two limits, both tested. A concrete name on the other side is untouched, since pruning needs a wildcard. And a side lying entirely outside the window is not emptied: no index matches, so the probe returns nothing and pruning declines rather than substituting an empty list. The manual said "every source the query reads is narrowed" and left the reader to work out what that means for a join. It now says it, and says what to do instead. The existing subsearch IT could not have caught this -- both sides read the same pattern, so one cached table served both. The new case gives the join side a pattern of its own. Signed-off-by: Kai Huang <ahkcs@amazon.com>
|
Persistent review updated to latest commit 42f9f71 |
@penghuo on opensearch-project#5766: what is the intent of this section? It had three intents and led with none of them -- a usage rule, the mechanism behind it, and two edge cases -- and it restated its own conclusion in the paragraph above, which ended with the same instruction. Now one rule, first sentence: send the bounds only alongside an equivalent filter in the query. Then why, in one clause: they exclude whole indices, so a window the query does not also restrict returns fewer rows. The mechanism is gone; a reader does not need to know that documents outside the window still count inside a retained index to follow the rule. The join and subsearch scope follows as a consequence rather than repeating the instruction. Signed-off-by: Kai Huang <ahkcs@amazon.com>
|
Persistent review updated to latest commit 9a5849c |
@penghuo on opensearch-project#5766: what are the supported literals -- the doc should be clear on it. Replaced the prose list with a table, one row per form, each with examples. Testing every candidate against a cluster rather than reading it off the format string turned up two things the prose had wrong. `yyyy-MM-dd HH:mm:ss` without milliseconds did not parse, only the `.SSS` form did. PPL accepts that exact literal in a `where` clause, so a client mirroring its own filter into these parameters would have got silent non-pruning. Added to the accepted formats, with the date-only form pinned in TimeBoundsTest alongside it. Epoch seconds are not accepted and cannot be: a ten-digit number parses as milliseconds, so it means January 1970 rather than failing. Nothing is pruned, since no index matches that window, but the doc now says so instead of leaving it to be discovered. Verified after the change: every listed form prunes, epoch seconds and malformed values decline. Signed-off-by: Kai Huang <ahkcs@amazon.com>
PR Code Analyzer ❗AI-powered 'Code-Diff-Analyzer' found issues on commit abd9510.
The table above displays the top 10 most important findings. Pull Requests Author(s): Please update your Pull Request according to the report above. Repository Maintainer(s): You can Thanks. |
|
Persistent review updated to latest commit efcd23d |
@penghuo on opensearch-project#5766: update docs/user/interfaces/endpoint.rst too. That is where a PPL request-body parameter belongs -- fetch_size is documented there, not only in the settings manual -- so the parameters had been described where the setting lives and nowhere a reader looking up the API would find them. Adds a "Time Bounds (PPL) [Experimental]" section following the Fetch Size one: what the three parameters do, the rule that they go alongside an equivalent filter rather than instead of one, the accepted literals as a table, and a worked example. Cross-references the setting for the limitations rather than repeating them. The example is a real transcript. Ran it against a cluster with two monthly indices and pasted what came back, including that the engine read only February. Also checked the command is valid shell as written: PPL string literals are double-quoted, since a single-quoted literal cannot be escaped inside curl's single-quoted -d argument. Signed-off-by: Kai Huang <ahkcs@amazon.com>
|
Persistent review updated to latest commit 2ee666e |
@penghuo on opensearch-project#5766: remove AI comments. An earlier pass shortened them, which was not the ask. Deleted instead: 276 lines originally, 184 after shortening, 107 now, and what is left is javadoc on the public surface plus two comments carrying facts the code cannot. Gone entirely are the inline comments restating the line beneath them -- that explain resolves the same tables, that bounds are passed as sent, that a LOG.warn exists because a one-sided window is usually a typo -- and the paragraphs of rationale in test javadoc, which belong in the PR and the manual. Kept: that the alias and data-stream gates must come last because they resolve the expression, that ppl_full_access grants the two probe actions, and one-line contracts on the new public types. Also removes a comment in IndexPrunerTest that had gone stale, still describing the unfiltered mapping probe deleted when the unmapped-field guard was reverted -- worse than verbose, since it described behaviour that no longer exists. Unit 9121, pruning IT 10, both unchanged. Signed-off-by: Kai Huang <ahkcs@amazon.com>
|
Persistent review updated to latest commit c128ebd |
@penghuo on opensearch-project#5766: why change this file, it seems unrelated -- is it absolutely needed, can we remove it? Removed. It is not needed here, and the reason I had for carrying it no longer holds. The three-banner behaviour it fixed is reproducible on main with pruning off and no bounds at all -- a text/keyword conflict plus `chart count() over ts by env` -- so this PR never caused it. I originally carried the fix because an earlier revision of this PR retained an index that did not map the time field, which gave the partial-result feature something to exclude and so surfaced the banners. That retention was reverted; without it, pruning only ever reduces the index count, and partial-result needs two disagreeing indices, so this change makes those warnings less likely rather than more. Kept as a patch for its own PR against opensearch-project#5657, where it can be reviewed on its merits: the warning names every group key rather than the ones that cannot be aggregated, which is both inaccurate and prevents drainWarnings() from de-duplicating repeated plan alternatives. Signed-off-by: Kai Huang <ahkcs@amazon.com>
|
Persistent review updated to latest commit d676381 |
chart and timechart desugar into two aggregations over the same scan -- one for the rows charted, one to rank the top-N columns -- each with its own group keys. Partial-result mode partitions indices per aggregate, so the two can exclude different index sets: the ranking is then computed over indices whose rows are not in the chart. It also raised one warning per aggregate, which de-duplication by rendered text could not collapse. Mark the plan when a chart is visited and skip the partial-result path for such a query, returning the complete result instead. The mark rides the thread-local snapshot because pushdown can run on the complex worker pool. Signed-off-by: Kai Huang <ahkcs@amazon.com>
|
Persistent review updated to latest commit abd9510 |
Signed-off-by: Kai Huang <ahkcs@amazon.com>
|
Persistent review updated to latest commit 336be69 |
2 participants
Description
Implements the schema-conflict half of Approach 1 that #5727 left open, as Approach 3 of that design: a request-level time range, applied while the table is still being resolved.
A wildcard index expression is expanded — and the mapping of every index it matches merged — before any predicate in the query text has been parsed, so nothing downstream of that resolution can narrow it. #5727 reads its range from the filter already sitting in the pushdown context, which is the broadest coverage available without touching the request contract but runs after the merge. Declaring the range on the request puts it in hand before planning starts:
Following the design's shape:
filterhave. That is what lets them be a plain request parameter with no AST work, no visitor change and no ordering problem.SupportsIndexPruningthe storage engine implements, not by mutating a table afterwards: a table's row type is derived from the merge, so once one exists the cost is already paid. The interface keeps the probe in the module owning a client to probe with, andcorefree of OpenSearch types._field_capsas-is, so OpenSearch's own date parser reads them and date math works. Re-interpreting them here risks a narrower window than the caller meant, which would drop an index that can match — the divergence hazard the design flags for Approach 1.Gated by
plugins.query.pruning.enabled, which #5759 turns on by default.Two additions the design leaves implicit
time_field. The design assumes@timestamp. A Dashboards index pattern is routinely configured on another field — OpenSearch Dashboards' own sample-data pattern usestimestamp— and would otherwise never prune. Defaults to@timestamp, so a request that says nothing behaves as designed.An index that does not map
time_fieldis pruned like any other that cannot match: under a request-level time range a document with no time value is in no window, which is also what the pushed-down-filter path does (index_pruning.ymlasserts it). The consequence is deliberate and documented — such an index's rows, and its schema, go with it, so a query naming a field only it maps fails withField [x] not found.rather than quietly returning nothing.A format list on the probe range. Passing bounds through raw means the field's own format applies, and OpenSearch's default rejects
YYYY-MM-DD HH:mm:ss.SSS— the spelling a client that also writes the bound into the query text produces, since PPL accepts it there. Without this the probe throws, the exception is caught, and pruning silently declines. This was caught only by the ITs; 2 of 8 failed with "nothing was pruned" and the sole symptom was absence. The list isstrict_date_optional_time||epoch_millis||yyyy-MM-dd HH:mm:ss.SSS; date math is resolved before any of it applies.Example
Eight monthly indices. The newest renamed a field's shape —
attributes.clusterwas an object holdingname, and is a plain keyword after the roll — so merging them makes charting by it fail.Without bounds, every index is merged and the object wins:
POST _plugins/_ppl {"query": "source=prune-demo-2026.* | where `timestamp` >= '...' and `timestamp` <= '...' | chart count() over timestamp by attributes.cluster"} {"error": {"reason": "Cannot chart by [attributes.cluster] because it is an object.", "status": 400}}With them, only the index that can hold data in the window is resolved:
{"query": "...", "time_field": "timestamp", "start_time": "now-15m", "end_time": "now"} {"schema": [{"name": "timestamp"}, {"name": "attributes.cluster"}, {"name": "count()"}], "total": 50}A wider picker range prunes proportionally rather than all-or-nothing —
prune-demo-2026.* → .07,.08,.09for two months — and a range no index can match declines rather than pruning everything:Testing the changes
core+ppl+opensearchunitCalciteTimeBoundsPruningIT(new)CalciteExplainITCalcitePPLBasicITyamlRestTest(incl. #5727'sindex_pruning.yml)TimeBoundsPruningSecurityIT(new)The explain and basic suites are regression cover for the
QueryService/QueryPlan/ExplainPlansignature changes.New IT cases: the schema resolves from the in-range indices only (a field only the out-of-range ones map stops resolving); the whole pattern resolves without bounds; row counts are identical with and without; a range covering every index prunes none; bounds are ignored when the setting is off, when unusable, and when the field is unmapped; an index without the time field is pruned along with its schema; and bounds reach a subsearch's source too.
Also verified live on a security-enabled cluster, since the bounds cross the transport→worker handoff that #5739 and #5758 each had to fix for a different per-request signal — here they ride the object graph, so there is nothing on
ThreadContextto drop.TimeBoundsPruningSecurityITcovers it; noteintegTestWithSecuritycannot run locally at the moment because today's snapshot distro shipsjackson-core-2.22.1while the security plugin's own zip carries2.22.2(the existingPartialResultSecurityITfails identically), so that suite is CI-verified only.Unusable bounds are dropped rather than rejected: they only decide which indices are read, so failing a query over a parameter it does not need is the worse outcome.
API surface
Three new request-body parameters —
start_time,end_time,time_field— and nothing else:POST /_plugins/_pplis unchanged.TransportPPLQueryRequest.writeTo, so there is noStreamOutputchange and no version gate.fetch_sizeandpartial_resultare the precedents for a planning-affecting body field read the same way.They do change behaviour when present, which is the point, so this is an API addition rather than a purely internal change — the api-specification companion PR is noted unchecked below.
Internally it also adds
SupportsIndexPruningtocore's storage package. That is plugin-internal, optional, and implemented only byOpenSearchStorageEngine; other engines are untouched.Files touched
core/.../executor/TimeBounds.javacore/.../storage/SupportsIndexPruning.javacoreneeds no OpenSearch types and other engines are unaffectedcore/.../calcite/OpenSearchSchema.javacore/.../executor/QueryService.javabuildFrameworkConfig, on the execute, explain and analyze pathscore/.../execution/{AbstractPlan,QueryPlan,ExplainPlan,AnalyzePlan}.javappl/.../domain/PPLQueryRequest.javastart_time/end_time/time_fieldoff the bodyppl/.../PPLService.javaopensearch/.../storage/OpenSearchStorageEngine.javaSupportsIndexPruning; builds the table already narrowedopensearch/.../request/IndexPruner.javaopensearch/.../scan/PartialResultAggregatePushdown.javadocs/user/admin/settings.rstAlso fixed here: three partial-result banners for one finding
Surfaced by this work but independent of it, and reproducible on
mainwith no bounds and pruning off — a plain text/keyword conflict pluschart count() over ts by env:The warning listed every group key rather than the fields that could not be aggregated. That made it inaccurate —
tsis a date, aggregatable in both indices, and nothing about it needed fixing — and impossible to de-duplicate:chartraises this once per equivalent plan alternative, whichdrainWarnings()collapses by value and #5657 wrote it for exactly that, but the keys arrive in whatever order the alternative had them, so logically identical findings differed as strings and survived as separate banners.Naming only the offending fields makes those three identical, so the existing dedup collapses them to one, and the one that remains names what to fix. Sorted for the same reason the excluded-index list already is, so plan ordering cannot reach the text. Verified against a cluster: three banners before, one after,
[env]alone.Related Issues
Part of #5698. Builds on #5727; expects #5759.
Front-end counterpart: opensearch-project/OpenSearch-Dashboards#12722.
Check List
--signoffor-s.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Review notes
A self-review and a joint review against the Dashboards side turned up several defects, fixed in the later commits and worth recording:
_field_capsreports it as not-matching, indistinguishably from one whose values fall outside the range. A first attempt retained such indices via an extrainclude_unmappedprobe, but that regressed Avoid PIT context exhaustion by pruning indices that cannot match #5727's ownindex_pruning.ymlcase (on the pushdown path the range is the query's filter, so dropping the index is lossless there) and it left the partial-result feature something to exclude, producing a spurious "mapping conflict" banner on a field that was merely absent. Both paths now prune it, consistently.analyzedropped the bounds on both of its phases, so"analyze": truereported a plan over an index expression the execution would not have used.BaseRestHandlerrejects parameters absent fromresponseParams(), so?start_time=…was a 400, and a GET carries no body for the loop to write into. Removed rather than half-supported.createRoleWithIndexAccessgranted neitherindices:admin/resolve/indexnorindices:data/read/field_caps*, so both probes were denied and pruning declined silently. Granting them is load-bearing: taking them back out makes the positive case fail again, which is also the first direct evidence for the permission limitation the manual documents.falseon teardown, which since Enableplugins.query.pruning.enabledby default #5759 disables the feature for every later class sharing the cluster. They clear the override instead.