Skip to content

Prune indices by request-level time bounds before the schema is resolved - #5766

Open
ahkcs wants to merge 23 commits into
opensearch-project:mainfrom
ahkcs:feat/time-range-index-pruning
Open

ahkcs wants to merge 23 commits into
opensearch-project:mainfrom
ahkcs:feat/time-range-index-pruning

Conversation

@ahkcs

@ahkcs ahkcs commented Sep 14, 2026

Copy link
Copy Markdown
Collaborator

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:

start_time / end_time / time_field
  → PPLQueryRequest.getTimeBounds
  → AbstractPlan → QueryService.buildFrameworkConfig   (seeded once)
  → OpenSearchSchema.registerTable
  → OpenSearchStorageEngine.getTable(.., TimeBounds)
  → IndexPruner → OpenSearchIndex                      (constructed pre-narrowed)

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.

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 uses timestamp — and would otherwise never prune. Defaults to @timestamp, so a request that says nothing behaves as designed.

An index that does not map time_field is 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.yml asserts 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 with Field [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 is strict_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.cluster was an object holding name, 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}
[INFO ][o.o.s.o.s.OpenSearchStorageEngine] Pruned index expression from prune-demo-2026.* to prune-demo-2026.09

A wider picker range prunes proportionally rather than all-or-nothing — prune-demo-2026.* → .07,.08,.09 for two months — and a range no index can match declines rather than pruning everything:

[INFO ][o.o.s.o.r.IndexPruner] Index pruning declined: 0 of 8 indices matched

Testing the changes

Suite Result
core + ppl + opensearch unit 9119 passed / 0 failed
CalciteTimeBoundsPruningIT (new) 9 passed (18 across both pushdown configurations)
CalciteExplainIT 288 passed
CalcitePPLBasicIT 94 passed
yamlRestTest (incl. #5727's index_pruning.yml) 34 passed
TimeBoundsPruningSecurityIT (new) 5 passed

The explain and basic suites are regression cover for the QueryService/QueryPlan/ExplainPlan signature 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 ThreadContext to drop. TimeBoundsPruningSecurityIT covers it; note integTestWithSecurity cannot run locally at the moment because today's snapshot distro ships jackson-core-2.22.1 while the security plugin's own zip carries 2.22.2 (the existing PartialResultSecurityIT fails 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:

  • No new endpoint. POST /_plugins/_ppl is unchanged.
  • No transport or BWC work. The body already travels as one JSON string in TransportPPLQueryRequest.writeTo, so there is no StreamOutput change and no version gate. fetch_size and partial_result are the precedents for a planning-affecting body field read the same way.
  • Reading them is additive. A request that omits them behaves exactly as before, and an older cluster ignores them, so the Dashboards side can ship independently.

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 SupportsIndexPruning to core's storage package. That is plugin-internal, optional, and implemented only by OpenSearchStorageEngine; other engines are untouched.

Files touched

File Change
core/.../executor/TimeBounds.java new — the request-level window, kept as the strings the request sent
core/.../storage/SupportsIndexPruning.java new — narrow interface so core needs no OpenSearch types and other engines are unaffected
core/.../calcite/OpenSearchSchema.java seeded with the bounds; offers them to every table it resolves
core/.../executor/QueryService.java threads the bounds to buildFrameworkConfig, on the execute, explain and analyze paths
core/.../execution/{AbstractPlan,QueryPlan,ExplainPlan,AnalyzePlan}.java carries them from the request to the worker thread
ppl/.../domain/PPLQueryRequest.java reads start_time / end_time / time_field off the body
ppl/.../PPLService.java sets them on the plan
opensearch/.../storage/OpenSearchStorageEngine.java implements SupportsIndexPruning; builds the table already narrowed
opensearch/.../request/IndexPruner.java bounds-driven overload beside #5727's filter-driven one
opensearch/.../scan/PartialResultAggregatePushdown.java warning names only the conflicting fields (see below)
docs/user/admin/settings.rst the parameters, their scope, and what they do and do not guarantee

Also fixed here: three partial-result banners for one finding

Surfaced by this work but independent of it, and reproducible on main with no bounds and pruning off — a plain text/keyword conflict plus chart count() over ts by env:

Results exclude 1 of 2 indices due to a mapping conflict on [env, ts].
Results exclude 1 of 2 indices due to a mapping conflict on [ts, env].
Results exclude 1 of 2 indices due to a mapping conflict on [env].

The warning listed every group key rather than the fields that could not be aggregated. That made it inaccurate — ts is a date, aggregatable in both indices, and nothing about it needed fixing — and impossible to de-duplicate: chart raises this once per equivalent plan alternative, which drainWarnings() 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

  • New functionality includes testing.
  • New functionality has been documented.
  • New functionality has javadoc added.
  • New functionality has a user manual doc added.
  • New PPL command checklist all confirmed.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff or -s.
  • Public documentation issue/PR created.

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:

  1. An index that does not map the time field. _field_caps reports it as not-matching, indistinguishably from one whose values fall outside the range. A first attempt retained such indices via an extra include_unmapped probe, but that regressed Avoid PIT context exhaustion by pruning indices that cannot match #5727's own index_pruning.yml case (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.
  2. analyze dropped the bounds on both of its phases, so "analyze": true reported a plan over an index expression the execution would not have used.
  3. URL parameters could not work. BaseRestHandler rejects parameters absent from responseParams(), so ?start_time=… was a 400, and a GET carries no body for the loop to write into. Removed rather than half-supported.
  4. The security IT could not pass. createRoleWithIndexAccess granted neither indices:admin/resolve/index nor indices: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.
  5. Both ITs pinned the pruning setting to false on teardown, which since Enable plugins.query.pruning.enabled by default #5759 disables the feature for every later class sharing the cluster. They clear the override instead.
  6. The probe's format list was needed for a bound spelled as a UTC wall clock — the spelling a client that also writes it into the query text produces. Without it the probe throws, the exception is swallowed, and pruning silently declines; two ITs failed with absence as their only symptom.

@github-actions

github-actions Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit 7a3d041)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review

Thread-local leak

chartPlanned is initialized with withInitial(() -> false) but cleared with set(false) instead of remove(). On a pooled thread, a subsequent query that does not chart will still see true if the previous query on that thread did chart, because set(false) leaves the ThreadLocal entry in place. The test at line 44 verifies the flag does not leak, but it calls run() which invokes remove() via clearTimewrapSignals(). A query executed outside run() or one that bypasses clearTimewrapSignals() would leak the flag.

private static final ThreadLocal<Boolean> chartPlanned = ThreadLocal.withInitial(() -> false);
Possible Issue

The method isPrunable at line 84 checks hasTimeRange before hasAlias and hasDataStream, with a comment stating the latter two resolve the expression. However, containsTimeRange is a pure predicate on the query structure and does not resolve anything. If the intent was to short-circuit expensive resolution calls, the order is correct, but the comment is misleading. If hasTimeRange was supposed to be checked last (as the old code did), the reordering introduces a logic error where expressions with a time range but no wildcard are now checked for alias/datastream unnecessarily.

private static boolean isPrunable(IndexExpression expression, boolean hasTimeRange) {
  return expression.hasWildcard()
      && hasTimeRange
      // Last: these resolve the expression. An alias may carry a filter that substituting its
      // concrete indices would drop.
      && !expression.hasAlias()
      && !expression.hasDataStream();
Possible Issue

The prune method at line 80 returns name unchanged when bounds is null or pruning is disabled. However, if getNodeClient() returns empty (no local node client available), it also returns name unchanged via orElse(name). This is silent: a cluster where the node client is unavailable will never prune, and no log or warning indicates why. If the unavailability is transient or a misconfiguration, queries will read more indices than intended without any signal to the operator.

private String prune(String name, @Nullable TimeBounds bounds) {
  if (bounds == null
      || !Boolean.TRUE.equals(settings.getSettingValue(Settings.Key.QUERY_PRUNING_ENABLED))) {
    return name;
  }
  return client
      .getNodeClient()
      .map(
          node -> {
            String pruned =
                new IndexPruner(node)
                    .prune(
                        new OpenSearchRequest.IndexName(name),
                        timeRangeQuery(bounds),
                        bounds.getTimeField())
                    .toString();
            if (!pruned.equals(name)) {
              log.info("Pruned index expression from {} to {}", name, pruned);
            }
            return pruned;
          })
      .orElse(name);
}

@github-actions

github-actions Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Latest suggestions up to 7a3d041

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Add exception handling for pruning fallback

The method should handle potential exceptions from getTable() calls to prevent
failures from propagating unchecked. If pruning fails, the system should fall back
to the unbounded resolution rather than failing the entire query.

core/src/main/java/org/opensearch/sql/calcite/OpenSearchSchema.java [64-72]

 private org.opensearch.sql.storage.Table resolve(
     StorageEngine engine,
     DataSourceSchemaName schemaName,
     DataSourceSchemaIdentifierNameResolver nameResolver) {
   if (timeBounds != null && engine instanceof SupportsIndexPruning pruning) {
-    return pruning.getTable(schemaName, nameResolver.getIdentifierName(), timeBounds);
+    try {
+      return pruning.getTable(schemaName, nameResolver.getIdentifierName(), timeBounds);
+    } catch (Exception e) {
+      // Fall back to unbounded resolution if pruning fails
+      return engine.getTable(schemaName, nameResolver.getIdentifierName());
+    }
   }
   return engine.getTable(schemaName, nameResolver.getIdentifierName());
 }
Suggestion importance[1-10]: 7

__

Why: Adding exception handling provides a safety net for pruning failures, ensuring the query falls back to unbounded resolution rather than failing entirely. This aligns with the PR's stated goal that "any failure while probing the cluster falls back to querying the full expression."

Medium
Validate time field before pruning

The method should validate that timeField is not null or blank before proceeding
with pruning logic. An invalid time field should cause pruning to be skipped rather
than potentially causing issues downstream in the probe.

opensearch/src/main/java/org/opensearch/sql/opensearch/request/IndexPruner.java [62-75]

 public IndexName prune(IndexName indexName, QueryBuilder filter, String timeField) {
+  if (timeField == null || timeField.isBlank()) {
+    log.info("Index pruning skipped: time field is null or blank");
+    return indexName;
+  }
   try {
     IndexExpression indexExpr = new IndexExpression(indexName, node);
     if (!isPrunable(indexExpr, containsTimeRange(filter, timeField))) {
       log.info("Index pruning skipped: {}", indexExpr);
       return indexName;
     }
 
     String[] candidates = indexExpr.probeMatching(filter, timeField).getIndices();
     if (0 < candidates.length && indexExpr.isPrunedBy(candidates.length)) {
       return new IndexName(String.join(",", candidates));
     }
     ...
Suggestion importance[1-10]: 6

__

Why: Validating timeField early prevents potential issues downstream and makes the pruning logic more robust. However, the containsTimeRange method already checks if the field matches, providing some implicit validation.

Low
Separate null and blank validation checks

The validation should check for null before calling isBlank() to avoid potential
NullPointerException. While the current code works due to short-circuit evaluation,
explicitly handling null first makes the intent clearer and more defensive.

core/src/main/java/org/opensearch/sql/executor/TimeBounds.java [33-38]

 private static String requireText(String value, String name) {
-  if (value == null || value.isBlank()) {
+  if (value == null) {
+    throw new IllegalArgumentException(String.format(Locale.ROOT, "%s must not be null", name));
+  }
+  if (value.isBlank()) {
     throw new IllegalArgumentException(String.format(Locale.ROOT, "%s must not be blank", name));
   }
   return value.trim();
 }
Suggestion importance[1-10]: 3

__

Why: While separating null and blank checks improves clarity, the existing code already handles both cases correctly through short-circuit evaluation. The improvement is marginal and primarily stylistic.

Low

Previous suggestions

Suggestions up to commit 9585c0a
CategorySuggestion                                                                                                                                    Impact
General
Fix validation inconsistency with trimming

The validation logic checks for blank strings but then trims the value, which could
result in an empty string being returned if the input was only whitespace. This
creates an inconsistency where a blank string passes validation but becomes empty
after trimming. Consider checking isBlank() after trimming to ensure consistency.

core/src/main/java/org/opensearch/sql/executor/TimeBounds.java [33-38]

 private static String requireText(String value, String name) {
-  if (value == null || value.isBlank()) {
+  if (value == null) {
     throw new IllegalArgumentException(String.format(Locale.ROOT, "%s must not be blank", name));
   }
-  return value.trim();
+  String trimmed = value.trim();
+  if (trimmed.isEmpty()) {
+    throw new IllegalArgumentException(String.format(Locale.ROOT, "%s must not be blank", name));
+  }
+  return trimmed;
 }
Suggestion importance[1-10]: 8

__

Why: The current implementation has a subtle bug: isBlank() returns true for whitespace-only strings, but after trimming, such a string becomes empty. The suggested fix properly validates after trimming, ensuring consistency and preventing edge cases where whitespace-only input could pass validation.

Medium
Prevent ThreadLocal memory leak

The chartPlanned ThreadLocal should be cleared in clearTimewrapSignals() using
remove() instead of set(false) to prevent memory leaks in thread pool environments.
When threads are reused, set(false) leaves the ThreadLocal entry in the thread's
map, while remove() cleans it up completely.

core/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.java [100]

-private static final ThreadLocal<Boolean> chartPlanned = ThreadLocal.withInitial(() -> false);
+private static final ThreadLocal<Boolean> chartPlanned = new ThreadLocal<>();
Suggestion importance[1-10]: 7

__

Why: Using ThreadLocal.withInitial() with set(false) in cleanup can leave entries in thread-local maps. However, the existing code at line 292 uses set(false) which is consistent with other ThreadLocals in the class. The suggestion to use new ThreadLocal<>() with remove() would be better, but the current approach is functional.

Medium
Suggestions up to commit 9585c0a
CategorySuggestion                                                                                                                                    Impact
General
Use remove() for ThreadLocal cleanup

The chartPlanned ThreadLocal should be removed in clearTimewrapSignals() instead of
reset to false. Using remove() ensures the ThreadLocal doesn't leak memory on pooled
threads, consistent with how partialResultOverride is handled.

core/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.java [292]

-private static final ThreadLocal<Boolean> chartPlanned = ThreadLocal.withInitial(() -> false);
+private static final ThreadLocal<Boolean> chartPlanned = new ThreadLocal<>();
 
+// In clearTimewrapSignals():
+chartPlanned.remove();
+
Suggestion importance[1-10]: 7

__

Why: Using remove() instead of set(false) is better for ThreadLocal cleanup on pooled threads to prevent memory leaks. This is consistent with how partialResultOverride is handled in the same method.

Medium
Validate probe response before pruning

The code retrieves the full FieldCapabilitiesResponse but only uses the indices
array. Consider checking if the response indicates any failures or issues before
proceeding with pruning, as the probe might have partially failed without throwing
an exception.

opensearch/src/main/java/org/opensearch/sql/opensearch/request/IndexPruner.java [70-72]

-String[] candidates = indexExpr.probeMatching(filter, timeField).getIndices();
+FieldCapabilitiesResponse response = indexExpr.probeMatching(filter, timeField);
+String[] candidates = response.getIndices();
 if (0 < candidates.length && indexExpr.isPrunedBy(candidates.length)) {
   return new IndexName(String.join(",", candidates));
 }
Suggestion importance[1-10]: 5

__

Why: Storing the FieldCapabilitiesResponse in a variable before extracting indices could allow for additional validation. However, the current code already handles failures via the try-catch block, and the suggestion doesn't specify what additional checks should be performed.

Low
Fix redundant blank check logic

The validation should check for blank strings before trimming, as isBlank() already
handles whitespace-only strings. Calling trim() after validation is redundant since
isBlank() returns true for strings that would become empty after trimming.

core/src/main/java/org/opensearch/sql/executor/TimeBounds.java [33-38]

 private static String requireText(String value, String name) {
-  if (value == null || value.isBlank()) {
+  if (value == null || value.trim().isEmpty()) {
     throw new IllegalArgumentException(String.format(Locale.ROOT, "%s must not be blank", name));
   }
   return value.trim();
 }
Suggestion importance[1-10]: 4

__

Why: The suggestion to use trim().isEmpty() instead of isBlank() is actually less clear. The current code using isBlank() is more readable and correct, as it handles null, empty, and whitespace-only strings appropriately before trimming.

Low
Suggestions up to commit a692a02
CategorySuggestion                                                                                                                                    Impact
General
Fix validation order for trimmed values

The validation logic should check for blank strings before trimming to avoid
potential issues where a string with only whitespace passes the null check but
becomes empty after trimming. Move the trim() call before the blank check.

core/src/main/java/org/opensearch/sql/executor/TimeBounds.java [33-38]

 private static String requireText(String value, String name) {
-  if (value == null || value.isBlank()) {
+  if (value == null) {
     throw new IllegalArgumentException(String.format(Locale.ROOT, "%s must not be blank", name));
   }
-  return value.trim();
+  String trimmed = value.trim();
+  if (trimmed.isBlank()) {
+    throw new IllegalArgumentException(String.format(Locale.ROOT, "%s must not be blank", name));
+  }
+  return trimmed;
 }
Suggestion importance[1-10]: 3

__

Why: The suggestion improves code clarity by separating the null check from the blank check, but the existing code is functionally correct since isBlank() already handles null values. The improvement is minor and mainly stylistic.

Low
Suggestions up to commit 145e40d
CategorySuggestion                                                                                                                                    Impact
General
Add exception handling for pruning fallback

The method should handle potential exceptions from getTable calls to prevent
failures from propagating unchecked. If pruning fails, it should fall back to the
unbounded resolution rather than failing the entire query.

core/src/main/java/org/opensearch/sql/calcite/OpenSearchSchema.java [64-72]

 private org.opensearch.sql.storage.Table resolve(
     StorageEngine engine,
     DataSourceSchemaName schemaName,
     DataSourceSchemaIdentifierNameResolver nameResolver) {
   if (timeBounds != null && engine instanceof SupportsIndexPruning pruning) {
-    return pruning.getTable(schemaName, nameResolver.getIdentifierName(), timeBounds);
+    try {
+      return pruning.getTable(schemaName, nameResolver.getIdentifierName(), timeBounds);
+    } catch (Exception e) {
+      // Fall back to unbounded resolution if pruning fails
+      return engine.getTable(schemaName, nameResolver.getIdentifierName());
+    }
   }
   return engine.getTable(schemaName, nameResolver.getIdentifierName());
 }
Suggestion importance[1-10]: 7

__

Why: Adding exception handling provides a safety net for pruning failures, ensuring the query falls back to unbounded resolution rather than failing entirely. This improves robustness, though the impact depends on how often pruning might throw exceptions in practice.

Medium
Add null check for candidates array

The code should verify that getIndices() returns a non-null array before accessing
its length. While the current implementation may guarantee non-null, defensive
programming prevents potential NullPointerException if the contract changes.

opensearch/src/main/java/org/opensearch/sql/opensearch/request/IndexPruner.java [70-73]

-String[] candidates = indexExpr.probeMatching(filter, timeField).getIndices();
-if (0 < candidates.length && indexExpr.isPrunedBy(candidates.length)) {
+FieldCapabilitiesResponse response = indexExpr.probeMatching(filter, timeField);
+String[] candidates = response.getIndices();
+if (candidates != null && 0 < candidates.length && indexExpr.isPrunedBy(candidates.length)) {
   return new IndexName(String.join(",", candidates));
 }
Suggestion importance[1-10]: 6

__

Why: Adding a null check for candidates is a defensive programming practice that prevents potential NullPointerException. While the current implementation may guarantee non-null, this check adds safety if the contract changes, though it's a minor improvement.

Low
Suggestions up to commit bb9c687
CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix return type mismatch in probe

The method signature changed to return FieldCapabilitiesResponse but the
implementation still calls .getIndices() on the response. This creates a type
mismatch where the method declares it returns FieldCapabilitiesResponse but actually
returns String[]. The caller expects the full response object.

opensearch/src/main/java/org/opensearch/sql/opensearch/request/IndexPruner.java [136-145]

-String[] probeMatching(QueryBuilder filter, String timeField) {
+FieldCapabilitiesResponse probeMatching(QueryBuilder filter, String timeField) {
   FieldCapabilitiesRequest request =
       new FieldCapabilitiesRequest()
           .indices(indexName.getIndexNames())
           .fields(timeField)
           .indexFilter(filter)
           .indicesOptions(DEFAULT_INDICES_OPTIONS);
-  return node.fieldCaps(request).actionGet(PROBE_TIMEOUT).getIndices();
+  return node.fieldCaps(request).actionGet(PROBE_TIMEOUT);
 }
Suggestion importance[1-10]: 10

__

Why: Critical bug: the method signature declares FieldCapabilitiesResponse as return type but the implementation calls .getIndices() which returns String[]. This is a compilation error that would prevent the code from building. The fix correctly removes the .getIndices() call to match the declared return type.

High

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>
@ahkcs
ahkcs force-pushed the feat/time-range-index-pruning branch from c16b768 to c90cee5 Compare September 14, 2026 17:24
@github-actions

Copy link
Copy Markdown
Contributor

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>
@github-actions

Copy link
Copy Markdown
Contributor

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>
@github-actions

Copy link
Copy Markdown
Contributor

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>
@ahkcs ahkcs added the enhancement New feature or request label Sep 14, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit c792001

@codecov

codecov Bot commented Sep 14, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 63.63636% with 40 lines in your changes missing coverage. Please review.
✅ Project coverage is 62.88%. Comparing base (6116c33) to head (7a3d041).
⚠️ Report is 1658 commits behind head on main.

Files with missing lines Patch % Lines
...ql/opensearch/storage/OpenSearchStorageEngine.java 36.36% 13 Missing and 1 partial ⚠️
...java/org/opensearch/sql/executor/QueryService.java 26.66% 11 Missing ⚠️
...org/opensearch/sql/calcite/CalcitePlanContext.java 50.00% 5 Missing ⚠️
...c/main/java/org/opensearch/sql/ppl/PPLService.java 20.00% 4 Missing ⚠️
...a/org/opensearch/sql/calcite/OpenSearchSchema.java 85.71% 2 Missing ⚠️
...ensearch/storage/scan/CalciteLogicalIndexScan.java 0.00% 2 Missing ⚠️
.../opensearch/sql/calcite/CalciteRelNodeVisitor.java 0.00% 1 Missing ⚠️
...opensearch/sql/executor/execution/AnalyzePlan.java 0.00% 1 Missing ⚠️

❌ 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.

❗ There is a different number of reports uploaded between BASE (6116c33) and HEAD (7a3d041). Click for more details.

HEAD has 4 uploads less than BASE
Flag BASE (6116c33) HEAD (7a3d041)
sql-engine 6 2
Additional details and impacted files
@@              Coverage Diff              @@
##               main    #5766       +/-   ##
=============================================
- Coverage     98.40%   62.88%   -35.52%     
- Complexity     2746     8802     +6056     
=============================================
  Files           266      938      +672     
  Lines          6758    40198    +33440     
  Branches        426     4520     +4094     
=============================================
+ Hits           6650    25278    +18628     
- Misses          107    14119    +14012     
- Partials          1      801      +800     
Flag Coverage Δ
sql-engine 62.88% <63.63%> (-35.52%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@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>
@github-actions

Copy link
Copy Markdown
Contributor

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>
@github-actions

github-actions Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit a692a02.

PathLineSeverityDescription
opensearch/src/main/java/org/opensearch/sql/opensearch/request/IndexPruner.java147mediumUser-controlled `time_field` from the request body is passed directly as the `fields()` argument to `FieldCapabilitiesRequest`. This creates a side-channel: an attacker can cycle through arbitrary field names and infer which indices contain each field by observing which indices survive pruning and appear in results or errors. No validation restricts what field names can be probed.
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteTimeBoundsPruningIT.java190lowThe test `shouldPruneAnIndexThatDoesNotMapTheTimeField` confirms that an index not mapping the user-supplied `time_field` is silently excluded from results. Combined with the user-controlled field name, a caller can exclude arbitrary indices from query results by naming a field those indices don't map, with no warning returned to the user.
ppl/src/main/java/org/opensearch/sql/ppl/domain/PPLQueryRequest.java213lowException message from `TimeBounds` constructor is logged via `LOG.warn("Ignoring unusable time bounds: {}", e.getMessage())`. If a future change adds user-controlled data to the exception message path (e.g. in `requireText`), it would flow into log output, enabling log injection. Currently limited to static format strings but the pattern is fragile.

The table above displays the top 10 most important findings.

Total: 3 | Critical: 0 | High: 0 | Medium: 1 | Low: 2


Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.


⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

@github-actions

Copy link
Copy Markdown
Contributor

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>
@github-actions

Copy link
Copy Markdown
Contributor

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>
@github-actions

Copy link
Copy Markdown
Contributor

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>
@github-actions

Copy link
Copy Markdown
Contributor

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>
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit abd9510

Signed-off-by: Kai Huang <ahkcs@amazon.com>
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 336be69

Rather than a TimeBounds-shaped entry point on IndexPruner, the caller
builds the range query and calls prune with the field it ranges on. The
existing two-argument overload keeps its @timestamp default, so the
pushdown path is unchanged, and IndexPruner no longer knows about
TimeBounds.

Signed-off-by: Kai Huang <ahkcs@amazon.com>
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit d2840a5

Signed-off-by: Kai Huang <ahkcs@amazon.com>
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit bb9c687

Signed-off-by: Kai Huang <ahkcs@amazon.com>
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 145e40d

Signed-off-by: Kai Huang <ahkcs@amazon.com>
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit a692a02

Signed-off-by: Kai Huang <ahkcs@amazon.com>
Signed-off-by: Kai Huang <ahkcs@amazon.com>
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 0f0e4f3

@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 9585c0a

Signed-off-by: Kai Huang <ahkcs@amazon.com>
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 7a3d041

@ahkcs
ahkcs requested a review from penghuo September 15, 2026 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants