Add can_match summary to profile=true for SHARD_FRAGMENT stages

[finnegancarroll]

Adding support for profile=true for the can_match stage. All metrics are already computed and surfaced on the coordinator.

A can_match block is attached to each SHARD_FRAGMENT stage in the profile response, present only when the can_match phase actually ran (query had range filters or a bounded-field sort, and the fan-out cleared the pre-filter threshold).

Fields

Field	Meaning
can_match_ms	Wall-clock latency of the parallel can_match probe round-trip
total_shards	Shard targets considered before can_match ran
shards_pruned_by_filter	Shards dropped pre-dispatch because parquet row-group stats proved their range disjoint from the query filters
shards_skipped_by_topn	Shards skipped during staggered dispatch because their folded sort bounds could not beat the top-N bar (`sort
topn_gate_armed	Whether the top-N gate reached its limit K (dynamic skipping was active)
shards_dispatched	Shards that actually ran a fragment (total_shards - pruned - skipped)

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 1b0b3d8)

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 negative dispatched count canMatchProfile() computes dispatched = canMatchTotalShards - canMatchPrunedByFilter - skipped. The canMatchSkippedByTopN counter is incremented from ShardTaskRunner during staggered dispatch, but shards can only be top-N-skipped from the set that survived pruning. If any accounting drift occurs (e.g., a shard counted both as pruned and skipped, or skips recorded for targets not included in canMatchTotalShards), dispatched could become negative and be reported to users. Consider clamping to 0 or asserting the invariant, since the field is user-visible and expected to be non-negative. public CanMatchProfile canMatchProfile() { if (canMatchRan == false) { return null; } int skipped = canMatchSkippedByTopN.get(); boolean armed = topNGate != null && topNGate.isArmed(); int dispatched = canMatchTotalShards - canMatchPrunedByFilter - skipped; return new CanMatchProfile(canMatchMs, canMatchTotalShards, canMatchPrunedByFilter, skipped, armed, dispatched); } Static state across tests indicesCsv and provisioned are static fields but ensureProvisioned() is an instance method. In JUnit test lifecycles where the class is reloaded per test run this is fine, but the static flag will persist across tests within the same JVM and index deletion is only attempted inside createDailyIndices() (guarded by !provisioned). If a prior test run left indices behind but the static flag was reset (e.g., JVM restart between suites is not guaranteed in Gradle test workers), stale data won't be cleaned up. Prefer @BeforeClass provisioning or explicit setup to make lifecycle intent unambiguous. private static String indicesCsv; // comma-list of the daily indices, provisioned once private static boolean provisioned = false; private String ensureProvisioned() throws IOException { if (!provisioned) { indicesCsv = createDailyIndices(); provisioned = true; } return indicesCsv; }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 1b0b3d8

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Make provisioning resilient to cluster resets Using static mutable state for provisioning across test methods is fragile: JUnit test classes may be re-instantiated per method, and if the cluster is torn down between test runs (or indices are wiped in @Before/@After inherited from the base class), provisioned=true will incorrectly short-circuit re-creation, causing tests to fail with missing indices. Consider using @BeforeClass or checking for index existence before skipping creation. sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/CanMatchProfileIT.java [47-56] private static String indicesCsv; // comma-list of the daily indices, provisioned once -private static boolean provisioned = false; private String ensureProvisioned() throws IOException { - if (!provisioned) { + if (indicesCsv == null || indexMissing()) { indicesCsv = createDailyIndices(); - provisioned = true; } return indicesCsv; } Suggestion importance[1-10]: 4 __ Why: The concern about static state fragility is valid in general, but the improved_code references an undefined indexMissing() method and doesn't actually resolve the issue. The suggestion identifies a real risk but does not provide a working fix.	Low
	Guard against negative dispatched count The dispatched computation can go negative if recordTopNSkip() is invoked for shards that were already pruned by the filter, or due to any accounting mismatch. Clamp to zero to avoid emitting a negative count in the profile, which would violate the invariants asserted in the IT (dispatched = total - pruned - skipped, skipped >= 0). sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/stage/shard/ShardFragmentStageExecution.java [243-251] public CanMatchProfile canMatchProfile() { if (canMatchRan == false) { return null; } int skipped = canMatchSkippedByTopN.get(); boolean armed = topNGate != null && topNGate.isArmed(); - int dispatched = canMatchTotalShards - canMatchPrunedByFilter - skipped; + int dispatched = Math.max(0, canMatchTotalShards - canMatchPrunedByFilter - skipped); return new CanMatchProfile(canMatchMs, canMatchTotalShards, canMatchPrunedByFilter, skipped, armed, dispatched); } Suggestion importance[1-10]: 3 __ Why: Clamping to zero is a defensive measure, but the counts should be internally consistent by design (pruned and skipped are mutually exclusive shard sets). The suggestion masks bugs rather than fixing them, and the invariant is asserted in the IT, so a real accounting issue would be better surfaced.	Low

---

Previous suggestions

Suggestions up to commit d03a2a0

Category	Suggestion	Impact
General	Make test provisioning memoization more robust Using static mutable state to memoize provisioning across test methods is fragile: JUnit may instantiate tests per-method, and if the cluster is torn down or reset between runs (or tests run in a different JVM), provisioned=true will incorrectly skip re-creating indices. Consider using @BeforeClass setup or verifying index existence before skipping creation. sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/CanMatchProfileIT.java [47-56] -private static String indicesCsv; // comma-list of the daily indices, provisioned once -private static boolean provisioned = false; +private static String indicesCsv; // comma-list of the daily indices, provisioned once per JVM private String ensureProvisioned() throws IOException { - if (!provisioned) { + if (indicesCsv == null) { indicesCsv = createDailyIndices(); - provisioned = true; } return indicesCsv; } Suggestion importance[1-10]: 4 __ Why: The suggestion simplifies the memoization by using indicesCsv == null instead of a separate boolean flag, a minor readability improvement. It does not actually solve the raised concern about cluster teardown between test runs.	Low
	Prevent negative dispatched shard count The dispatched calculation can go negative if skipped accrues on shards that were already counted as pruned or if the accounting drifts. Clamp dispatched to a non-negative value to avoid exposing nonsensical negative counts in the profile output, which would also break the assertConsistent invariant in the IT. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/stage/shard/ShardFragmentStageExecution.java [243-251] public CanMatchProfile canMatchProfile() { if (canMatchRan == false) { return null; } int skipped = canMatchSkippedByTopN.get(); boolean armed = topNGate != null && topNGate.isArmed(); - int dispatched = canMatchTotalShards - canMatchPrunedByFilter - skipped; + int dispatched = Math.max(0, canMatchTotalShards - canMatchPrunedByFilter - skipped); return new CanMatchProfile(canMatchMs, canMatchTotalShards, canMatchPrunedByFilter, skipped, armed, dispatched); } Suggestion importance[1-10]: 3 __ Why: Clamping dispatched to non-negative is a minor defensive measure; in practice pruned and skipped are disjoint sets, so the value should not go negative. Low impact.	Low

Suggestions up to commit fc209dc

Category	Suggestion	Impact
General	Guarantee cluster setting cleanup between tests testNoCanMatchWhenNoFilterOrSort runs without a try/finally guard around cluster settings; if the previous test's resetSetting fails (network/error), the transient pre_filter_shard_size=1 will leak into the second test and cause can_match to fire, invalidating the assertion assertNull(...). Consider resetting the setting in a @After/tearDown hook to guarantee isolation. sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/CanMatchProfileIT.java [48-77] applySetting(PRE_FILTER_SETTING, "1"); try { Map<String, Object> result = executeWithProfile("source = " + INDEX + " | sort ts | fields host | head 3"); ... assertTrue("counts must not exceed total. can_match: " + canMatch, pruned + skipped <= total); } finally { resetSetting(PRE_FILTER_SETTING); } +// Additionally add: +// @Override public void tearDown() throws Exception { resetSetting(PRE_FILTER_SETTING); super.tearDown(); } Suggestion importance[1-10]: 5 __ Why: Adding a tearDown reset provides stronger test isolation against leaked cluster settings, which is a reasonable robustness improvement for IT reliability.	Low
	Prevent negative dispatched shard count dispatched can go negative if recordTopNSkip() is invoked for shards that were already pruned by the filter, or if counters race during snapshot. Clamp the value to 0 to guarantee the invariant dispatched >= 0 that the IT asserts and downstream consumers expect. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/stage/shard/ShardFragmentStageExecution.java [243-251] public CanMatchProfile canMatchProfile() { if (canMatchRan == false) { return null; } int skipped = canMatchSkippedByTopN.get(); boolean armed = topNGate != null && topNGate.isArmed(); - int dispatched = canMatchTotalShards - canMatchPrunedByFilter - skipped; + int dispatched = Math.max(0, canMatchTotalShards - canMatchPrunedByFilter - skipped); return new CanMatchProfile(canMatchMs, canMatchTotalShards, canMatchPrunedByFilter, skipped, armed, dispatched); } Suggestion importance[1-10]: 4 __ Why: The clamp is a defensive measure; in the current design recordTopNSkip and pruning are meant to be disjoint sets, so negative values shouldn't normally occur. Still, it's a reasonable safeguard for the invariant asserted by the IT.	Low
	Avoid static state for test provisioning Using a static mutable provisioned flag makes tests order-dependent and fragile across test-runner instances or reruns where the cluster state may not persist. Either recreate the index per test, or check for existence via a HEAD request rather than relying on a static boolean. sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/CanMatchProfileIT.java [30-38] -private static boolean provisioned = false; - private void ensureProvisioned() throws IOException { - if (!provisioned) { + Response head; + try { + head = client().performRequest(new Request("HEAD", "/" + INDEX)); + } catch (Exception e) { + head = null; + } + if (head == null || head.getStatusLine().getStatusCode() != 200) { createIndex(); indexData(); - provisioned = true; } } Suggestion importance[1-10]: 3 __ Why: Using a static provisioned flag can be fragile across test reruns, but this is a minor test-quality improvement rather than a correctness fix.	Low

Suggestions up to commit 8fa9abd

Category	Suggestion	Impact
Possible issue	Fix volatile publication ordering Setting canMatchRan = true before the numeric fields is a publication hazard: a reader on another thread (the profile snapshot) that sees the volatile canMatchRan may still observe stale/default values for the non-final fields written afterward. Reorder so canMatchRan is assigned last, ensuring the happens-before edge via the volatile write covers all the value fields. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/stage/shard/ShardFragmentStageExecution.java [296-299] -this.canMatchRan = true; this.canMatchMs = elapsed; this.canMatchTotalShards = targets.size(); this.canMatchPrunedByFilter = targets.size() - checked.targets().size(); +this.canMatchRan = true; Suggestion importance[1-10]: 7 __ Why: Correct observation about volatile publication semantics: setting the canMatchRan flag last ensures its volatile write establishes a happens-before relationship covering the other field writes, preventing readers from observing stale values.	Medium
General	Make test provisioning idempotent and robust A static provisioning flag persists across test class instances and JVM reuse in Gradle test workers, but the index itself may be wiped between runs (e.g., by test cleanup), leading to "index not found" failures on the second test. Either drop the static flag and always provision (createIndex already deletes first), or key provisioning off an actual index-exists probe. sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/CanMatchProfileIT.java [30-38] -private static boolean provisioned = false; - private void ensureProvisioned() throws IOException { - if (!provisioned) { + Response head; + try { + head = client().performRequest(new Request("HEAD", "/" + INDEX)); + } catch (Exception e) { + head = null; + } + if (head == null || head.getStatusLine().getStatusCode() != 200) { createIndex(); indexData(); - provisioned = true; } } Suggestion importance[1-10]: 5 __ Why: Valid concern: a static provisioned flag can survive across test worker JVM reuse while the index may be cleaned up, causing flaky failures. Making provisioning idempotent improves test reliability.	Low
	Guard against negative dispatched count dispatched can become negative if skipped accrues concurrently with the snapshot or if any counters drift (e.g., a shard is both pruned and skipped due to a race). Clamp to zero to avoid emitting a nonsensical negative shards_dispatched value in the profile output, which would also violate the invariant asserted by the IT. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/stage/shard/ShardFragmentStageExecution.java [248] public org.opensearch.analytics.exec.profile.CanMatchProfile canMatchProfile() { if (canMatchRan == false) { return null; } int skipped = canMatchSkippedByTopN.get(); boolean armed = topNGate != null && topNGate.isArmed(); - int dispatched = canMatchTotalShards - canMatchPrunedByFilter - skipped; + int dispatched = Math.max(0, canMatchTotalShards - canMatchPrunedByFilter - skipped); Suggestion importance[1-10]: 4 __ Why: Clamping dispatched to zero is a minor defensive measure. In practice all fields are volatile and updated in a controlled sequence, but a race between top-N skips and the snapshot could theoretically produce inconsistent counts.	Low

[github-actions]

Persistent review updated to latest commit fc209dc

[github-actions]

Persistent review updated to latest commit d03a2a0

[github-actions]

✅ Gradle check result for d03a2a0: SUCCESS

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.57%. Comparing base (24a14b9) to head (1b0b3d8).
⚠️ Report is 7 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #22838      +/-   ##
============================================
- Coverage     71.58%   71.57%   -0.01%     
+ Complexity    77353    77330      -23     
============================================
  Files          6170     6170              
  Lines        359700   359700              
  Branches      52459    52459              
============================================
- Hits         257493   257459      -34     
- Misses        81808    81827      +19     
- Partials      20399    20414      +15     

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

[github-actions]

Persistent review updated to latest commit 1b0b3d8

[github-actions]

✅ Gradle check result for 1b0b3d8: SUCCESS

[finnegancarroll]

testMultiIndexConcurrentRecovery

is flaky