[dai-chen]

Description

This PR broadens index pruning coverage in two cases:

1. When a wildcard matches data streams, or a query names a data stream explicitly, pruning selects matching backing indices as it does regular indices.
2. When no time field is explicitly supplied for query-filter pruning, a range on @timestamp, timestamp, or time can trigger pruning (previously, only @timestamp).

Original PRs for reference:

• Index resolving stage: Prune indices by request-level time bounds before the schema is resolved #5766
• PIT stage: Avoid PIT context exhaustion by pruning indices that cannot match #5727

Implementation notes

• The probe timeout increases from 10 to 30 seconds to allow for occasional Field Caps delays during cross-cluster searches.
• When request time bounds successfully narrow the expression to concrete indices, the later pruner call performs a quick Resolve Index probe and exits without repeating Field Caps.

Related Issues

Resolves #5906

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.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit e8e09e0)

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 Possible Issue The deleteRemote method silently swallows all non-404 exceptions after rethrowing them. The finally block in tearDownDataStreams calls deleteRemote twice, but if the first call throws a non-404 exception, the second call never executes, leaving the index template behind. This can cause test pollution if the template remains in the cluster. private void deleteRemote(String path) throws IOException { try { remoteClient().performRequest(new Request("DELETE", path)); } catch (ResponseException e) { if (e.getResponse().getStatusLine().getStatusCode() != 404) { throw e; } } } Possible Issue findTimeRangeField returns the first matching time field from a boolean query's clauses without checking if multiple different time fields are present. If a query contains ranges on both @timestamp and time, the method returns whichever it encounters first, potentially selecting a field that does not match the majority of the data or the user's intent. This could lead to incorrect pruning decisions. static Optional<String> findTimeRangeField(QueryBuilder query, Set<String> candidateTimeFields) { if (query instanceof RangeQueryBuilder range) { String field = range.fieldName(); return candidateTimeFields.contains(field) ? Optional.of(field) : Optional.empty(); } if (query instanceof BoolQueryBuilder bool) { return Stream.of(bool.must(), bool.filter(), bool.should()) .flatMap(List::stream) .flatMap(clause -> findTimeRangeField(clause, candidateTimeFields).stream()) .findFirst(); } if (query instanceof ConstantScoreQueryBuilder constantScore) { return findTimeRangeField(constantScore.innerQuery(), candidateTimeFields); } return Optional.empty(); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to e8e09e0

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Review increased probe timeout duration The probe timeout was increased from 10 to 30 seconds. This is a significant increase that could impact query latency. Consider if this timeout is appropriate for production environments, especially when pruning fails and the query must wait for the timeout before falling back to the full expression. opensearch/src/main/java/org/opensearch/sql/opensearch/request/IndexPruner.java [39] -private static final TimeValue PROBE_TIMEOUT = TimeValue.timeValueSeconds(30); +private static final TimeValue PROBE_TIMEOUT = TimeValue.timeValueSeconds(10); Suggestion importance[1-10]: 4 __ Why: The timeout increase from 10 to 30 seconds is a valid concern for production environments. However, this is a configuration change that may be intentional to reduce fallback failures. The suggestion asks for verification rather than identifying a clear bug.	Low

---

Previous suggestions

Suggestions up to commit 8a0316d

Category	Suggestion	Impact
General	Clarify safe Optional access pattern The timeField.orElseThrow() call will throw NoSuchElementException if empty, but this should never happen due to the isEmpty() check above. However, this creates a subtle dependency between the check and usage. Consider using timeField.get() with a comment, or restructure to make the flow clearer. opensearch/src/main/java/org/opensearch/sql/opensearch/request/IndexPruner.java [74-81] Optional<String> timeField = findTimeRangeField(filter, candidateTimeFields); IndexExpression indexExpr = new IndexExpression(indexName, node); if (timeField.isEmpty() || !isPrunable(indexExpr)) { log.info("Index pruning skipped: {}", indexExpr); return indexName; } -String[] candidates = indexExpr.probeMatching(filter, timeField.orElseThrow()).getIndices(); +// timeField is guaranteed present by isEmpty() check above +String[] candidates = indexExpr.probeMatching(filter, timeField.get()).getIndices(); Suggestion importance[1-10]: 3 __ Why: While the suggestion correctly identifies that timeField.orElseThrow() is safe due to the isEmpty() check, changing to get() with a comment is a minor stylistic improvement that slightly enhances code clarity but has minimal impact on functionality.	Low

Suggestions up to commit 2eda651

Category	Suggestion	Impact
General	Verify behavior when no indices match When candidates.length is 0, the method returns the original indexName, but this could lead to querying all indices when none match the time range. Verify that returning the full index expression is the intended behavior when no indices match, as this might cause unnecessary load or incorrect results. opensearch/src/main/java/org/opensearch/sql/opensearch/request/IndexPruner.java [81-85] -private IndexName doPrune( - IndexName indexName, QueryBuilder filter, Set<String> candidateTimeFields) { - try { - Optional<String> timeField = findTimeRangeField(filter, candidateTimeFields); - IndexExpression indexExpr = new IndexExpression(indexName, node); - if (timeField.isEmpty() || !isPrunable(indexExpr)) { - log.info("Index pruning skipped: {}", indexExpr); - return indexName; - } +String[] candidates = indexExpr.probeMatching(filter, timeField.orElseThrow()).getIndices(); +if (candidates.length > 0) { + return new IndexName(String.join(",", candidates)); +} +log.warn("Index pruning found no matching indices for time range; querying full expression"); - String[] candidates = indexExpr.probeMatching(filter, timeField.orElseThrow()).getIndices(); - if (candidates.length > 0) { - return new IndexName(String.join(",", candidates)); - } - log.info("Index pruning declined: field caps returned no candidate indices"); - Suggestion importance[1-10]: 7 __ Why: Valid concern about returning the full indexName when no candidates match. The suggestion to change log level to warn is reasonable, though the behavior of falling back to the full expression appears intentional based on the code comment at line 49-50.	Medium
	Review increased probe timeout duration The probe timeout was increased from 10 to 30 seconds, which is very generous for a synchronous operation. Consider if this timeout is appropriate for production environments where query latency is critical, as it could significantly delay query responses when probes fail or are slow. opensearch/src/main/java/org/opensearch/sql/opensearch/request/IndexPruner.java [39] -private static final TimeValue PROBE_TIMEOUT = TimeValue.timeValueSeconds(30); +private static final TimeValue PROBE_TIMEOUT = TimeValue.timeValueSeconds(10); Suggestion importance[1-10]: 5 __ Why: The timeout increase from 10 to 30 seconds is a valid concern for production latency, but the PR description states this is "generous because a fallback can fail the query." The suggestion to revert is questionable without understanding the specific requirements that drove this change.	Low
	Add logging for ignored 404 errors The deleteRemote method silently swallows 404 errors, which could mask issues during test cleanup. Consider logging when a 404 occurs to aid debugging, especially since this is used in cleanup operations where knowing what was or wasn't present can be valuable. integ-test/src/test/java/org/opensearch/sql/security/CrossClusterIndexPruningIT.java [186-194] private void deleteRemote(String path) throws IOException { try { remoteClient().performRequest(new Request("DELETE", path)); } catch (ResponseException e) { if (e.getResponse().getStatusLine().getStatusCode() != 404) { throw e; } + log.debug("Resource not found during cleanup: {}", path); } } Suggestion importance[1-10]: 4 __ Why: Adding debug logging for 404 errors during cleanup could help with test debugging, but this is a minor improvement. The current behavior of silently ignoring 404s is standard for cleanup operations in tests.	Low

Suggestions up to commit 3c3f6d4

Category	Suggestion	Impact
Possible issue	Add null check for query parameter Add null safety check for the query parameter before processing. If query is null, the method should return Optional.empty() immediately to prevent potential NullPointerException when accessing query properties. opensearch/src/main/java/org/opensearch/sql/opensearch/request/IndexPruner.java [98-113] private static Optional<String> findTimeRangeField(QueryBuilder query, Set<String> candidateTimeFields) { + if (query == null) { + return Optional.empty(); + } if (query instanceof RangeQueryBuilder range) { String field = range.fieldName(); return candidateTimeFields.contains(field) ? Optional.of(field) : Optional.empty(); } if (query instanceof BoolQueryBuilder bool) { return Stream.of(bool.must(), bool.filter(), bool.should()) .flatMap(List::stream) .flatMap(clause -> findTimeRangeField(clause, candidateTimeFields).stream()) .findFirst(); } if (query instanceof ConstantScoreQueryBuilder constantScore) { return findTimeRangeField(constantScore.innerQuery(), candidateTimeFields); } return Optional.empty(); } Suggestion importance[1-10]: 7 __ Why: Adding a null check for the query parameter is a valid defensive programming practice. The method is called from doPrune where filter can be null (as seen in test cases), so this check prevents potential NullPointerException when processing query types.	Medium
General	Validate candidate time fields parameter Validate that candidateTimeFields is not null or empty before calling findTimeRangeField. An empty or null set would cause findTimeRangeField to never find a match, leading to unnecessary index expression resolution and potentially misleading log messages. opensearch/src/main/java/org/opensearch/sql/opensearch/request/IndexPruner.java [71-90] private IndexName doPrune( IndexName indexName, QueryBuilder filter, Set<String> candidateTimeFields) { try { + if (candidateTimeFields == null || candidateTimeFields.isEmpty()) { + log.info("Index pruning skipped: no candidate time fields provided"); + return indexName; + } Optional<String> timeField = findTimeRangeField(filter, candidateTimeFields); IndexExpression indexExpr = new IndexExpression(indexName, node); if (timeField.isEmpty() || !isPrunable(indexExpr)) { log.info("Index pruning skipped: {}", indexExpr); return indexName; } String[] candidates = indexExpr.probeMatching(filter, timeField.orElseThrow()).getIndices(); if (candidates.length > 0) { return new IndexName(String.join(",", candidates)); } log.info("Index pruning declined: field caps returned no candidate indices"); } catch (Exception e) { log.warn("Index pruning failed; querying the full index expression", e); } return indexName; } Suggestion importance[1-10]: 5 __ Why: While validating candidateTimeFields for null/empty is reasonable defensive programming, the current code already handles empty results from findTimeRangeField via timeField.isEmpty() check. The suggestion adds an early exit that improves clarity but doesn't fix a critical issue since the method is called internally with controlled sets like IMPLICIT_TIME_FIELDS and Set.of(timeField).	Low

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.38%. Comparing base (665e1b2) to head (e8e09e0).

❌ Your project check has failed because the head coverage (75.38%) 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    #5908   +/-   ##
=========================================
  Coverage     75.38%   75.38%           
- Complexity    10385    10386    +1     
=========================================
  Files           962      962           
  Lines         41087    41089    +2     
  Branches       4682     4681    -1     
=========================================
+ Hits          30973    30975    +2     
  Misses         8806     8806           
  Partials       1308     1308           

Flag	Coverage Δ
sql-engine	75.38% <100.00%> (+<0.01%)	⬆️

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.

[github-actions]

Persistent review updated to latest commit 2eda651

[github-actions]

Persistent review updated to latest commit 8a0316d

[github-actions]

Persistent review updated to latest commit e8e09e0

[dai-chen]

I removed this check because it's complicated with backing indices in consideration. In common case field caps narrows a wildcard to a small subset, so checking for the rare no-reduction case adds little value.