Remove DatetimeOutputCastRule on the analytics-engine route (#5420)

[mengweieric]

Problem

On the analytics-engine route, every datetime root column is wrapped in CAST(<DATE/TIME/TIMESTAMP> AS VARCHAR) by DatetimeOutputCastRule. A matching DatetimeOutputCastRewriter on the engine side then translates that cast into DataFusion's to_char extension. Whenever the rewriter's format string and the PPL formatter disagree (e.g. trailing Z, T separator), users see wire-format divergence — issue #5420.

Solution

Drop the cast rule and let the engine return real datetime cells. The PPL response pipeline already handles datetime → string conversion natively at the formatter layer:

• AnalyticsExecutionEngine.convertRows feeds engine cells (LocalDateTime / LocalDate / LocalTime) through ExprValueUtils.fromObjectValue, which produces ExprTimestampValue / ExprDateValue / ExprTimeValue.
• Their value() methods render the documented PPL space-separated format (yyyy-MM-dd HH:mm:ss[.SSSSSSSSS] etc.).

The companion change in opensearch-project/OpenSearch#21748 removes the matching DatetimeOutputCastRewriter and its callsites.

Changes

• Delete DatetimeOutputCastRule.
• Drop the unused UdtMapping.isDatetimeType helper.
• Rewrite the four cast-shape assertions in DatetimeExtensionTest to assert the post-removal RelNode (no CAST(... VARCHAR) wrapper, schema reports DATE / TIME / TIMESTAMP).
• Add CalciteAnalyticsDatetimeWireFormatIT as the wire-format regression net.

Verification

1. Unit tests — RelNode shape + return-type assertions

api/src/test/java/org/opensearch/sql/api/spec/datetime/DatetimeExtensionTest.java (PPL V3 parser path) covers:

Test	What it pins
testUdfResultNormalized	DATE/TIME/TIMESTAMP UDFs produce standard Calcite types, no CAST(... VARCHAR) wrapper
testNestedUdfCallsNormalized	UDT normalization survives through nested calls (DATEDIFF(DATE(name), DATE(name)))
testDateLiteralNormalized	Literal DATE('2024-01-01') returns DATE, not VARCHAR
testFilterWithTimestampLiteral	WHERE created_at > "..." keeps TIMESTAMP end-to-end
testComparisonWithDatetimeUdf	Mixed-type comparison promotes DATE → TIMESTAMP, never VARCHAR
testStandardDatetimeFieldsNotWrapped	Direct datetime field projection — no projection wrapper at all
testNonDatetimeFieldsNotWrapped	Non-datetime fields are untouched (regression guard)
testDatetimeFieldsPreserveStandardTypes	End-to-end JDBC schema check — compiles + executes a plan, asserts ResultSetMetaData reports java.sql.Types.DATE / TIME / TIMESTAMP, not VARCHAR

DatetimeExtensionSqlTest covers the same shape on the SQL V2 parser path (3 cases).

CalcitePlannerConcurrencyIT (added in #5458) is preserved — its testSequentialPlanCallsDoNotCorruptShuttleStack case is a regression guard for the singleton-shuttle bug and exercises the post-analysis pipeline that this PR slims down.

2. Integration test — CalciteAnalyticsDatetimeWireFormatIT

integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteAnalyticsDatetimeWireFormatIT.java provisions a parquet-backed index on the force-routed analytics-engine cluster and asserts:

• Schema labels are timestamp / date / time (never string).
• Row values are PPL space-separated (2024-03-15 10:30:00), never ISO T-separator or trailing Z.

3. Curl wire-format probes (manual, pre-PR)

Driver: /tmp/ae-curl-verify/run-verify.py against a force-routed runTask cluster on parquet_wire_format_dt. Coverage:

• Routing proofs — _explain shows LogicalTableScan + lowercase opensearch (AE route) on the parquet index, and CalciteLogicalIndexScan + capital OpenSearch on the legacy probe index, confirming routing is binary.
• No CAST(... VARCHAR) — _explain of fields ts, fields d, t, and sort ts | fields ts contain neither CAST(... VARCHAR) nor to_char.
• 11 pinned wire-format queries — equality filter, projection, eval-rebind, min/max/stats, > filter, sort, nanosecond precision, dc(). Each asserts schema label and PPL space-separator value.
• 7 broader queries — where+fields, head+fields, stats min/max, group by date, dc(d), multi-column projection sorted by ts, nanos > filter.

All 18 wire-format probes return timestamp/date/time schema labels with space-separated values; no T separator, no trailing Z, no string schema labels.

4. Local CI gates (run before each push)

• :api:test --tests DatetimeExtensionTest → 8/8 pass
• :api:test --tests DatetimeExtensionSqlTest → 3/3 pass
• :api:spotlessJavaCheck
• :integ-test:compileTestJava

Test plan

• UTs (DatetimeExtensionTest, DatetimeExtensionSqlTest)
• Wire-format curl sweep (18/18 PASS)
• CalciteAnalyticsDatetimeWireFormatIT shape
• CI gates
• :integ-test:integTestRemote against force-routed AE cluster

Fixes #5420

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit c5aa1d5)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ No major issues detected

[github-actions]

Persistent review updated to latest commit cf16199

[dai-chen]

Thanks for the fix! Please check the CI failure.

[github-actions]

Persistent review updated to latest commit b5e8976

[github-actions]

Persistent review updated to latest commit aea12b6

[github-actions]

Persistent review updated to latest commit c5aa1d5

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Make routing assertion more robust The assertion checks for a hardcoded lowercase string "opensearch" in the explain output. If the index name or table reference format changes, this check will fail. Consider making the assertion more robust by checking for the presence of LogicalTableScan separately from the table name pattern. integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteAnalyticsDatetimeWireFormatIT.java [101-105] String explained = explainQueryToString(query); Assert.assertTrue( - "Expected analytics-engine route (LogicalTableScan + lowercase 'opensearch'), got: " - + explained, - explained.contains("LogicalTableScan(table=[[opensearch,")); + "Expected analytics-engine route with LogicalTableScan, got: " + explained, + explained.contains("LogicalTableScan(table=[[")); +Assert.assertTrue( + "Expected lowercase 'opensearch' in table reference, got: " + explained, + explained.toLowerCase().contains("opensearch")); Suggestion importance[1-10]: 3 __ Why: The suggestion proposes splitting the assertion into two separate checks, which could provide slightly more granular error messages. However, the original assertion is already clear and the improvement is marginal. The use of toLowerCase() in the improved version could actually weaken the test by accepting uppercase variations.	Low

[dai-chen]

Add some minor comments. Thanks for the fix!

[dai-chen]

CI seems failing on spotless check