Default plugins.calcite.enabled=true on the unified query path

[ahkcs]

Summary

Companion to opensearch-project/OpenSearch#21521 (sort/aggregation/eval-predicate scalar surface) and opensearch-project/OpenSearch#21498 (eval string concat / CAST). Two changes — both strictly scoped to PPL fields / rename / head / sort; aggregation work (AVG / STDDEV_* / VAR_* / stats surface) is explicitly out of scope.

1. UnifiedQueryContext — default plugins.calcite.enabled=true

The unified PPL parser reuses the v2 AstBuilder, which gates table / regex / rex / convert on this setting. The unified context built empty maps without it, so analytics-routed table queries 500'd with Table command is supported only when plugins.calcite.enabled=true even when the cluster setting was true.

Per @dai-chen's review: the default goes in the Map.of(...) initializer at line 123 (alongside QUERY_SIZE_LIMIT/PPL_SUBSEARCH_MAXOUT/PPL_JOIN_SUBSEARCH_MAXOUT), not as a special case in getSettingValue.

2. CalcitePPLRenameIT — column-name-keyed row matcher

Previously, verifyStandardDataRows and a handful of bespoke verifyDataRows calls compared rows positionally — they assumed the engine emits columns in the v2 / Lucene _source iteration order. Under the analytics-engine route the parquet-backed reader returns columns in storage order, so the same 4 canonical state_country rows came back as [70,"USA",4,"Jake","California",2023] instead of [Jake,USA,California,4,2023,70] and 12 of 22 tests failed despite the data being identical.

Replace with verifyStandardDataRows(result, canonicalColumns...) plus a generic verifyDataRowsByColumn(result, Map<String, Object>...) helper. Both read the actual schema from the response, look up each canonical value by column name, and place it at the corresponding schema position before comparing — the test no longer leaks the engine's emission order into the assertion.

Verified working on both paths: 20 / 22 pass on each. The 2 remaining failures (testRenameInAgg, testRenameWithBackticksInAgg) are the Unable to find binding for call AVG aggregation issue — out of scope for this PR; tracked in #21521's "Out of scope" section.

Test plan

• ./gradlew :api:check green
• Manual: with plugins.calcite.analytics.force_routing=true, POST /_plugins/_ppl {\"query\":\"source=test_eval | table name\"} no longer 500s with the plugins.calcite.enabled=true error
• :integ-test:integTestRemote against an analytics cluster carrying #21521 + #21498, both with and without -Dtests.analytics.force_routing=true -Dtests.analytics.parquet_indices=true:

IT	Analytics path	v2 / Calcite path	Notes
CalciteFieldsCommandIT	39 / 39 ✅	39 / 39 ✅	scope of this PR
CalciteRenameCommandIT	2 / 2 ✅	2 / 2 ✅	scope of this PR
CalciteHeadCommandIT	4 / 4 ✅ (2 skipped)	4 / 4 ✅	scope of this PR
CalcitePPLRenameIT	20 / 22	20 / 22	column-order helper added by this PR; remaining 2 are AVG-binding (out of scope)
CalciteEvalCommandIT	4 / 4 ✅	4 / 4 ✅	scope of #21498
CalcitePPLSortIT	14 / 18	18 / 18 ✅	4 analytics-side fails on tie-break ordering / date-format diff — separate follow-up
CalciteSortCommandIT	26 / 30	30 / 30 ✅	4 analytics-side fails on weblogs IP-type scan — separate follow-up

Companion routing-and-coverage workflow docs

The docs/dev/ppl-analytics-engine-{routing,coverage-sop}.md files originally bundled with the first revision of this PR were dropped per @dai-chen's review — they belong on a separate doc PR.

Out of scope

Aggregation surface — AVG, SUM, COUNT, STDDEV_*, VAR_*, the stats family, count(eval(...)), etc. — is not addressed by this PR. The Unable to find binding for call AVG($N) Substrait isthmus issue is a cross-plugin design discussion (the SQL plugin's NullableSqlAvgAggFunction extends SqlAggFunction directly rather than SqlAvgAggFunction, so isthmus' built-in translator skips it). Tracked in #21521.

[github-actions]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 4ef37bd.

Path	Line	Severity	Description
api/src/main/java/org/opensearch/sql/api/UnifiedQueryContext.java	230	medium	Hard-coded override forces CALCITE_ENGINE_ENABLED to true when the key is absent from settings, silently enabling the Calcite routing path regardless of administrator configuration intent. A missing key typically means the feature is disabled or unset; returning TRUE instead bypasses that default-off contract and could route queries through a non-expected engine path without any visible configuration change.
docs/dev/ppl-analytics-engine-coverage-sop.md	16	medium	Documentation directs developers to add a personal GitHub fork (ahkcs/sql) as a named remote and to base production-relevant work on ahkcs/feature/ppl-coverage-bundle. Establishing a personal fork as a sanctioned part of the official development workflow creates an unverified trust anchor — code or tooling pulled from that branch bypasses normal upstream review, making it a potential supply chain injection point.
docs/dev/ppl-analytics-engine-coverage-sop.md	38	low	SOP instructs developers to manually add 'runtimeOnly org.apache.commons:commons-text:1.11.0' directly to a production sandbox build file outside the normal PR/review process. While framed as a workaround pending an upstream fix, the instruction socially engineers an unreviewed dependency addition into a production artifact.
docs/dev/ppl-analytics-engine-routing.md	420	low	A specific commit on the personal fork (ahkcs/sql@29339c9c) is cited as the 'canonical reference for the SQL-side test infra'. Designating an unverifiable personal-fork commit as the authoritative source for infrastructure patterns encourages developers to replicate code from outside the reviewed upstream history.

The table above displays the top 10 most important findings.

Total: 4 | Critical: 0 | High: 0 | Medium: 2 | Low: 2

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 4ade92f)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 Multiple PR themes Sub-PR theme: Default CALCITE_ENGINE_ENABLED=true in UnifiedQueryContext Relevant files: api/src/main/java/org/opensearch/sql/api/UnifiedQueryContext.java Sub-PR theme: Column-order-agnostic rename integration test helpers Relevant files: integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLRenameIT.java

⚡ Recommended focus areas for review Hardcoded Default CALCITE_ENGINE_ENABLED is hardcoded to true in the UnifiedQueryContext.Builder settings map. If a caller explicitly passes false for this key (e.g., to disable Calcite for a specific unified-path request), the hardcoded default will be overridden by that caller's value — but there is no mechanism to propagate the actual cluster setting into this context. This means the unified path always behaves as if Calcite is enabled, regardless of any runtime configuration that might want to disable it. This should be validated as an intentional design decision and documented accordingly. private final Map<Settings.Key, Object> settings = new HashMap<Settings.Key, Object>( Map.of( QUERY_SIZE_LIMIT, SysLimit.DEFAULT.querySizeLimit(), PPL_SUBSEARCH_MAXOUT, SysLimit.DEFAULT.subsearchLimit(), PPL_JOIN_SUBSEARCH_MAXOUT, SysLimit.DEFAULT.joinSubsearchLimit(), CALCITE_ENGINE_ENABLED, true)); Unchecked Cast In verifyStandardDataRows, new LinkedHashMap[canonicalValues.length] creates a raw array of LinkedHashMap and is assigned to Map<String, Object>[] expectedRows. This produces an unchecked cast warning and is not type-safe. Consider using a List<Map<String, Object>> instead to avoid raw array creation. Map<String, Object>[] expectedRows = new LinkedHashMap[canonicalValues.length]; for (int i = 0; i < canonicalValues.length; i++) { Map<String, Object> row = new LinkedHashMap<>(); for (int c = 0; c < canonicalColumns.length; c++) { row.put(canonicalColumns[c], canonicalValues[i][c]); } expectedRows[i] = row; } Missing Row Count Check verifyDataRowsByColumn does not assert that the total number of rows in the response matches the number of expected rows. If the response returns extra rows, the test will still pass as long as the expected rows are present. Consider adding a check that result.getJSONArray("datarows").length() == expectedRows.length. JSONObject result, Map<String, Object>... expectedRows) { JSONArray schema = result.getJSONArray("schema"); int n = schema.length(); String[] columnOrder = new String[n]; for (int i = 0; i < n; i++) { columnOrder[i] = schema.getJSONObject(i).getString("name"); } @SuppressWarnings({"unchecked", "rawtypes"}) Matcher<JSONArray>[] rowMatchers = new Matcher[expectedRows.length]; for (int r = 0; r < expectedRows.length; r++) { Object[] reordered = new Object[n]; for (int c = 0; c < n; c++) { if (!expectedRows[r].containsKey(columnOrder[c])) { throw new IllegalArgumentException( "Expected row at index " + r + " is missing canonical value for response column [" + columnOrder[c] + "]; provided keys: " + expectedRows[r].keySet()); } reordered[c] = expectedRows[r].get(columnOrder[c]); } rowMatchers[r] = rows(reordered); } verifyDataRows(result, rowMatchers);

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 4ade92f
Explore these optional code suggestions:

Category	Suggestion	Impact
General	Validate key types to prevent silent runtime errors The cast (String) pairs[i] will throw a ClassCastException at runtime if a non-String key is accidentally passed. Since the method signature accepts Object..., this is a silent misuse risk. Consider adding an explicit check or changing the signature to alternate String, Object pairs. integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLRenameIT.java [379-388] private static Map<String, Object> rowOf(Object... pairs) { if (pairs.length % 2 != 0) { throw new IllegalArgumentException("rowOf expects an even number of args (key, value, ...)"); } Map<String, Object> row = new LinkedHashMap<>(); for (int i = 0; i < pairs.length; i += 2) { + if (!(pairs[i] instanceof String)) { + throw new IllegalArgumentException("rowOf expects String keys, but got: " + pairs[i]); + } row.put((String) pairs[i], pairs[i + 1]); } return row; } Suggestion importance[1-10]: 3 __ Why: The suggestion adds a runtime type check for keys in rowOf, which is a defensive improvement. However, since this is a test helper method called only with string literals in the same file, the practical risk of a non-String key being passed is very low.	Low
	Suppress unchecked cast warning explicitly The array is declared as Map<String, Object>[] but instantiated as a raw LinkedHashMap[], which causes an unchecked cast warning and could mask type errors. Use a consistent generic type or suppress with a comment explaining the limitation. integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLRenameIT.java [423] +@SuppressWarnings("unchecked") Map<String, Object>[] expectedRows = new LinkedHashMap[canonicalValues.length]; Suggestion importance[1-10]: 2 __ Why: The suggestion is valid but minor — the existing code already has @SuppressWarnings annotations nearby (lines 451-452), and this is a test utility method. The improvement is cosmetic and has minimal impact on correctness or functionality.	Low

---

Previous suggestions

Suggestions up to commit 4ef37bd

Category	Suggestion	Impact
General	Use equals() for key comparison safety The current code uses reference equality (==) to compare Key enum values, which works for enums but is fragile if Key is ever not an enum or if the comparison needs to handle subclasses. More importantly, the settings.containsKey(key) check is done but then settings.get(key) is called unconditionally for all other keys — if settings can return null for a key that is present but mapped to null, the behavior is correct, but the intent is clearer with .equals() for non-enum types. Verify that Key is indeed an enum; if it is, the == is fine, but add a comment clarifying this assumption. api/src/main/java/org/opensearch/sql/api/UnifiedQueryContext.java [233-236] -if (key == Key.CALCITE_ENGINE_ENABLED && !settings.containsKey(key)) { +if (Key.CALCITE_ENGINE_ENABLED.equals(key) && !settings.containsKey(key)) { return (T) Boolean.TRUE; } return (T) settings.get(key); Suggestion importance[1-10]: 2 __ Why: The suggestion to use .equals() over == for enum comparison is generally unnecessary since Java enums are singletons and == is the idiomatic and correct way to compare them. The suggestion itself acknowledges this (if it is, the == is fine), making the change purely cosmetic with no real correctness benefit.	Low

[dai-chen]

Either add it to default setting list or pass via setting() API since only unified PPL require this?

Ref:

1. https://github.com/opensearch-project/sql/blob/main/api/src/main/java/org/opensearch/sql/api/UnifiedQueryContext.java#L123
2. https://github.com/opensearch-project/sql/blob/main/integ-test/src/test/java/org/opensearch/sql/api/UnifiedQueryOpenSearchIT.java#L51

[ahkcs]

Updated

[github-actions]

Persistent review updated to latest commit 1d4c113

[github-actions]

Persistent review updated to latest commit 4ade92f