dsl-query-executor: route _search through DslCalciteGrammar with codec fallback

[aggarwalmayank]

Description

Introduces a request-level routing gate between the DSL request and the Calcite execution path in the dsl-query-executor sandbox plugin.

Today the plugin's SearchActionFilter unconditionally intercepts every _search request and dispatches it to DslExecuteAction. This change
adds two layers of gating so requests the Calcite path can't handle (or shouldn't handle yet) fall back to the codec path transparently.

Design

Layer 1 — master switch (dsl.query_executor.calcite.enabled, dynamic, default true): cluster-scoped setting; when false, every request
passes through to the codec path unchanged. Serves as an operational escape hatch — a persistent PUT _cluster/settings update flips routing
without redeploy.

Layer 2 — request-shape whitelist (DslCalciteGrammar): walks the SearchSourceBuilder and classifies it. The gate is registry-lookup +
per-parameter checks:

• Registry-lookup — queryRegistry.hasTranslator(class) / aggRegistry.hasTranslator(class). Any query or aggregation whose class isn't
  registered rejects immediately with a short reason code (query:match, agg:cardinality, pipeline_agg:cumulative_sum, etc.).
• Per-parameter — for registered types with known request-shape restrictions, the grammar mirrors the translator's rejects to route to codec
  early instead of failing at conversion time. E.g. range.boost, range.name, range.no_bounds; terms.terms_lookup, terms.boost,
  terms.no_values; exists.boost; prefix.boost, prefix.rewrite; wildcard.boost, wildcard.rewrite.
• Compound queries — bool and constant_score are transparent to the registry; the walker recurses into their children.
• Aggregations — walker rejects all pipeline aggs (no Calcite equivalent). Per-aggregation parameter checks are stubbed as TODO until each
  response translator's exact request-shape support is reviewed.

Fallback — ConversionException at execution time: schema-side checks the grammar can't run (field existence, binary-field guards,
date_nanos precision, etc.) may still fail during SearchSourceConverter conversion. SearchActionFilter wraps the DslExecuteAction
listener; on any ConversionException (including wrapped causes, guarded by a seen-set traversal), the request retries via chain.proceed(...)
to the codec path. Non-conversion errors (engine timeouts, runtime failures) surface to the caller so they can be diagnosed.

Behavior matrix

Setting	Grammar	Calcite exec	Result
off	—	—	codec
on	reject	—	codec
on	accept	ConversionException	codec
on	accept	success	Calcite response
on	accept	other error	surfaced to caller

Additional cleanup

Registry factories (QueryRegistryFactory, AggregationRegistryFactory) now return process-wide singletons. The grammar (long-lived) and the
per-request SearchSourceConverter share a single populated map instead of each allocating their own translator instances.

Check List

• [ X] 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 9b52c28)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

📝 TODO sections sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/action/SearchActionFilter.java [97]: add support for other search-related APIs (_msearch, _count, _search_shards, etc.). sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/aggregation/AggregationRegistryFactory.java [37]: add other aggregation translators sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/query/QueryRegistryFactory.java [32]: add other query translators sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/router/DslCalciteGrammar.java [248]: Replace blanket pipeline reject with registry + per-type checks. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/router/DslCalciteGrammar.java [265]: Per-parameter checks per aggregation type.

🔀 No multiple PR themes

⚡ Recommended focus areas for review Response Type Cast Assumption The wrapped listener casts listener to ActionListener<SearchResponse> unconditionally. Since the filter is generic over Response, this relies on the invariant that any request routed here is a SearchRequest producing a SearchResponse. While the current guard on SearchAction.NAME enforces this today, the unchecked cast could hide a ClassCastException if the filter is ever invoked for other actions that share the same name namespace. Consider using the raw listener directly or documenting the invariant. ActionListener<SearchResponse> calciteListener = ActionListener.wrap( (SearchResponse resp) -> ((ActionListener<SearchResponse>) listener).onResponse(resp), error -> { if (isFallbackable(error)) { logger.debug("Calcite path threw ConversionException, falling back to codec", error); chain.proceed(task, action, request, listener); } else { listener.onFailure(error); } } ); client.execute(DslExecuteAction.INSTANCE, searchRequest, calciteListener);

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 9b52c28
Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Avoid double-invocation on listener errors If listener.onResponse(resp) throws, ActionListener.wrap will invoke the failure handler with that same exception, which could then trigger chain.proceed and deliver a second response/failure to the same listener. Guard against this by delivering the response outside the wrap (or by ensuring onResponse cannot re-enter the failure branch), so a downstream listener exception does not cause double invocation. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/action/SearchActionFilter.java [112-123] -ActionListener<SearchResponse> calciteListener = ActionListener.wrap( - (SearchResponse resp) -> ((ActionListener<SearchResponse>) listener).onResponse(resp), - error -> { +ActionListener<SearchResponse> calciteListener = new ActionListener<>() { + @Override + public void onResponse(SearchResponse resp) { + ((ActionListener<SearchResponse>) listener).onResponse(resp); + } + @Override + public void onFailure(Exception error) { if (isFallbackable(error)) { logger.debug("Calcite path threw ConversionException, falling back to codec", error); chain.proceed(task, action, request, listener); } else { listener.onFailure(error); } } -); +}; client.execute(DslExecuteAction.INSTANCE, searchRequest, calciteListener); Suggestion importance[1-10]: 7 __ Why: Valid concern: ActionListener.wrap routes exceptions from onResponse into onFailure, which here could cause chain.proceed to be called after the listener was already invoked, resulting in double-invocation. The suggested fix using an explicit ActionListener avoids this trap.	Medium
General	Guard null inner query in constant_score ConstantScoreQueryBuilder.innerQuery() can be null (or the builder may carry a non-default boost that the underlying translator cannot handle). Guard against a null inner query to prevent an NPE during validation, and consider checking csq.boost() similarly to other per-parameter checks. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/router/DslCalciteGrammar.java [103-105] case ConstantScoreQueryBuilder csq -> { + if (csq.innerQuery() == null) { + return reject("constant_score.no_inner", issues); + } + if (csq.boost() != AbstractQueryBuilder.DEFAULT_BOOST) { + return reject("constant_score.boost", issues); + } return visitQuery(csq.innerQuery(), issues); } Suggestion importance[1-10]: 5 __ Why: A null innerQuery() in ConstantScoreQueryBuilder would cause an NPE during validation; the guard is a reasonable defensive check, though it's unclear if such a builder can be constructed in practice.	Low
	Avoid global mutable singleton registry Turning the registry into a process-wide singleton means tests that mutate the registry (e.g., via register(...)) or override the same class across builds will leak state across test cases, and prevents multiple plugin instances/tests from using distinct registries. Consider keeping create() returning a fresh instance, or expose both a shared and a fresh-builder factory method. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/aggregation/AggregationRegistryFactory.java [25-26] -/** Built once at class init and cached forever. */ -private static final AggregationRegistry INSTANCE = build(); +/** Returns a freshly built registry. */ +public static AggregationRegistry create() { + return build(); +} Suggestion importance[1-10]: 3 __ Why: The concern about test isolation is valid but the PR explicitly documents the registry as effectively immutable after class init. This is a design tradeoff rather than a bug.	Low

---

Previous suggestions

Suggestions up to commit 556c612

Category	Suggestion	Impact
Possible issue	Reject unsupported bool query parameters BoolQueryBuilder also supports minimum_should_match, adjust_pure_negative, boost, and _name parameters that may affect semantics but are silently ignored here. If the Calcite translator doesn't honor these, requests carrying them will be routed to Calcite and produce incorrect results instead of falling back. Consider adding explicit rejects for these parameters mirroring the pattern used for other query types. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/router/DslCalciteGrammar.java [178-182] private boolean visitBool(BoolQueryBuilder b, List<String> issues) { + if (b.boost() != AbstractQueryBuilder.DEFAULT_BOOST) { + return reject(BoolQueryBuilder.NAME + ".boost", issues); + } + if (b.queryName() != null) { + return reject(BoolQueryBuilder.NAME + ".name", issues); + } + if (b.minimumShouldMatch() != null) { + return reject(BoolQueryBuilder.NAME + ".minimum_should_match", issues); + } return Stream.of(b.must(), b.filter(), b.should(), b.mustNot()) .flatMap(List::stream) .allMatch(inner -> visitQuery(inner, issues)); } Suggestion importance[1-10]: 7 __ Why: Valid concern: minimum_should_match, boost, and _name on bool queries could produce incorrect results if the translator ignores them. Adding these rejects preserves the grammar's safety invariant, though the exact translator behavior should be verified.	Medium
General	Guard synchronous fallback against exceptions The lambda passed to ActionListener.wrap casts error (a Throwable) to Exception implicitly via listener.onFailure, but isFallbackable accepts Throwable. More critically, ActionListener.wrap's failure handler signature takes Exception, so if a non-Exception Throwable (e.g. Error) propagates, it may not reach this handler at all. Consider ensuring the fallback path also handles the case where chain.proceed itself throws synchronously, which would leave listener never notified. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/action/SearchActionFilter.java [112-123] ActionListener<SearchResponse> calciteListener = ActionListener.wrap( (SearchResponse resp) -> ((ActionListener<SearchResponse>) listener).onResponse(resp), error -> { if (isFallbackable(error)) { logger.debug("Calcite path threw ConversionException, falling back to codec", error); - chain.proceed(task, action, request, listener); + try { + chain.proceed(task, action, request, listener); + } catch (Exception chainErr) { + chainErr.addSuppressed(error); + listener.onFailure(chainErr); + } } else { listener.onFailure(error); } } ); client.execute(DslExecuteAction.INSTANCE, searchRequest, calciteListener); Suggestion importance[1-10]: 4 __ Why: Wrapping chain.proceed in a try/catch adds defensive handling for a synchronous throw during fallback, but this is an edge case and OpenSearch's action chain typically routes failures via the listener. Minor robustness improvement.	Low
	Enforce registry immutability post-init Making the registry a process-wide singleton means the AggregationRegistry.register() method (which mutates the internal map) becomes reachable from any code holding the shared instance, breaking the "effectively immutable after class init" invariant. Consider returning an unmodifiable view or documenting/enforcing that register must not be called after build() completes to avoid accidental cross-test or cross-plugin pollution. sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/aggregation/AggregationRegistryFactory.java [25-26] -/** Built once at class init and cached forever. */ +/** Built once at class init and cached forever. Immutable after construction. */ private static final AggregationRegistry INSTANCE = build(); Suggestion importance[1-10]: 2 __ Why: The improved_code only changes the comment and does not actually enforce immutability. The suggestion is largely a no-op documentation tweak.	Low

[github-actions]

Persistent review updated to latest commit 9b52c28

[github-actions]

✅ Gradle check result for 9b52c28: SUCCESS

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.42%. Comparing base (d19f68e) to head (9b52c28).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #22597      +/-   ##
============================================
- Coverage     71.43%   71.42%   -0.02%     
+ Complexity    76760    76745      -15     
============================================
  Files          6142     6142              
  Lines        357766   357794      +28     
  Branches      52148    52162      +14     
============================================
- Hits         255581   255556      -25     
- Misses        81861    81884      +23     
- Partials      20324    20354      +30     

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