[analytics-engine] Fix dc() on short/tinyint fields and DF54 keyword field over-counting by sandeshkr419 · Pull Request #22299 · opensearch-project/OpenSearch

sandeshkr419 · 2026-06-24T09:05:42Z

Summary

Fix dc() on TINYINT/SMALLINT: cast to INTEGER before APPROX_COUNT_DISTINCT so DataFusion uses HLL (Binary state) instead of bitmap (List state)
Fix dc() overcount on keyword fields: custom approx_distinct UDAF routes Utf8View through consistent &str hashing, avoiding different hashing paths introduced in perf: Optimize approx_distinct for inline Utf8View apache/datafusion#21064
Add DistinctCountIT — 13 tests covering keyword, short, integer, long, filtered, and grouped dc() on a 2-shard clickbench dataset

Test Plan

DistinctCountIT — 13/13 pass
Full IT suite — no regressions
Validated on high doc count cluster as well: over-counting of keyword fields now fixed

Check List

Functionality includes testing.
API changes companion pull request created, if applicable.
Public documentation issue/PR created, if applicable.

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.

Signed-off-by: Sandesh Kumar [email protected]

github-actions · 2026-06-24T09:07:12Z

PR Reviewer Guide 🔍

(Review updated until commit `3812166`)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
📝 TODO sections sandbox/plugins/analytics-backend-datafusion/rust/src/udaf/approx_distinct_safe.rs [12]: Remove once fixed upstream (likely DataFusion v55/v56). sandbox/plugins/analytics-backend-datafusion/rust/src/udaf/approx_distinct_safe.rs [13]: Evaluate UDAF extension points to avoid same-name overrides.
🔀 No multiple PR themes
⚡ Recommended focus areas for review Signature mismatch via Any(1) `SafeApproxDistinct` uses `Signature::any(1, Immutable)`, which accepts any single-arg input including types DataFusion's built-in `approx_distinct` would reject (e.g., complex/list types). Since this UDAF is registered with the same name as the built-in and delegates to it, callers may now succeed at signature resolution but fail later inside the inner accumulator with confusing errors. Consider mirroring the upstream signature instead of `any(1)`. fn new() -> Self { Self { signature: Signature::any(1, Volatility::Immutable), } } } Unchecked downcast `update_batch` unconditionally downcasts `values[0]` to `StringViewArray` whenever the registered type is `Utf8View`, but does not validate `values[0].data_type()`. If a caller passes a different array (e.g., due to upstream coercion changes), `downcast_value!` will panic rather than return an error. Returning a typed error on mismatch would be safer. fn update_batch(&mut self, values: &[ArrayRef]) -> Result<()> { let view_array: &StringViewArray = downcast_value!(values[0], StringViewArray); // Materialize as StringArray — consistent hashing via StringHLLAccumulator let string_array: StringArray = view_array.iter().collect(); self.inner.update_batch(&[Arc::new(string_array) as ArrayRef]) }

github-actions · 2026-06-24T09:08:00Z

PR Code Suggestions ✨

Latest suggestions up to 3812166

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Make one-time provisioning thread-safe The `provisioned` check is not thread-safe even with `volatile` — two tests running concurrently could both pass the check and both invoke `DatasetProvisioner.provision`, potentially causing duplicate/conflicting setup. Use a synchronized block or `AtomicBoolean.compareAndSet` to ensure single provisioning. sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/DistinctCountIT.java [23-31] -private static volatile boolean provisioned = false; +private static final java.util.concurrent.atomic.AtomicBoolean provisioned = new java.util.concurrent.atomic.AtomicBoolean(false); @Override protected void onBeforeQuery() throws IOException { - if (provisioned == false) { - DatasetProvisioner.provision(client(), CLICKBENCH, 2); - provisioned = true; + if (provisioned.compareAndSet(false, true)) { + try { + DatasetProvisioner.provision(client(), CLICKBENCH, 2); + } catch (IOException e) { + provisioned.set(false); + throw e; + } } } Suggestion importance[1-10]: 4 __ Why: Using `AtomicBoolean.compareAndSet` improves thread safety for the provisioning flag, though test classes typically run sequentially making the issue mostly theoretical.	Low
	Detect approx-count-distinct by name too The widening check only considers calls whose aggregation is the canonical `SqlStdOperatorTable.APPROX_COUNT_DISTINCT`, but the rule also rewrites the PPL `APPROX_COUNT_DISTINCT` UDF (see `isPplDistinctCountApproxUdf`). After `rewriteToApprox` all rewritten calls become the stdop, so the check should iterate over `rewritten` (which it does), but only after rewrite — confirm that no non-rewritten APPROX_COUNT_DISTINCT UDF calls slip past. Consider also matching by name to be safe. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/rules/OpenSearchDistinctCountRule.java [125-139] private static RelNode widenSmallIntArgs(RelOptRuleCall ruleCall, RelNode input, List<AggregateCall> calls) { List<RelDataTypeField> fields = input.getRowType().getFieldList(); boolean needsWiden = false; for (AggregateCall call : calls) { - if (call.getAggregation() == SqlStdOperatorTable.APPROX_COUNT_DISTINCT) { + boolean isApprox = call.getAggregation() == SqlStdOperatorTable.APPROX_COUNT_DISTINCT + \|\| "APPROX_COUNT_DISTINCT".equals(call.getAggregation().getName()); + if (isApprox) { for (int argIdx : call.getArgList()) { SqlTypeName typeName = fields.get(argIdx).getType().getSqlTypeName(); if (typeName == SqlTypeName.TINYINT \|\| typeName == SqlTypeName.SMALLINT) { needsWiden = true; break; } } } if (needsWiden) break; } Suggestion importance[1-10]: 3 __ Why: The `widenSmallIntArgs` is called after `rewriteToApprox` has already converted PPL UDF calls to the canonical stdop, so the additional name check is largely redundant. Low impact.	Low
	Guard against missing argument fields `expr_fields` indexes argument 0 without bounds-checking. If a future caller invokes this UDAF with zero arguments (e.g., parser error path), this will panic. Validate `expr_fields.len() >= 1` and return a `DataFusionError` instead of indexing directly. sandbox/plugins/analytics-backend-datafusion/rust/src/udaf/approx_distinct_safe.rs [60-74] fn accumulator(&self, acc_args: AccumulatorArgs) -> Result<Box<dyn Accumulator>> { + if acc_args.expr_fields.is_empty() { + return Err(datafusion::common::DataFusionError::Internal( + "approx_distinct requires exactly one argument".into(), + )); + } let data_type = acc_args.expr_fields[0].data_type(); if matches!(data_type, DataType::Utf8View) { - // Wrap DF's Utf8 accumulator, feed it materialized &str from StringViewArray let utf8_args = AccumulatorArgs { return_field: acc_args.return_field, schema: acc_args.schema, ignore_nulls: acc_args.ignore_nulls, order_bys: acc_args.order_bys, name: acc_args.name, is_distinct: acc_args.is_distinct, exprs: acc_args.exprs, expr_fields: &[Arc::new(Field::new("x", DataType::Utf8, true))], is_reversed: acc_args.is_reversed, }; Suggestion importance[1-10]: 3 __ Why: Defensive guard against an empty `expr_fields` is a minor robustness improvement; DataFusion's signature validation should already enforce arity 1, making this unlikely in practice.	Low

Previous suggestions

Suggestions up to commit c5cae93

Category	Suggestion	Impact
Possible issue	Exclude NULL groups from ground truth Using `datarows.size()` as ground truth for distinct count includes groups whose key is NULL, while `distinct_count`/HLL typically excludes NULLs. This can cause off-by-one tolerance failures on fields with NULL values. Filter out NULL key rows from the ground truth (or add `\| where field is not null`) before counting. sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/DistinctCountIT.java [112-113] -Map<String, Object> grouped = executePpl("source=" + IDX + " \| stats count() by " + field); +Map<String, Object> grouped = executePpl("source=" + IDX + " \| where " + field + " is not null \| stats count() by " + field); int truth = ((List<List<Object>>) grouped.get("datarows")).size(); Suggestion importance[1-10]: 7 __ Why: Valid concern: `count() by field` may include a NULL group, while `distinct_count` typically excludes NULLs, which could cause off-by-one mismatches especially with the strict equality check for low cardinality fields.	Medium
	Also widen args for UDF approx variant The widening check only considers calls already using the stdop `APPROX_COUNT_DISTINCT`, but at this point `calls` contains the newly rewritten list which may also include the PPL UDF variant produced by `rewriteToApprox`/originals. To be safe and consistent with `isPplDistinctCountApproxUdf`, also include calls whose aggregation name equals `"APPROX_COUNT_DISTINCT"`, otherwise sub-32-bit args under the UDF variant won't be widened. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/rules/OpenSearchDistinctCountRule.java [128-139] for (AggregateCall call : calls) { - if (call.getAggregation() == SqlStdOperatorTable.APPROX_COUNT_DISTINCT) { + boolean isApprox = call.getAggregation() == SqlStdOperatorTable.APPROX_COUNT_DISTINCT + \|\| "APPROX_COUNT_DISTINCT".equals(call.getAggregation().getName()); + if (isApprox) { for (int argIdx : call.getArgList()) { SqlTypeName typeName = fields.get(argIdx).getType().getSqlTypeName(); if (typeName == SqlTypeName.TINYINT \|\| typeName == SqlTypeName.SMALLINT) { needsWiden = true; break; } } } if (needsWiden) break; } Suggestion importance[1-10]: 6 __ Why: The suggestion has merit: `rewriteToApprox` produces stdop APPROX_COUNT_DISTINCT, but pass-through calls from `isPplDistinctCountApproxUdf` keep the UDF variant name. Including those would make the widening logic more robust, though it's unclear if the UDF variant path is currently reachable here.	Low
	Avoid panicking downcast in accumulator `update_batch` assumes `values[0]` is always a `StringViewArray`, but the accumulator is selected based on the declared type `Utf8View`; if an upstream stage passes a different concrete array (e.g. after a cast or in tests), the `downcast_value!` macro will panic. Return an error via a checked downcast instead of panicking. sandbox/plugins/analytics-backend-datafusion/rust/src/udaf/approx_distinct_safe.rs [94-99] impl Accumulator for Utf8ViewToUtf8Accumulator { fn update_batch(&mut self, values: &[ArrayRef]) -> Result<()> { - let view_array: &StringViewArray = downcast_value!(values[0], StringViewArray); - // Materialize as StringArray — consistent hashing via StringHLLAccumulator + let view_array = values[0].as_any().downcast_ref::<StringViewArray>() + .ok_or_else(\|\| datafusion::common::DataFusionError::Internal( + format!("Utf8ViewToUtf8Accumulator expected StringViewArray, got {:?}", values[0].data_type())))?; let string_array: StringArray = view_array.iter().collect(); self.inner.update_batch(&[Arc::new(string_array) as ArrayRef]) } Suggestion importance[1-10]: 4 __ Why: Replacing the panicking macro with a checked downcast is a defensive improvement, but in practice the accumulator is selected based on declared type so the input should always be StringViewArray. Minor robustness gain.	Low

Suggestions up to commit 34d0afc

Category	Suggestion	Impact
Possible issue	Exclude nulls from ground-truth count Counting all rows of `stats count() by field` includes a row for NULL values, but `distinct_count` typically excludes NULLs. This skews the ground truth by 1 when the field has any nulls, potentially causing false test failures at low cardinality where exact equality is asserted. Filter out nulls or subtract the null group. sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/DistinctCountIT.java [112-113] -Map<String, Object> grouped = executePpl("source=" + IDX + " \| stats count() by " + field); +Map<String, Object> grouped = executePpl("source=" + IDX + " \| where isnotnull(" + field + ") \| stats count() by " + field); int truth = ((List<List<Object>>) grouped.get("datarows")).size(); Suggestion importance[1-10]: 7 __ Why: Valid concern: `distinct_count` typically excludes nulls, but `count() by field` may include a null group, skewing ground truth. This could cause false test failures, especially for the exact-equality branch at low cardinality.	Medium
Possible issue	Restrict cast to APPROX_COUNT_DISTINCT args only The widening check only inspects calls whose aggregation is `SqlStdOperatorTable.APPROX_COUNT_DISTINCT`, but the project step below casts those columns for ALL agg calls referencing them. If a non-rewritten call (e.g., SUM on the same SMALLINT column) shares the index, its semantics change because the input type is now INTEGER. Restrict the cast to columns referenced only by APPROX_COUNT_DISTINCT, or verify other calls tolerate the widened type. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/rules/OpenSearchDistinctCountRule.java [148-157] +java.util.Set<Integer> widenIdx = new java.util.HashSet<>(); for (AggregateCall call : calls) { if (call.getAggregation() == SqlStdOperatorTable.APPROX_COUNT_DISTINCT) { for (int argIdx : call.getArgList()) { SqlTypeName typeName = fields.get(argIdx).getType().getSqlTypeName(); if (typeName == SqlTypeName.TINYINT \|\| typeName == SqlTypeName.SMALLINT) { - needsWiden = true; - break; + widenIdx.add(argIdx); } } } - if (needsWiden) break; } +boolean needsWiden = !widenIdx.isEmpty(); Suggestion importance[1-10]: 6 __ Why: Reasonable concern about correctness when other aggregations reference the same column. However, the project widens the input row to all downstream agg calls, which could inadvertently affect other calls; the suggestion's improved code is incomplete (only changes detection, not the projection logic).	Low
General	Avoid panic on empty argument list `acc_args.expr_fields[0]` will panic with an index out-of-bounds if invoked with zero arguments. While the signature requires arity 1, defensive indexing prevents a process crash from malformed plans. Use `.get(0)` or check `len()` first. sandbox/plugins/analytics-backend-datafusion/rust/src/udaf/approx_distinct_safe.rs [60-62] fn accumulator(&self, acc_args: AccumulatorArgs) -> Result<Box<dyn Accumulator>> { - let data_type = acc_args.expr_fields[0].data_type(); + let data_type = acc_args.expr_fields.first() + .map(\|f\| f.data_type().clone()) + .unwrap_or(DataType::Null); if matches!(data_type, DataType::Utf8View) { Suggestion importance[1-10]: 3 __ Why: Defensive coding suggestion; signature enforces arity 1 so this is unlikely in practice. Marginal robustness improvement.	Low

Suggestions up to commit 5bcb413

Category	Suggestion	Impact
Possible issue	Avoid panic on unexpected array type `Utf8ViewToUtf8Accumulator` assumes `values[0]` is always a `StringViewArray`, but `accumulator` is only steered to this wrapper when the field's declared type is `Utf8View`. If a batch arrives with a different array type (e.g. after a cast or merge state), `downcast_value!` will panic. Consider returning an error instead of panicking, or handling `StringArray` input as a fallback. sandbox/plugins/analytics-backend-datafusion/rust/src/udaf/approx_distinct_safe.rs [94-99] fn update_batch(&mut self, values: &[ArrayRef]) -> Result<()> { - let view_array: &StringViewArray = downcast_value!(values[0], StringViewArray); - // Materialize as StringArray — consistent hashing via StringHLLAccumulator - let string_array: StringArray = view_array.iter().collect(); - self.inner.update_batch(&[Arc::new(string_array) as ArrayRef]) + use datafusion::arrow::array::AsArray; + if let Some(view_array) = values[0].as_any().downcast_ref::<StringViewArray>() { + let string_array: StringArray = view_array.iter().collect(); + self.inner.update_batch(&[Arc::new(string_array) as ArrayRef]) + } else { + self.inner.update_batch(values) + } } Suggestion importance[1-10]: 4 __ Why: Defensive handling of unexpected array types could prevent panics in edge cases, though the wrapper is only constructed for `Utf8View` so the risk is low in practice.	Low
General	Widen check should cover all approx-count calls The widening predicate only triggers for the canonical `APPROX_COUNT_DISTINCT` stdop, but the rule also rewrites a PPL UDF named `APPROX_COUNT_DISTINCT` (see `isPplDistinctCountApproxUdf`) and `COUNT(DISTINCT ...)`. After rewriting these to the stdop, the new calls in `rewritten` will be stdop calls, but if `widenSmallIntArgs` is ever invoked with mixed lists, the check should match what was actually rewritten. Consider checking by name or matching all rewritten calls to avoid missing cases. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/rules/OpenSearchDistinctCountRule.java [125-139] private static RelNode widenSmallIntArgs(RelOptRuleCall ruleCall, RelNode input, List<AggregateCall> calls) { List<RelDataTypeField> fields = input.getRowType().getFieldList(); boolean needsWiden = false; for (AggregateCall call : calls) { - if (call.getAggregation() == SqlStdOperatorTable.APPROX_COUNT_DISTINCT) { + if (call.getAggregation() == SqlStdOperatorTable.APPROX_COUNT_DISTINCT + \|\| "APPROX_COUNT_DISTINCT".equals(call.getAggregation().getName())) { for (int argIdx : call.getArgList()) { SqlTypeName typeName = fields.get(argIdx).getType().getSqlTypeName(); if (typeName == SqlTypeName.TINYINT \|\| typeName == SqlTypeName.SMALLINT) { needsWiden = true; break; } } } if (needsWiden) break; } Suggestion importance[1-10]: 3 __ Why: The widening is invoked after rewriting, where calls in `rewritten` should already be the canonical `APPROX_COUNT_DISTINCT` stdop, so the additional name check is mostly redundant. Minor defensive improvement.	Low
	Make provisioning flag thread-safe and rollback-aware The `provisioned` flag is read and written without synchronization. If the test framework runs `onBeforeQuery` concurrently from multiple threads, two provisioning calls may race. Use a synchronized block or `AtomicBoolean.compareAndSet` to ensure single provisioning. sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/DistinctCountIT.java [23-31] -private static volatile boolean provisioned = false; +private static final java.util.concurrent.atomic.AtomicBoolean provisioned = new java.util.concurrent.atomic.AtomicBoolean(false); @Override protected void onBeforeQuery() throws IOException { - if (provisioned == false) { - DatasetProvisioner.provision(client(), CLICKBENCH, 2); - provisioned = true; + if (provisioned.compareAndSet(false, true)) { + try { + DatasetProvisioner.provision(client(), CLICKBENCH, 2); + } catch (IOException e) { + provisioned.set(false); + throw e; + } } } Suggestion importance[1-10]: 3 __ Why: Test framework typically runs sequentially per test class, so the race is unlikely. However, the rollback-aware pattern adds robustness in case of provisioning failure.	Low
	Cache upstream UDAF instance to avoid reallocation Delegating `return_type` to upstream is fine, but `state_fields` and `accumulator` are also delegated using `approx_distinct_udaf()` which constructs a fresh UDAF on each call. Cache the instance once (e.g. via `OnceLock` or a `lazy_static`) to avoid repeated allocation on every plan/accumulator instantiation, especially since `accumulator` runs per-partition. sandbox/plugins/analytics-backend-datafusion/rust/src/udaf/approx_distinct_safe.rs [52-54] fn return_type(&self, args: &[DataType]) -> Result<DataType> { - approx_distinct_udaf().inner().return_type(args) + use std::sync::OnceLock; + static INNER: OnceLock<std::sync::Arc<datafusion::logical_expr::AggregateUDF>> = OnceLock::new(); + INNER.get_or_init(\|\| approx_distinct_udaf()).inner().return_type(args) } Suggestion importance[1-10]: 2 __ Why: `approx_distinct_udaf()` is typically a cheap constructor and not a hot path concern; caching is a marginal optimization with minimal real impact.	Low

Suggestions up to commit 2be7e20

Category	Suggestion	Impact
General	Guard one-time provisioning against races The `provisioned` flag is a non-synchronized static `volatile`, leading to a race where multiple test methods may concurrently call `DatasetProvisioner.provision` if executed in parallel. Use synchronization or a double-checked lock to ensure provisioning happens exactly once. sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/DistinctCountIT.java [26-31] private static final Dataset CLICKBENCH = ClickBenchTestHelper.DATASET; private static final String IDX = CLICKBENCH.indexName; +private static final Object PROVISION_LOCK = new Object(); private static volatile boolean provisioned = false; @Override protected void onBeforeQuery() throws IOException { if (provisioned == false) { - DatasetProvisioner.provision(client(), CLICKBENCH, 2); - provisioned = true; + synchronized (PROVISION_LOCK) { + if (provisioned == false) { + DatasetProvisioner.provision(client(), CLICKBENCH, 2); + provisioned = true; + } + } } } Suggestion importance[1-10]: 4 __ Why: Adding double-checked locking around `provisioned` is a reasonable safety improvement for concurrent test execution, though tests in a single class typically run sequentially, limiting impact.	Low
General	Assert single argument in update_batch `update_batch` only handles a single-argument call and ignores any additional arrays in `values`. While `approx_distinct` is unary, forwarding extra arrays would be safer for forward compatibility; at minimum, debug-assert the expected length to fail fast on misuse. sandbox/plugins/analytics-backend-datafusion/rust/src/udaf/approx_distinct_safe.rs [90-95] fn update_batch(&mut self, values: &[ArrayRef]) -> Result<()> { + debug_assert_eq!(values.len(), 1, "approx_distinct expects exactly one argument"); let view_array: &StringViewArray = downcast_value!(values[0], StringViewArray); // Materialize as StringArray — consistent hashing via StringHLLAccumulator let string_array: StringArray = view_array.iter().collect(); self.inner.update_batch(&[Arc::new(string_array) as ArrayRef]) } Suggestion importance[1-10]: 2 __ Why: A debug assertion is a minor defensive improvement; `approx_distinct` is unary by definition so the practical impact is low.	Low
Possible issue	Match all approx-count-distinct variants for widening The widening loop only inspects calls whose aggregation is the standard `APPROX_COUNT_DISTINCT`, but the rewrite step in `onMatch` also rewrites PPL `APPROX_COUNT_DISTINCT` UDF calls (per `isPplDistinctCountApproxUdf`) that may not equal `SqlStdOperatorTable.APPROX_COUNT_DISTINCT` after rewrite. Verify the post-rewrite calls are matched here, or check by name to avoid missing widening for PPL-originated calls. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/rules/OpenSearchDistinctCountRule.java [128-139] for (AggregateCall call : calls) { - if (call.getAggregation() == SqlStdOperatorTable.APPROX_COUNT_DISTINCT) { + if (call.getAggregation() == SqlStdOperatorTable.APPROX_COUNT_DISTINCT + \|\| "APPROX_COUNT_DISTINCT".equals(call.getAggregation().getName())) { for (int argIdx : call.getArgList()) { SqlTypeName typeName = fields.get(argIdx).getType().getSqlTypeName(); if (typeName == SqlTypeName.TINYINT \|\| typeName == SqlTypeName.SMALLINT) { needsWiden = true; break; } } } if (needsWiden) break; } Suggestion importance[1-10]: 3 __ Why: The `rewriteToApprox` method always returns calls using `SqlStdOperatorTable.APPROX_COUNT_DISTINCT`, so post-rewrite all approx calls are the standard one. The suggestion's concern is largely unfounded.	Low

Suggestions up to commit 2a55b03

Category	Suggestion	Impact
Possible issue	Exclude nulls from ground-truth count `stats count() by field` includes a row for `NULL` values, but `distinct_count` typically excludes nulls. This will cause spurious test failures (off-by-one) on fields with null values at low cardinality where exact equality is asserted. Filter out nulls in the ground-truth query or subtract the null group. sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/DistinctCountIT.java [111-113] private void assertDcAccurate(String field) throws Exception { - Map<String, Object> grouped = executePpl("source=" + IDX + " \| stats count() by " + field); + Map<String, Object> grouped = executePpl("source=" + IDX + " \| where isnotnull(" + field + ") \| stats count() by " + field); int truth = ((List<List<Object>>) grouped.get("datarows")).size(); Suggestion importance[1-10]: 7 __ Why: Valid concern: `distinct_count` typically excludes nulls while `stats count() by field` includes a null group, which could cause off-by-one failures at low cardinality where exact equality is enforced.	Medium
General	Make provisioning guard thread-safe The check-then-set on `provisioned` is not atomic even though the field is volatile, so concurrent test executions could trigger provisioning twice. Use synchronization or an `AtomicBoolean.compareAndSet` to ensure single provisioning. sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/DistinctCountIT.java [23-31] -private static volatile boolean provisioned = false; +private static final java.util.concurrent.atomic.AtomicBoolean provisioned = new java.util.concurrent.atomic.AtomicBoolean(false); @Override protected void onBeforeQuery() throws IOException { - if (provisioned == false) { + if (provisioned.compareAndSet(false, true)) { DatasetProvisioner.provision(client(), CLICKBENCH, 2); - provisioned = true; } } Suggestion importance[1-10]: 3 __ Why: Tests within a class are typically run sequentially, so the race condition is unlikely to occur in practice, though using `AtomicBoolean` is a minor robustness improvement.	Low

github-actions · 2026-06-24T11:08:17Z

❌ Gradle check result for 5f2557a: null

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2026-06-24T16:33:07Z

Persistent review updated to latest commit bb22ba7

github-actions · 2026-06-24T17:27:25Z

❌ Gradle check result for bb22ba7: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2026-06-24T18:37:55Z

Persistent review updated to latest commit 8644aa8

github-actions · 2026-06-24T19:15:09Z

Persistent review updated to latest commit 9f69501

github-actions · 2026-06-24T19:18:40Z

Persistent review updated to latest commit 2a55b03

github-actions · 2026-06-24T19:26:31Z

Persistent review updated to latest commit 2be7e20

expani

Thanks for the fix @sandeshkr419 I analyzed DF Physical plan rewrites as well and it seems more hairy because of lack of sufficient extension points.

github-actions · 2026-06-24T22:29:18Z

Persistent review updated to latest commit 5bcb413

github-actions · 2026-06-24T23:22:03Z

✅ Gradle check result for 5bcb413: SUCCESS

codecov · 2026-06-24T23:22:52Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.38%. Comparing base (79d8e1e) to head (3812166).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #22299      +/-   ##
============================================
- Coverage     73.38%   73.38%   -0.01%     
+ Complexity    76071    76063       -8     
============================================
  Files          6076     6076              
  Lines        345461   345463       +2     
  Branches      49723    49724       +1     
============================================
- Hits         253527   253517      -10     
+ Misses        71731    71723       -8     
- Partials      20203    20223      +20

☔ 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 · 2026-06-24T23:35:42Z

Persistent review updated to latest commit 34d0afc

github-actions · 2026-06-25T00:30:38Z

✅ Gradle check result for 34d0afc: SUCCESS

github-actions · 2026-06-25T04:27:24Z

Persistent review updated to latest commit c5cae93

Cast TINYINT/SMALLINT to INTEGER before APPROX_COUNT_DISTINCT so DataFusion uses HLL (Binary state) instead of bitmap (List state). Widen derive_schema_from_partial_plan to detect List state types. Signed-off-by: Sandesh Kumar <[email protected]>

…ntIT Register custom approx_distinct UDAF for Utf8View that uses consistent &str hashing, fixing apache/datafusion#21064. Add DistinctCountIT covering all field types on 2-shard clickbench. Signed-off-by: Sandesh Kumar <[email protected]>

Add unit test asserting CAST(SMALLINT→INTEGER) is present in the plan when COUNT(DISTINCT) targets a sub-32-bit column. Add TODOs on approx_distinct_safe.rs for upstream removal timeline. Signed-off-by: Sandesh Kumar <[email protected]>

dc(UserID) grouped by RegionID with sort+head produces non-deterministic results on 2-node clusters due to HLL estimate variance across shards for tied groups. Mute until golden comparison handles tied-row tolerance. Signed-off-by: Sandesh Kumar <[email protected]>

github-actions · 2026-06-25T05:09:56Z

Persistent review updated to latest commit 3812166

github-actions · 2026-06-25T06:05:10Z

✅ Gradle check result for 3812166: SUCCESS

…field over-counting (opensearch-project#22299) * [analytics-engine] Fix dc() on short/tinyint fields Cast TINYINT/SMALLINT to INTEGER before APPROX_COUNT_DISTINCT so DataFusion uses HLL (Binary state) instead of bitmap (List state). Widen derive_schema_from_partial_plan to detect List state types. Signed-off-by: Sandesh Kumar <[email protected]> * [analytics-engine] Fix dc() overcount on keyword fields + DistinctCountIT Register custom approx_distinct UDAF for Utf8View that uses consistent &str hashing, fixing apache/datafusion#21064. Add DistinctCountIT covering all field types on 2-shard clickbench. Signed-off-by: Sandesh Kumar <[email protected]> * [analytics-engine] Add UT for SMALLINT widening + TODOs for upstream fix Add unit test asserting CAST(SMALLINT→INTEGER) is present in the plan when COUNT(DISTINCT) targets a sub-32-bit column. Add TODOs on approx_distinct_safe.rs for upstream removal timeline. Signed-off-by: Sandesh Kumar <[email protected]> * [analytics-engine] Mute flaky PplClickBenchIT Q9/Q10 dc(UserID) grouped by RegionID with sort+head produces non-deterministic results on 2-node clusters due to HLL estimate variance across shards for tied groups. Mute until golden comparison handles tied-row tolerance. Signed-off-by: Sandesh Kumar <[email protected]> --------- Signed-off-by: Sandesh Kumar <[email protected]>

sandeshkr419 requested a review from a team as a code owner June 24, 2026 09:05

expani reviewed Jun 24, 2026

View reviewed changes

Comment thread ...engine/src/main/java/org/opensearch/analytics/planner/rules/OpenSearchDistinctCountRule.java Resolved

sandeshkr419 force-pushed the dchash branch from 5f2557a to bb22ba7 Compare June 24, 2026 16:31

This was referenced Jun 24, 2026

[AUTOCUT] Gradle Check Flaky Test Report for WarmIndexBasicIT #22194

Open

[AUTOCUT] Gradle Check Flaky Test Report for ShardIndexingPressureSettingsIT #14331

Open

sandeshkr419 force-pushed the dchash branch from 8644aa8 to 9f69501 Compare June 24, 2026 19:13

sandeshkr419 force-pushed the dchash branch from 9f69501 to 2a55b03 Compare June 24, 2026 19:16

sandeshkr419 force-pushed the dchash branch from 2a55b03 to 2be7e20 Compare June 24, 2026 19:23

opensearch-ci-bot mentioned this pull request Jun 24, 2026

[AUTOCUT] Gradle Check Flaky Test Report for ThirdPartyAuditTaskIT #22215

Open

expani reviewed Jun 24, 2026

View reviewed changes

Comment thread ...engine/src/main/java/org/opensearch/analytics/planner/rules/OpenSearchDistinctCountRule.java Resolved

expani approved these changes Jun 24, 2026

View reviewed changes

expani reviewed Jun 24, 2026

View reviewed changes

Comment thread sandbox/plugins/analytics-backend-datafusion/rust/src/udaf/approx_distinct_safe.rs Resolved

sandeshkr419 force-pushed the dchash branch from 2be7e20 to 5bcb413 Compare June 24, 2026 22:28

sandeshkr419 force-pushed the dchash branch from 5bcb413 to 34d0afc Compare June 24, 2026 23:33

mch2 approved these changes Jun 25, 2026

View reviewed changes

sandeshkr419 force-pushed the dchash branch from 34d0afc to c5cae93 Compare June 25, 2026 04:26

sandeshkr419 added 4 commits June 25, 2026 05:08

sandeshkr419 force-pushed the dchash branch from c5cae93 to 3812166 Compare June 25, 2026 05:08

mch2 merged commit a4302a3 into opensearch-project:main Jun 25, 2026
16 checks passed

Conversation

sandeshkr419 commented Jun 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Check List

Uh oh!

github-actions Bot commented Jun 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Reviewer Guide 🔍

(Review updated until commit 3812166)

Uh oh!

github-actions Bot commented Jun 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Previous suggestions

Uh oh!

Uh oh!

github-actions Bot commented Jun 24, 2026

Uh oh!

github-actions Bot commented Jun 24, 2026

Uh oh!

github-actions Bot commented Jun 24, 2026

Uh oh!

github-actions Bot commented Jun 24, 2026

Uh oh!

github-actions Bot commented Jun 24, 2026

Uh oh!

github-actions Bot commented Jun 24, 2026

Uh oh!

github-actions Bot commented Jun 24, 2026

Uh oh!

Uh oh!

expani left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions Bot commented Jun 24, 2026

Uh oh!

github-actions Bot commented Jun 24, 2026

Uh oh!

codecov Bot commented Jun 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions Bot commented Jun 24, 2026

Uh oh!

github-actions Bot commented Jun 25, 2026

Uh oh!

github-actions Bot commented Jun 25, 2026

Uh oh!

github-actions Bot commented Jun 25, 2026

Uh oh!

github-actions Bot commented Jun 25, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sandeshkr419 commented Jun 24, 2026 •

edited

Loading

github-actions Bot commented Jun 24, 2026 •

edited

Loading

(Review updated until commit `3812166`)

github-actions Bot commented Jun 24, 2026 •

edited

Loading

codecov Bot commented Jun 24, 2026 •

edited

Loading