Skip to content

Commit 36bfcce

Browse files
ahkcsnssuresh2007
authored andcommitted
[analytics-engine] Coverage fixes for search command on the analytics-engine route (opensearch-project#21681)
* [analytics-backend-datafusion] Register TIMESTAMP in STANDARD_PROJECT_OPS PPL `timestamp(expr)` lowers to a `ScalarFunction.TIMESTAMP` call. The TimestampFunctionAdapter already wires the call into DataFusion's native `to_timestamp`, but `STANDARD_PROJECT_OPS` did not declare TIMESTAMP, so `OpenSearchProjectRule.annotateExpr` rejected every plan that contained the operator with "No backend supports scalar function [TIMESTAMP] among [datafusion]". Same call also shows up implicitly after the analyzer coerces a string literal to TIMESTAMP for column comparisons such as `@timestamp="2024-01-15T10:30:00Z"` once `@timestamp` is typed as TIMESTAMP (see the date_nanos schema fix in the previous commit). Update the inline comment block to match: the legacy-engine-only path the prior comment described is gone now that the capability is wired. Signed-off-by: Kai Huang <ahkcs@amazon.com> * [analytics-engine] Catch Calcite Litmus.THROW AssertionError in DefaultPlanExecutor RexUtil.isFlat / RelOptUtil.eq / Project.isValid / RexChecker call into Calcite's Litmus.THROW, which raises AssertionError from raw Java code rather than via the `assert` keyword. JVM `-da` doesn't gate that path, so an assertion firing inside a search thread escapes to OpenSearchUncaughtExceptionHandler and exits the cluster JVM. This bit hard once `CalciteRelNodeVisitor` started lowering structured PPL `search` predicates to native filter shape: queries like `severityNumber="not-a-number"` fold to `=(SAFE_CAST($X), null)` ahead of the marking phase, and the Litmus check fires before any plan-executor listener gets to translate the error. The cluster died mid-IT and 21 subsequent tests failed with `Connection refused`. Convert AssertionError caught at the executor entrypoint to an IllegalStateException so the query reports as HTTP 500 with a bucketable message and the cluster survives. The same pattern is already in place at `UnifiedQueryPlanner.plan` on the SQL plugin side; this is the analytics-engine-side mirror so neither layer can produce a cluster-fatal assertion. Signed-off-by: Kai Huang <ahkcs@amazon.com> * [analytics-backend-datafusion] Preserve fractional seconds in DatetimeOutputCastRewriter format Widen the format string passed to `to_char` from the seconds-only {@code "%Y-%m-%d %H:%M:%S"} to {@code "%Y-%m-%d %H:%M:%S%.f"}. The trailing {@code %.f} is chrono's variable-length fractional-second specifier — a leading dot followed by 0-9 digits, omitted when the value has no sub-second precision. This matches PPL's legacy formatting for {@code date} and {@code date_nanos} fields where the displayed precision tracks the source value: - {@code 2024-01-15T10:30:01.23456789Z} (date_nanos) → {@code "2024-01-15 10:30:01.23456789"} (legacy) / now {@code "2024-01-15 10:30:01.234567890"} (analytics route — internally 9-digit, leading 0 because Arrow Timestamp(ns) precision is fixed) - {@code 2025-08-01T03:47:41Z} (date) → {@code "2025-08-01 03:47:41"} (both paths) — no decimal because the source value has no fractional component Surfaced by `CalciteSearchCommandIT.testSearchWithDateRangeComparisons`. Follow-up to opensearch-project/sql#5420 which the original PR (opensearch-project#21650) closed with a seconds-only format that dropped the fractional digits the tests still expect. Signed-off-by: Kai Huang <ahkcs@amazon.com> * [analytics-backend-datafusion] Apply schema coercion on indexed-executor placeholder The indexed-execution path inferred the parquet schema via build_segments (which calls FileFormat::infer_schema) and registered it directly on the PlaceholderProvider that from_substrait_plan binds against. The non-indexed paths (session_context.rs, query_executor.rs, api.rs) all routed their inferred schemas through schema_coerce::coerce_inferred_schema before registering, but the indexed path skipped this step. Result: an OpenSearch `ip` column lands as parquet `BinaryView` on disk; isthmus on the Java side serializes the Substrait base schema as plain `Binary` (Substrait has no view types). The placeholder reports `BinaryView` while the plan declares `Binary` — DataFusion's substrait consumer rejects the bind with: Substrait error: Field '<x>' in Substrait schema has a different type (Binary) than the corresponding field in the table schema (BinaryView). Every analytics-engine query against an index that includes an `ip` column (every OTEL-logs query in CalciteSearchCommandIT, for example) fails at fragment start. Apply coerce_inferred_schema right after build_segments, before PlaceholderProvider construction. The placeholder, the expr_to_bool_tree analysis, and the downstream IndexedTableProvider all see the same Substrait-compatible (Binary / Int64 / Float32) schema, so the bind succeeds and the parquet reader's SchemaAdapter handles the per-batch BinaryView→Binary relabeling at scan time. This restores parity with the other infer_schema sites; no behavior change for non-IP columns. Signed-off-by: Kai Huang <ahkcs@amazon.com> --------- Signed-off-by: Kai Huang <ahkcs@amazon.com>
1 parent f669b8e commit 36bfcce

5 files changed

Lines changed: 30 additions & 14 deletions

File tree

sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_executor.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -450,6 +450,7 @@ pub async unsafe fn execute_indexed_with_context(
450450
let (segments, schema) = build_segments(&state, Arc::clone(&store), object_metas.as_ref(), writer_generations.as_ref())
451451
.await
452452
.map_err(DataFusionError::Execution)?;
453+
let schema = crate::schema_coerce::coerce_inferred_schema(schema);
453454
for (i, seg) in segments.iter().enumerate() {
454455
}
455456

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionAnalyticsBackendPlugin.java

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -232,11 +232,9 @@ public class DataFusionAnalyticsBackendPlugin implements AnalyticsSearchBackendP
232232
// see a real time/date type and Isthmus serializes accordingly.
233233
ScalarFunction.TIME,
234234
ScalarFunction.DATE,
235+
ScalarFunction.TIMESTAMP,
235236
// PPL `datetime(expr)` — parse/cast into a TIMESTAMP. Routes to DF's
236-
// builtin `to_timestamp` via DatetimeAdapter. The single-arg
237-
// `timestamp(expr)` form shares these semantics but its ScalarFunction
238-
// slot is already bound to TimestampFunctionAdapter for VARCHAR literal
239-
// folding, so it stays on the legacy engine.
237+
// builtin `to_timestamp` via DatetimeAdapter.
240238
ScalarFunction.DATETIME,
241239
// PPL extract / make* / format / from_unixtime are implemented as Rust UDFs
242240
// to preserve MySQL semantics that DataFusion builtins don't match: EXTRACT

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DatetimeOutputCastRewriter.java

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -82,9 +82,15 @@ final class DatetimeOutputCastRewriter {
8282
/**
8383
* PPL's documented timestamp output format (space separator). Mirrors the
8484
* format used by Calcite's reference planner so the analytics-engine path
85-
* matches per-row output exactly.
85+
* matches per-row output exactly. The trailing {@code %.f} is chrono's
86+
* variable-length fractional-second specifier — a leading dot followed by
87+
* 0-9 digits, omitted when the value has no sub-second precision. This
88+
* matches PPL's legacy formatting for {@code date} and {@code date_nanos}
89+
* fields where the displayed precision tracks the source value (e.g.
90+
* {@code "2024-01-15 10:30:01.23456789"} for a date_nanos with 8 fractional
91+
* digits, {@code "2025-08-01 03:47:41"} for a whole-second value).
8692
*/
87-
static final String PPL_TIMESTAMP_FORMAT = "%Y-%m-%d %H:%M:%S";
93+
static final String PPL_TIMESTAMP_FORMAT = "%Y-%m-%d %H:%M:%S%.f";
8894

8995
private DatetimeOutputCastRewriter() {}
9096

sandbox/plugins/analytics-backend-datafusion/src/test/java/org/opensearch/be/datafusion/DatetimeOutputCastRewriterTests.java

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -55,8 +55,11 @@ public void setUp() throws Exception {
5555

5656
/**
5757
* Motivating shape: outer Project slot is exactly {@code CAST(<TIMESTAMP> AS VARCHAR)}.
58-
* Rewriter must replace it with a {@code TO_CHAR(<TIMESTAMP>, '%Y-%m-%d %H:%M:%S')}
59-
* call whose result type matches the original cast's VARCHAR type.
58+
* Rewriter must replace it with a {@code TO_CHAR(<TIMESTAMP>, '%Y-%m-%d %H:%M:%S%.f')}
59+
* call whose result type matches the original cast's VARCHAR type. The {@code %.f}
60+
* tail is chrono's variable-length fractional-seconds specifier so source values
61+
* with sub-second precision (DATE_NANOS) keep their fractional digits while
62+
* whole-second values display cleanly without a trailing decimal.
6063
*/
6164
public void testDirectTimestampOutputCastIsRewrittenToToChar() {
6265
RelDataType timestampType = typeFactory.createTypeWithNullability(typeFactory.createSqlType(SqlTypeName.TIMESTAMP, 6), true);
@@ -236,11 +239,11 @@ public void testTimestampCastToCharIsUntouched() {
236239
}
237240

238241
/**
239-
* The format string is {@code "%Y-%m-%d %H:%M:%S"} — seconds-only — to match
240-
* Calcite's reference output for {@code CAST(TIMESTAMP AS VARCHAR)}, which
241-
* truncates fractional seconds. This pins that contract: a TIMESTAMP(9)
242-
* source still resolves to the seconds-only format literal in the
243-
* rewritten {@code TO_CHAR} call.
242+
* The format string is precision-agnostic: a TIMESTAMP(0) and a TIMESTAMP(9)
243+
* source both resolve to the same {@code "%Y-%m-%d %H:%M:%S%.f"} literal in the
244+
* rewritten {@code TO_CHAR} call. The {@code %.f} tail handles the runtime
245+
* variation — whole-second values render without a trailing decimal, sub-second
246+
* values render with as many fractional digits as the source carries.
244247
*/
245248
public void testTimestampPrecisionDoesNotChangeFormat() {
246249
RelDataType nanoTimestampType = typeFactory.createTypeWithNullability(typeFactory.createSqlType(SqlTypeName.TIMESTAMP, 9), true);
@@ -255,7 +258,7 @@ public void testTimestampPrecisionDoesNotChangeFormat() {
255258
RexCall call = (RexCall) ((LogicalProject) rewritten).getProjects().get(0);
256259
RexLiteral formatLit = (RexLiteral) call.getOperands().get(1);
257260
assertEquals(
258-
"TIMESTAMP(9) source must still resolve to the seconds-only format — fractional seconds are dropped",
261+
"Format literal is precision-agnostic — chrono's %.f handles the per-value fractional digits",
259262
DatetimeOutputCastRewriter.PPL_TIMESTAMP_FORMAT,
260263
formatLit.getValueAs(String.class)
261264
);

sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/DefaultPlanExecutor.java

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,14 @@ public void execute(RelNode logicalFragment, Object context, ActionListener<Iter
118118
executeInternal(logicalFragment, listener);
119119
} catch (Exception e) {
120120
listener.onFailure(e);
121+
} catch (AssertionError e) {
122+
// Calcite's Litmus.THROW (used by RelOptUtil.eq, RexUtil.isFlat, Project.isValid,
123+
// RexChecker) throws AssertionError directly via Java code rather than via the
124+
// `assert` keyword, so JVM -da doesn't gate them. If one fires inside this
125+
// executor, OpenSearchUncaughtExceptionHandler exits the cluster JVM. Convert to
126+
// an IllegalStateException so the query path treats it as a per-query failure
127+
// (HTTP 500 with a bucketable message) instead of cluster-fatal.
128+
listener.onFailure(new IllegalStateException("Analytics-engine executor rejected the plan: " + e.getMessage(), e));
121129
}
122130
});
123131
}

0 commit comments

Comments
 (0)