Add bool query and minimum_should_match support to DSL query executor

[ask-kamal-nayan]

Description

Adds BoolQueryTranslator with full minimum_should_match support to the dsl-query-executor sandbox plugin's translator registry, enabling DSL bool queries to be converted into Calcite logical expressions for the multi-engine analytics path.

What: A translator that handles must, filter, should, must_not, and all six minimum_should_match grammar variants, producing RexNode trees that Calcite can plan and execute.

Why: bool is the composition layer of the OpenSearch DSL — without it, only isolated leaf queries (term, terms, match_all, exists) can be translated. With this PR, those leaf queries can be nested and combined, unlocking real-world compound DSL queries on the analytics path.

Where: BoolQueryTranslator registers in QueryRegistryFactory alongside existing leaf translators. MinimumShouldMatchParser is a standalone utility replicating the semantics of the legacy Queries.calculateMinShouldMatch.

Scope: Bool composition + MSM parsing + conjoined arithmetic form. Does not add new leaf translators or scoring semantics.

Translation Model

Input DSL	Calcite Logical Expression
{"bool":{"must":[{"term":{"status":"active"}}],"should":[{"term":{"brand":"X"}},{"term":{"brand":"Y"}}]}}	status = 'active' (should is optional — MSM defaults to 0 when must is present)
{"bool":{"should":[{"term":{"a":"1"}},{"term":{"b":"2"}},{"term":{"c":"3"}}],"minimum_should_match":"2"}}	AND(OR(a='1', b='2', c='3'), GTE(CASE(a='1',1,0) + CASE(b='2',1,0) + CASE(c='3',1,0), 2)) — conjoined arithmetic form, linear in clause count
{"bool":{"must_not":[{"bool":{"must_not":[{"term":{"name":"val"}}]}}]}}	name = 'val' — double negation eliminated: NOT(NOT(x)) → x

Supported / Rejected Parameter Matrix

Parameter	Status	Legacy Citation
must	✅ Supported (AND)	BoolQueryBuilder.doToQuery lines 243–256: Occur.MUST
filter	✅ Supported (AND, identical to must at Calcite level)	BoolQueryBuilder.addBooleanClauses line 262: Occur.FILTER
should	✅ Supported (OR, MSM-controlled)	BoolQueryBuilder.addBooleanClauses line 264: Occur.SHOULD
must_not	✅ Supported (NOT with double-negation elimination)	BoolQueryBuilder.addBooleanClauses line 266: Occur.MUST_NOT
minimum_should_match	✅ Supported (all 6 grammars)	Queries.calculateMinShouldMatch lines 129–165
adjust_pure_negative	✅ Supported (shape-gated legacy parity)	BoolQueryBuilder.doToQuery line 338, Queries.isNegativeQuery lines 113–119, Queries.fixNegativeQueryIfNeeded lines 121–130: pure-negative + false → match-none FALSE literal; all other shapes ignore the flag (no-op) because isNegativeQuery requires every clause to be prohibited
boost	❌ Rejected when ≠ 1.0	AbstractQueryBuilder.toQuery lines 130–136: BoostQuery wrapping
_name	❌ Rejected when non-null	AbstractQueryBuilder.toQuery lines 137–139: named query registration

minimum_should_match Variant Table

Grammar	Example	Semantics
Non-negative integer	"2"	Exactly 2 should clauses must match
Negative integer	"-1"	total − 1 must match
Non-negative percentage	"70%"	⌊total × 0.70⌋ must match
Negative percentage	"-30%"	total − ⌊total × 0.30⌋ must match
Single combination	"2<75%"	If total ≤ 2 match all; else apply 75%
Multiple combinations	"3<-1 5<50%"	Threshold ranges evaluated left-to-right; last exceeded wins

Conjoined minimum_should_match Form

For required matches strictly between 1 and the clause count, the translator emits:

AND(OR(p1..pn), GTE(left-deep PLUS chain of CASE(pi, 1, 0), k))

Aspect	Detail
Arithmetic GTE	Enforces exact at-least-k-of-n semantics — each CASE(pi, 1, 0) contributes 1 when its predicate is true, the sum is compared >= k
Redundant OR conjunct	Logically implied by GTE (any k ≥ 1 means at least one clause matched), but exists so the analytics backend page pruner sees column-versus-constant leaves it can use for page skipping. AND intersects per-child pruning bitmaps; an opaque child (the counting expression) contributes an all-true vector, so only the OR child drives pruning
Expression size	Linear in clause count (2n + constant nodes), not combinatorial — no query is rejected for size
Fast paths	required = 0 → should is optional (omitted); required = 1 → plain OR; required = n → plain AND; required > n → match-none FALSE literal

Tests

Category	File	Count
Unit — translator	BoolQueryTranslatorTests.java	50
Unit — MSM parser	MinimumShouldMatchParserTests.java	11
Integration	DslBoolQueryIT.java	18
Golden files	bool_must_should_hits.json, bool_minimum_should_match_hits.json	2
Total		61 unit + 18 IT

DslBoolQueryIT is parked with @AwaitsFix(bugUrl = "analytics engine pipeline not E2E complete: fragment conversion + shard execution + Arrow Flight drain not yet wired"), matching sibling ITs (DslRangeQueryIT, etc.) until the DSL pipeline is wired end-to-end.

Known Gaps and Divergences

Case	This Path	Legacy	Reason / Follow-up
Child translator surface	Only term, terms, match_all, exists, bool registered today	All 50+ registered query types	Tracked via // TODO: add other query translators in QueryRegistryFactory; each query type lands as its own PR
Scoring	Not represented — filter and must produce identical AND	must scores, filter does not	Calcite path is non-scoring by design; scoring is a separate analytics-engine concern
Page pruning precision	Conjoined form provides coarser pruning than the enumerated form (OR of all clauses vs. per-combination conjuncts)	N/A (no analytics path)	Results are identical; only page-skip granularity differs marginally. Tradeoff: linear plan size and full query acceptance vs. marginally finer pruning at small clause counts
boost / _name	Rejected	BoostQuery wrapping / named query registration	Not representable on the analytics path today

Related Issues

Part of the DSL query support track for the multi-engine analytics path.

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.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 6403717)

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 Incorrect Flatten Operator In processShouldClauses, when requiredMatches == shouldConditions.size(), the code flattens shouldConditions using SqlStdOperatorTable.AND. However, shouldConditions contains the results of individual should-clause conversions, which may themselves be OR nodes (e.g., nested bools). Flattening with AND will not flatten nested ORs and, more importantly, is semantically off: the intent is to produce AND over the conditions, but the flatten call is meant to unwrap any inner AND from a converted clause, not to convert OR to AND. This is likely fine for the common case, but a should-clause containing a nested bool with should-only would yield an OR RexNode that is combined via outer AND — that's correct — but the flatten step does nothing useful here. Verify this branch behaves as intended and consider whether flattening is required at all. if (requiredMatches == shouldConditions.size()) { // All should conditions must match — produce AND List<RexNode> flatAnd = flattenConditions(shouldConditions, SqlStdOperatorTable.AND); return flatAnd.size() == 1 ? flatAnd.get(0) : ctx.getRexBuilder().makeCall(SqlStdOperatorTable.AND, flatAnd); } Percentage Parsing Divergence parsePercentage uses Math.floor for both positive and negative percentages, but legacy Queries.calculateMinShouldMatch uses (int)(shouldClauses * percent * 1e-2) for positive (truncation toward zero, equivalent to floor for positives) and shouldClauses + (int)(shouldClauses * -percent * 1e-2) for negative. For fractional percentages the two approaches agree for positive values but for negative percentages the legacy semantics apply floor to the absolute value; behavior should match. Also, negative-percentage handling passes -percent which is positive, so Math.floor behaves like truncation — verify this exactly matches legacy behavior for edge cases like -33% on 3 clauses. private int parsePercentage(String value, int total) throws ConversionException { String numStr = value.substring(0, value.length() - 1); try { double percent = Double.parseDouble(numStr); if (percent >= 0) { return (int) Math.floor(total * percent / 100.0); } else { int allowed = (int) Math.floor(total * (-percent) / 100.0); return total - allowed; } } catch (NumberFormatException e) { throw new ConversionException("Invalid percentage in minimum_should_match: \"" + value + "\"", e); } } Golden Plan Missing Should The expectedRelNodePlan shows only =($0, CAST('laptop')) with no representation of the should clauses. Given that must is present without minimum_should_match, should clauses are optional and contribute only to scoring in legacy behavior, so dropping them at the filter level is acceptable — but this is worth confirming as the intended semantic (since scoring is not modeled). If future consumers expect should clauses to influence result ordering, this silent drop will surprise them. "expectedRelNodePlan": [ "LogicalFilter(condition=[=($0, CAST('laptop':VARCHAR):VARCHAR)])", " LogicalTableScan(table=[[test-index]])" ],

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 6403717

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Align percentage rounding with Lucene semantics Legacy Queries.calculateMinShouldMatch uses Math.round (not Math.floor) for percentage calculations, and uses optionalClauseCount * percent / 100 with integer semantics. Using Math.floor here causes 70% of 4 to return 2 while legacy Lucene returns 2 as well, but for cases like 75% of 4 the results diverge (floor=3, round=3 — same, but e.g. 50% of 5 gives floor=2, round=3). This creates behavioral drift from the OpenSearch/Lucene semantics documented in the PR. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/query/BoolQueryTranslator.java [258-271] private int parsePercentage(String value, int total) throws ConversionException { String numStr = value.substring(0, value.length() - 1); try { double percent = Double.parseDouble(numStr); if (percent >= 0) { - return (int) Math.floor(total * percent / 100.0); + return (int) (total * percent / 100); } else { - int allowed = (int) Math.floor(total * (-percent) / 100.0); + int allowed = (int) (total * (-percent) / 100); return total - allowed; } } catch (NumberFormatException e) { throw new ConversionException("Invalid percentage in minimum_should_match: \"" + value + "\"", e); } } Suggestion importance[1-10]: 5 __ Why: The suggestion raises a valid concern about matching Lucene's Queries.calculateMinShouldMatch semantics, but the reasoning in the example is partially incorrect (50% of 5 with integer truncation is 2, not 3). Still, aligning with legacy behavior is a reasonable correctness concern.	Low
Possible issue	Prevent silent match-all on null conversion The pure-negative detection uses boolQuery.mustNot().isEmpty() before recursive conversion, but the isPureNegative check occurs before any conversion. If must_not clauses convert to null (unsupported), the resulting expression may still degenerate to match-all incorrectly. Also, silently skipping null conditions from queryRegistry.convert can produce semantically wrong translations (e.g., an unconvertible must clause becomes match-all). Consider throwing ConversionException when a clause returns null, or explicitly documenting/handling this behavior. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/query/BoolQueryTranslator.java [113-122] // Must_not clauses (NOT) with double-negation elimination for (QueryBuilder mustNotClause : boolQuery.mustNot()) { RexNode condition = queryRegistry.convert(mustNotClause, ctx); - if (condition != null) { - if (condition instanceof RexCall && ((RexCall) condition).getOperator() == SqlStdOperatorTable.NOT) { - conditions.add(((RexCall) condition).getOperands().get(0)); - } else { - conditions.add(ctx.getRexBuilder().makeCall(SqlStdOperatorTable.NOT, condition)); - } + if (condition == null) { + throw new ConversionException("Unsupported must_not clause: " + mustNotClause.getName()); + } + if (condition instanceof RexCall && ((RexCall) condition).getOperator() == SqlStdOperatorTable.NOT) { + conditions.add(((RexCall) condition).getOperands().get(0)); + } else { + conditions.add(ctx.getRexBuilder().makeCall(SqlStdOperatorTable.NOT, condition)); } } Suggestion importance[1-10]: 5 __ Why: Valid concern: silently skipping null clauses could produce semantically incorrect translations. However, the current registry design may guarantee non-null results, so throwing may be a matter of policy rather than a definite bug.	Low
	Fix short-circuit logic in multi-combination parser The multi-combination loop returns result (initialized to total) as soon as total <= threshold, but per Lucene's spec the last applicable rule (largest threshold not exceeding total) should be used. On the first iteration, if total <= threshold, this returns total (match-all) which is correct only for the initial state — but subsequent iterations may need to also short-circuit. Additionally, thresholds should be assumed to be sorted; consider validating or documenting this assumption to avoid silent misbehavior with unsorted input like "5<50% 3<-1". sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/query/BoolQueryTranslator.java [298-318] private int parseMultipleCombinations(String value, int total) throws ConversionException { String[] combinations = value.trim().split("\\s+"); int result = total; for (String combination : combinations) { String[] parts = combination.split("<"); if (parts.length < 2 || parts[1].isEmpty()) { throw new ConversionException("Malformed combination in minimum_should_match: \"" + combination + "\""); } int threshold; try { threshold = Integer.parseInt(parts[0]); } catch (NumberFormatException e) { throw new ConversionException("Invalid threshold in minimum_should_match: \"" + parts[0] + "\"", e); } if (total <= threshold) { - return result; + break; } result = parts[1].endsWith("%") ? parsePercentage(parts[1], total) : parseInteger(parts[1], total); } return result; } Suggestion importance[1-10]: 4 __ Why: The change from return result to break is functionally equivalent since result is returned after the loop anyway. The suggestion's semantic concern is not clearly demonstrated as a bug.	Low

---

Previous suggestions

Suggestions up to commit 7761f42

Category	Suggestion	Impact
General	Match legacy percentage rounding semantics The legacy Queries.calculateMinShouldMatch uses Math.round (or truncation toward zero) for percentage calculation, not Math.floor. For negative doubles, Math.floor rounds toward negative infinity which can produce different results than the legacy semantics. Verify the rounding mode matches OpenSearch's Queries.calculateMinShouldMatch exactly to avoid divergence on edge cases. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/query/BoolQueryTranslator.java [258-271] private int parsePercentage(String value, int total) throws ConversionException { String numStr = value.substring(0, value.length() - 1); try { double percent = Double.parseDouble(numStr); if (percent >= 0) { - return (int) Math.floor(total * percent / 100.0); + return (int) (total * percent / 100.0); } else { - int allowed = (int) Math.floor(total * (-percent) / 100.0); + int allowed = (int) (total * (-percent) / 100.0); return total - allowed; } } catch (NumberFormatException e) { throw new ConversionException("Invalid percentage in minimum_should_match: \"" + value + "\"", e); } } Suggestion importance[1-10]: 4 __ Why: The concern about matching legacy Queries.calculateMinShouldMatch semantics is valid, but the suggestion is speculative ("verify") and the proposed change to (int) truncation may not actually match legacy behavior either. For non-negative values, Math.floor and (int) cast behave identically.	Low
	Defer double-negation elimination to simplifier Double-negation elimination on NOT(x) -> x is semantically incorrect for three-valued logic (SQL NULL semantics): NOT NOT NULL remains NULL, but x may be NULL — these are equivalent, so the transformation is safe. However, if the inner NOT was produced from a must_not in a nested bool, the semantics differ around unknown/null field values. Consider deferring this optimization to Calcite's simplifier (RexSimplify) rather than performing it manually. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/query/BoolQueryTranslator.java [113-122] -// Must_not clauses (NOT) with double-negation elimination +// Must_not clauses (NOT); let Calcite simplifier handle double-negation elimination for (QueryBuilder mustNotClause : boolQuery.mustNot()) { RexNode condition = queryRegistry.convert(mustNotClause, ctx); if (condition != null) { - if (condition instanceof RexCall && ((RexCall) condition).getOperator() == SqlStdOperatorTable.NOT) { - conditions.add(((RexCall) condition).getOperands().get(0)); - } else { - conditions.add(ctx.getRexBuilder().makeCall(SqlStdOperatorTable.NOT, condition)); - } + conditions.add(ctx.getRexBuilder().makeCall(SqlStdOperatorTable.NOT, condition)); } } Suggestion importance[1-10]: 3 __ Why: The suggestion is stylistic and admits the current transformation is safe. Removing the optimization could produce less-flat expressions and there's an existing test (testDoubleNegationElimination) that verifies the current behavior.	Low
	Ensure match-none short-circuit is robust When requiredMatches > totalShould and the bool query has required clauses (must/filter), returning FALSE from processShouldClauses triggers the isAlwaysFalse() branch in convert(), which short-circuits to FALSE, correctly. However the FALSE literal is added to conditions before that check only if not caught — verify FALSE literal survives simplification. Consider directly returning FALSE from convert() on this branch for clarity and to avoid depending on isAlwaysFalse() detection of the literal. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/query/BoolQueryTranslator.java [174-176] -int requiredMatches = calculateRequiredMatches(minimumShouldMatch, totalShould, hasRequired); - -if (requiredMatches == 0) { - return null; // Should clauses are optional -} - -// WHY: Legacy Lucene accepts MSM > shouldCount but matches nothing (impossible constraint). -// Witnessed: BoolQueryBuilderTests.testMinShouldMatchBiggerThanNumberOfShouldClauses sets -// MSM=3 on 2 SHOULD clauses — Lucene accepts, query matches zero documents. if (requiredMatches > totalShould) { + // Impossible constraint — signal match-none to caller which returns FALSE literal return ctx.getRexBuilder().makeLiteral(false); } -List<RexNode> shouldConditions = new ArrayList<>(); -for (QueryBuilder shouldClause : shouldClauses) { - RexNode condition = queryRegistry.convert(shouldClause, ctx); - if (condition != null) { - shouldConditions.add(condition); - } -} - Suggestion importance[1-10]: 2 __ Why: The suggestion asks to "verify" behavior and the improved_code is essentially identical to the existing_code (only comment changes). The existing code already handles this correctly via the isAlwaysFalse() check in convert().	Low
Possible issue	Sort combinations by threshold for correctness The initial value of result is total, meaning if the very first threshold is already >= total, all clauses must match. However, the loop overwrites result on each iteration where total > threshold, so only the last matching combination's value is kept — but legacy semantics require the LAST rule whose threshold is exceeded to win. The current loop order works only if combinations are sorted by ascending threshold; otherwise the result depends on input order. Consider sorting combinations by threshold ascending before iteration to match documented semantics. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/query/BoolQueryTranslator.java [298-318] private int parseMultipleCombinations(String value, int total) throws ConversionException { String[] combinations = value.trim().split("\\s+"); - int result = total; + // Parse and sort by threshold ascending to match documented semantics + java.util.List<String[]> parsed = new java.util.ArrayList<>(); for (String combination : combinations) { String[] parts = combination.split("<"); if (parts.length < 2 || parts[1].isEmpty()) { throw new ConversionException("Malformed combination in minimum_should_match: \"" + combination + "\""); } - int threshold; try { - threshold = Integer.parseInt(parts[0]); + Integer.parseInt(parts[0]); } catch (NumberFormatException e) { throw new ConversionException("Invalid threshold in minimum_should_match: \"" + parts[0] + "\"", e); } + parsed.add(parts); + } + parsed.sort((a, b) -> Integer.compare(Integer.parseInt(a[0]), Integer.parseInt(b[0]))); + int result = total; + for (String[] parts : parsed) { + int threshold = Integer.parseInt(parts[0]); if (total <= threshold) { return result; } result = parts[1].endsWith("%") ? parsePercentage(parts[1], total) : parseInteger(parts[1], total); } return result; } Suggestion importance[1-10]: 3 __ Why: The claim that legacy semantics require sorting by threshold is not clearly established; OpenSearch's documented behavior processes combinations in order. The suggestion may introduce a behavioral divergence rather than fix one.	Low

Suggestions up to commit 71f2752

Category	Suggestion	Impact
Possible issue	Fix percentage rounding to match legacy semantics The legacy Queries.calculateMinShouldMatch uses Math.ceil (not Math.floor) for the non-negative percentage branch, and rounds allowed misses via Math.ceil too. Using Math.floor here causes "70%" of 4 to compute as 2 (floor(2.8)) while legacy Lucene rounds to 3 (ceil(2.8)), producing incorrect match semantics. Align rounding with legacy behavior. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/query/BoolQueryTranslator.java [260-273] private int parsePercentage(String value, int total) throws ConversionException { String numStr = value.substring(0, value.length() - 1); try { double percent = Double.parseDouble(numStr); if (percent >= 0) { - return (int) Math.floor(total * percent / 100.0); + return (int) Math.ceil(total * percent / 100.0); } else { - int allowed = (int) Math.floor(total * (-percent) / 100.0); + int allowed = (int) Math.ceil(total * (-percent) / 100.0); return total - allowed; } } catch (NumberFormatException e) { throw new ConversionException("Invalid percentage in minimum_should_match: \"" + value + "\"", e); } } Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies that legacy Queries.calculateMinShouldMatch uses different rounding (roundUp/floor combinations), and the existing tests actually assert floor(4 * 0.7) = 2. However, legacy Lucene indeed uses Math.floor for percentage (per Queries.calculateMinShouldMatch), so the claim about Math.ceil may be incorrect. Given the ambiguity but potential semantic mismatch, moderate-high impact.	Medium
	Handle literal results in must_not clauses When a nested must_not produces FALSE literal (e.g., pure-negative with adjust_pure_negative=false), wrapping it in NOT yields TRUE, which is then AND-ed with the parent's conditions—effectively dropping the must_not clause. Handle literal FALSE/TRUE explicitly: NOT(FALSE) should short-circuit correctly, but a TRUE inner result should make the whole bool match-none. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/query/BoolQueryTranslator.java [115-124] // Must_not clauses (NOT) with double-negation elimination for (QueryBuilder mustNotClause : boolQuery.mustNot()) { RexNode condition = queryRegistry.convert(mustNotClause, ctx); if (condition != null) { + if (condition.isAlwaysTrue()) { + return ctx.getRexBuilder().makeLiteral(false); + } + if (condition.isAlwaysFalse()) { + continue; + } if (condition instanceof RexCall && ((RexCall) condition).getOperator() == SqlStdOperatorTable.NOT) { conditions.add(((RexCall) condition).getOperands().get(0)); } else { conditions.add(ctx.getRexBuilder().makeCall(SqlStdOperatorTable.NOT, condition)); } } } Suggestion importance[1-10]: 6 __ Why: The suggestion identifies a real edge case where a nested must_not producing FALSE literal would incorrectly wrap to NOT(FALSE)=TRUE and be dropped. This can affect correctness but only in rare cases involving nested pure-negative bool with adjust_pure_negative=false.	Low
General	Trim input before dispatching parser branches minimumShouldMatch may contain leading/trailing whitespace (e.g. " 2" or "2 "), causing the contains(" ") branch to falsely route a single value into parseMultipleCombinations, which then throws a malformed exception. Trim the input before dispatching to the parser branches to match legacy Queries.calculateMinShouldMatch tolerance. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/query/BoolQueryTranslator.java [229-237] int result; - -if (minimumShouldMatch.contains(" ")) { - result = parseMultipleCombinations(minimumShouldMatch, totalShould); -} else if (minimumShouldMatch.contains("<")) { - result = parseCombination(minimumShouldMatch, totalShould); -} else if (minimumShouldMatch.endsWith("%")) { - result = parsePercentage(minimumShouldMatch, totalShould); -} else { - result = parseInteger(minimumShouldMatch, totalShould); +String trimmed = minimumShouldMatch.trim(); +if (trimmed.isEmpty()) { + return hasRequired ? 0 : 1; } +if (trimmed.contains(" ")) { + result = parseMultipleCombinations(trimmed, totalShould); +} else if (trimmed.contains("<")) { + result = parseCombination(trimmed, totalShould); +} else if (trimmed.endsWith("%")) { + result = parsePercentage(trimmed, totalShould); +} else { + result = parseInteger(trimmed, totalShould); +} + Suggestion importance[1-10]: 5 __ Why: Trimming whitespace improves robustness for edge cases like " 2", which would otherwise incorrectly route to parseMultipleCombinations. This is a reasonable defensive improvement but likely a minor edge case.	Low

Suggestions up to commit 7ff3be2

Category	Suggestion	Impact
Possible issue	Fix binomial coefficient exact-division invariant The overflow guard result > Long.MAX_VALUE / (n - i) checks before multiplication, but the subsequent division result * (n - i) / (i + 1) may not be exact because result * (n - i) isn't guaranteed to be divisible by (i + 1) at each intermediate step. This can produce incorrect binomial values. The standard idiom multiplies then divides while iterating i from 1 to k with result = result * (n - i + 1) / i, which is exact at each step by a mathematical invariant. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/query/BoolQueryTranslator.java [381-391] static long binomialCoefficient(int n, int k) { if (k < 0 || k > n) return 0; if (k == 0 || k == n) return 1; if (k > n - k) k = n - k; long result = 1; - for (int i = 0; i < k; i++) { - if (result > Long.MAX_VALUE / (n - i)) return Long.MAX_VALUE; - result = result * (n - i) / (i + 1); + for (int i = 1; i <= k; i++) { + if (result > Long.MAX_VALUE / (n - i + 1)) return Long.MAX_VALUE; + result = result * (n - i + 1) / i; } return result; } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that the standard exact-integer binomial computation requires the invariant result = result * (n - i + 1) / i iterating i from 1 to k to guarantee exact division at each step. The original form may produce incorrect intermediate values, though it happens to work for the C(n,k) form used here because C(n,i+1) = C(n,i) * (n-i)/(i+1) is exact. Still, this is a legitimate correctness concern worth reviewing.	Medium
General	Align percentage rounding with legacy Lucene Math.floor on a positive double truncates the same as an (int) cast, but Lucene's Queries.calculateMinShouldMatch uses integer rounding via Math.round/ceil semantics differently. More importantly, (int) Math.floor(...) on negative doubles rounds toward negative infinity, so if percent is passed as a positive fractional value that yields a negative product (not possible here) it would misbehave; however, the real issue is that this diverges from Lucene: legacy uses (int) (result * pct / 100) truncation, not floor. Align with legacy semantics to avoid off-by-one mismatches. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/query/BoolQueryTranslator.java [272-285] private int parsePercentage(String value, int total) throws ConversionException { String numStr = value.substring(0, value.length() - 1); try { double percent = Double.parseDouble(numStr); if (percent >= 0) { - return (int) Math.floor(total * percent / 100.0); + return (int) (total * percent / 100.0); } else { - int allowed = (int) Math.floor(total * (-percent) / 100.0); + int allowed = (int) (total * (-percent) / 100.0); return total - allowed; } } catch (NumberFormatException e) { throw new ConversionException("Invalid percentage in minimum_should_match: \"" + value + "\"", e); } } Suggestion importance[1-10]: 4 __ Why: For positive doubles (int) Math.floor(x) and (int) x produce the same result, and the suggestion does not clearly demonstrate a semantic divergence from Lucene. The impact is marginal and potentially incorrect.	Low
	Handle null-converted must_not in pure-negative bool The isPureNegative check earlier uses !boolQuery.mustNot().isEmpty(), but if all mustNot clauses convert to null (i.e., condition == null here), the resulting bool has no conditions and falls through to the empty-bool TRUE (match-all) branch. This silently changes semantics for pure-negative bools whose inner queries return null. Consider preserving match-none or match-all semantics explicitly instead of relying on the flattened-empty path. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/query/BoolQueryTranslator.java [127-136] // Must_not clauses (NOT) with double-negation elimination +boolean anyMustNotConverted = false; for (QueryBuilder mustNotClause : boolQuery.mustNot()) { RexNode condition = queryRegistry.convert(mustNotClause, ctx); if (condition != null) { + anyMustNotConverted = true; if (condition instanceof RexCall && ((RexCall) condition).getOperator() == SqlStdOperatorTable.NOT) { conditions.add(((RexCall) condition).getOperands().get(0)); } else { conditions.add(ctx.getRexBuilder().makeCall(SqlStdOperatorTable.NOT, condition)); } } } +if (isPureNegative && !anyMustNotConverted) { + return ctx.getRexBuilder().makeLiteral(true); +} Suggestion importance[1-10]: 3 __ Why: The edge case (all must_not clauses converting to null) is unlikely in practice given the existing translators, and the suggestion's proposed behavior (return TRUE) is debatable. Minor robustness concern.	Low
	Document short-circuit skipping must_not conversion Returning early with FALSE when the should branch is unsatisfiable skips processing of mustNot clauses, which is fine, but it also skips the adjustPureNegative / empty-bool logic below. More importantly, the short-circuit ignores any already-collected must/filter conditions — semantically correct (AND with FALSE is FALSE), but it also silently discards mustNot conversion side effects. Consider adding the FALSE literal to conditions instead of returning early so downstream flattening/short-circuit handles it uniformly, or explicitly document that mustNot conversion side effects are intentionally skipped. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/query/BoolQueryTranslator.java [115-124] // Should clauses with minimum_should_match if (!boolQuery.should().isEmpty()) { RexNode shouldCondition = processShouldClauses(boolQuery, ctx); if (shouldCondition != null) { - // Check if should processing determined match-none (FALSE literal) if (shouldCondition.isAlwaysFalse()) { + // Short-circuit: bool with unsatisfiable should → match-none regardless of other clauses return ctx.getRexBuilder().makeLiteral(false); } conditions.add(shouldCondition); } } Suggestion importance[1-10]: 2 __ Why: The improved_code is essentially identical to the existing_code with only a comment change. The suggestion offers minimal actionable improvement.	Low

Suggestions up to commit 7aa5eb0

Category	Suggestion	Impact
Possible issue	Match legacy Lucene percentage rounding semantics Legacy Queries.calculateMinShouldMatch uses Math.ceil (not floor) when applying a non-negative percentage, and computes total - (int)(total * -percent/100) (truncation, not floor) for negative percentages. Using Math.floor here diverges from Lucene semantics — e.g., 70% of 4 legacy returns ceil(2.8)=3, but this code returns floor(2.8)=2. Align with the legacy formula to preserve query semantics. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/query/BoolQueryTranslator.java [272-285] private int parsePercentage(String value, int total) throws ConversionException { String numStr = value.substring(0, value.length() - 1); try { double percent = Double.parseDouble(numStr); if (percent >= 0) { - return (int) Math.floor(total * percent / 100.0); + return (int) Math.ceil(total * percent / 100.0); } else { - int allowed = (int) Math.floor(total * (-percent) / 100.0); + int allowed = (int) (total * (-percent) / 100.0); return total - allowed; } } catch (NumberFormatException e) { throw new ConversionException("Invalid percentage in minimum_should_match: \"" + value + "\"", e); } } Suggestion importance[1-10]: 6 __ Why: The suggestion claims legacy Lucene uses Math.ceil, but actual Lucene Queries.calculateMinShouldMatch uses floor via (int)(optionalClauseCount * pct / 100.0). However, the tests in this PR document floor semantics (e.g., 70% of 4 = 2), so the suggested change would break the existing test expectations. The concern is worth verifying against legacy code but the specific fix may be incorrect.	Low
General	Make CASE NULL-to-zero mapping explicit Calcite's CASE operator typically requires the condition operand to be boolean-typed and the SEARCH/THEN/ELSE operands to be paired properly. Additionally, if cond can evaluate to SQL NULL, CASE WHEN NULL THEN 1 ELSE 0 yields 0 as intended, but some Calcite validators reject CASE with a nullable boolean without an explicit IS TRUE wrapper. Consider wrapping cond with IS TRUE (or using COALESCE(cond, FALSE)) to make NULL-to-0 mapping explicit and portable across Calcite validators. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/query/BoolQueryTranslator.java [360-371] private RexNode createCountingExpression(List<RexNode> conditions, int required, ConversionContext ctx) { RexBuilder rb = ctx.getRexBuilder(); RelDataType intType = rb.getTypeFactory().createSqlType(SqlTypeName.INTEGER); RexNode one = rb.makeExactLiteral(BigDecimal.ONE, intType); RexNode zero = rb.makeExactLiteral(BigDecimal.ZERO, intType); - // Left-folded PLUS chain of CASE expressions RexNode sum = null; for (RexNode cond : conditions) { - RexNode caseTerm = rb.makeCall(SqlStdOperatorTable.CASE, cond, one, zero); + RexNode safeCond = rb.makeCall(SqlStdOperatorTable.IS_TRUE, cond); + RexNode caseTerm = rb.makeCall(SqlStdOperatorTable.CASE, safeCond, one, zero); sum = (sum == null) ? caseTerm : rb.makeCall(SqlStdOperatorTable.PLUS, sum, caseTerm); } Suggestion importance[1-10]: 5 __ Why: Wrapping the predicate with IS TRUE makes NULL handling explicit and more portable, which aligns with the documented intent that NULL predicates contribute 0. Reasonable defensive improvement though the current form likely works.	Low
	Short-circuit bool to FALSE on any always-false clause The pure-negative match-none branch returns FALSE before evaluating should clauses, but when should is present with an unsatisfiable MSM the code correctly returns FALSE here. However, if there are also must_not clauses that would still produce conditions, the early return correctly short-circuits. Consider also handling the case where must/filter produced isAlwaysFalse() conditions to short-circuit the entire bool to FALSE, avoiding building unnecessary AND expressions around a constant false. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/query/BoolQueryTranslator.java [115-124] // Should clauses with minimum_should_match if (!boolQuery.should().isEmpty()) { RexNode shouldCondition = processShouldClauses(boolQuery, ctx); if (shouldCondition != null) { - // Check if should processing determined match-none (FALSE literal) if (shouldCondition.isAlwaysFalse()) { return ctx.getRexBuilder().makeLiteral(false); } conditions.add(shouldCondition); } } +for (RexNode c : conditions) { + if (c.isAlwaysFalse()) { + return ctx.getRexBuilder().makeLiteral(false); + } +} Suggestion importance[1-10]: 3 __ Why: Minor optimization that adds redundant traversal; the existing code already handles the primary match-none case via processShouldClauses. Marginal benefit without demonstrated need.	Low
	Clarify early-return value in multi-spec parser When the first combination's threshold satisfies total <= threshold, the method returns result = total (match all), but legacy semantics say specs before the first satisfied threshold do not apply — the default is to match all only if no spec matches at all. If the very first spec has total <= threshold, legacy behavior is to require all (which equals total), so this happens to be correct. However, result is initialized to total and never updated before the early return, which may mask bugs; consider explicitly returning total for clarity, and ensuring specs are processed in ascending threshold order per Lucene's contract (or documenting that the caller must order them). sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/query/BoolQueryTranslator.java [312-332] private int parseMultipleCombinations(String value, int total) throws ConversionException { String[] combinations = value.trim().split("\\s+"); int result = total; for (String combination : combinations) { String[] parts = combination.split("<"); if (parts.length < 2 || parts[1].isEmpty()) { throw new ConversionException("Malformed combination in minimum_should_match: \"" + combination + "\""); } int threshold; try { threshold = Integer.parseInt(parts[0]); } catch (NumberFormatException e) { throw new ConversionException("Invalid threshold in minimum_should_match: \"" + parts[0] + "\"", e); } if (total <= threshold) { - return result; + return total; } result = parts[1].endsWith("%") ? parsePercentage(parts[1], total) : parseInteger(parts[1], total); } return result; } Suggestion importance[1-10]: 2 __ Why: Minor readability improvement; the functional behavior is unchanged since result is initialized to total before the early return. Low impact.	Low

Suggestions up to commit f48e299

Category	Suggestion	Impact
Possible issue	Fix binomial coefficient integer division The integer division result * (n - i) / (i + 1) can silently produce incorrect binomial coefficients because result * (n - i) is not always divisible by (i + 1) at each step. The standard safe formulation multiplies then divides using the invariant that C(n,i+1) is integral; use a running accumulator that divides by the gcd, or compute using the standard iterative form which is only guaranteed correct when the loop uses result = result * (n - k + i + 1) / (i + 1) in ascending order. Verify or fix to prevent wrong cap decisions. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/query/BoolQueryTranslator.java [357-362] long result = 1; -for (int i = 0; i < k; i++) { - if (result > Long.MAX_VALUE / (n - i)) return Long.MAX_VALUE; - result = result * (n - i) / (i + 1); +for (int i = 1; i <= k; i++) { + if (result > Long.MAX_VALUE / (n - k + i)) return Long.MAX_VALUE; + result = result * (n - k + i) / i; } return result; Suggestion importance[1-10]: 6 __ Why: The concern about integer division correctness is a valid consideration, though the existing formulation result * (n - i) / (i + 1) is actually a well-known correct iterative binomial computation because the partial product is divisible at each step. The suggestion offers a mathematically equivalent alternative.	Low
	Match Lucene percentage rounding semantics Legacy Queries.calculateMinShouldMatch uses Math.round (not Math.floor) for non-negative percentages and rounds the "allowed misses" for negative percentages similarly. Using Math.floor here diverges from Lucene semantics — e.g., "70%" of 4 clauses should yield 3 (round(2.8)), not 2. This changes matching behavior versus the legacy engine. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/query/BoolQueryTranslator.java [260-273] private int parsePercentage(String value, int total) throws ConversionException { String numStr = value.substring(0, value.length() - 1); try { double percent = Double.parseDouble(numStr); if (percent >= 0) { - return (int) Math.floor(total * percent / 100.0); + return (int) Math.round(total * percent / 100.0); } else { - int allowed = (int) Math.floor(total * (-percent) / 100.0); + int allowed = (int) Math.round(total * (-percent) / 100.0); return total - allowed; } } catch (NumberFormatException e) { throw new ConversionException("Invalid percentage in minimum_should_match: \"" + value + "\"", e); } } Suggestion importance[1-10]: 4 __ Why: Legacy Lucene Queries.calculateMinShouldMatch actually uses (int)(total * percent / 100) which truncates (equivalent to floor for positives), not Math.round. The suggestion's claim about Math.round is incorrect, so applying it would diverge from Lucene semantics rather than match them.	Low
General	Fix must_not NULL semantics vs Lucene NULL handling for must_not differs from Lucene semantics: NOT(expr) in SQL returns NULL when expr is NULL, but Lucene's must_not excludes only documents that match — documents where the clause is unknown/NULL should still be kept. Consider wrapping with IS NOT TRUE (or COALESCE(NOT expr, TRUE)) so NULLs are treated as non-matching and thus not excluded. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/query/BoolQueryTranslator.java [115-124] // Must_not clauses (NOT) with double-negation elimination for (QueryBuilder mustNotClause : boolQuery.mustNot()) { RexNode condition = queryRegistry.convert(mustNotClause, ctx); if (condition != null) { + RexNode negated; if (condition instanceof RexCall && ((RexCall) condition).getOperator() == SqlStdOperatorTable.NOT) { - conditions.add(((RexCall) condition).getOperands().get(0)); + negated = ((RexCall) condition).getOperands().get(0); } else { - conditions.add(ctx.getRexBuilder().makeCall(SqlStdOperatorTable.NOT, condition)); + negated = ctx.getRexBuilder().makeCall(SqlStdOperatorTable.IS_NOT_TRUE, condition); } + conditions.add(negated); } } Suggestion importance[1-10]: 6 __ Why: This is a valid semantic concern: SQL NOT(NULL) yields NULL which would drop documents with missing fields, whereas Lucene's must_not only excludes matching documents. Using IS NOT TRUE would better preserve Lucene semantics, though the fix might affect other clauses similarly.	Low

[github-actions]

✅ Gradle check result for 5927272: SUCCESS

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.37%. Comparing base (03a4de3) to head (6403717).

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #22604      +/-   ##
============================================
- Coverage     71.42%   71.37%   -0.05%     
+ Complexity    76786    76776      -10     
============================================
  Files          6148     6148              
  Lines        357980   357980              
  Branches      52177    52177              
============================================
- Hits         255689   255511     -178     
- Misses        81937    82097     +160     
- Partials      20354    20372      +18     

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

[github-actions]

✅ Gradle check result for e9675cb: SUCCESS

[github-actions]

Persistent review updated to latest commit f48e299

[github-actions]

Persistent review updated to latest commit 7aa5eb0

[github-actions]

Persistent review updated to latest commit 7ff3be2

[github-actions]

Persistent review updated to latest commit 71f2752

[github-actions]

❌ Gradle check result for 71f2752: 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]

Persistent review updated to latest commit 7761f42

[github-actions]

Persistent review updated to latest commit 6403717

[github-actions]

✅ Gradle check result for 6403717: SUCCESS