Skip to content

Commit 35bf78f

Browse files
committed
fix: instantiate DatetimeUdt normalize/output-cast rules per plan() call
DatetimeUdtNormalizeRule and DatetimeOutputCastRule extend RelHomogeneousShuttle, which inherits a stateful Deque<RelNode> stack from RelShuttleImpl. DatetimeExtension.postAnalysisRules() returned the static INSTANCE of each rule, sharing the same shuttle (and the same stack) across every UnifiedQueryPlanner.plan() invocation. If any traversal ever ends with an unbalanced stack, residual entries persist to the next query. The next query's visitChild() then pops a stale or empty stack and throws NoSuchElementException at RelShuttleImpl.visitChild line 67 (the stack.pop() in the finally block) — surfacing as the cluster-side stack trace reported on analytics-engine-routed parquet indices for queries that combine aggregations over datetime UDT columns (e.g. "stats count() as field_count, distinct_count(field)"). Return fresh instances per plan() instead. Drop the INSTANCE constants and the Lombok @NoArgsConstructor on both rules; document the singleton-unsafety on each class JavaDoc. Add a regression test that runs several plan() calls in sequence against the same context, covering stats+distinct_count over both schema-declared and eval-derived datetime columns. Signed-off-by: Kai Huang <ahkcs@amazon.com>
1 parent f1f39ef commit 35bf78f

4 files changed

Lines changed: 32 additions & 12 deletions

File tree

api/src/main/java/org/opensearch/sql/api/spec/datetime/DatetimeExtension.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,8 @@ public class DatetimeExtension implements LanguageExtension {
2222

2323
@Override
2424
public List<RelShuttle> postAnalysisRules() {
25-
return List.of(DatetimeUdtNormalizeRule.INSTANCE, DatetimeOutputCastRule.INSTANCE);
25+
// Fresh instances per plan() because RelHomogeneousShuttle inherits a stateful stack.
26+
return List.of(new DatetimeUdtNormalizeRule(), new DatetimeOutputCastRule());
2627
}
2728

2829
/** Maps datetime UDT types to their standard Calcite equivalents. */

api/src/main/java/org/opensearch/sql/api/spec/datetime/DatetimeOutputCastRule.java

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,6 @@
99

1010
import java.util.ArrayList;
1111
import java.util.List;
12-
import lombok.AccessLevel;
13-
import lombok.NoArgsConstructor;
1412
import org.apache.calcite.rel.RelHomogeneousShuttle;
1513
import org.apache.calcite.rel.RelNode;
1614
import org.apache.calcite.rel.logical.LogicalProject;
@@ -21,12 +19,14 @@
2119
import org.apache.calcite.rex.RexNode;
2220
import org.apache.calcite.sql.type.SqlTypeName;
2321

24-
/** Wraps the root output with CAST(datetime → VARCHAR) for PPL wire-format compatibility. */
25-
@NoArgsConstructor(access = AccessLevel.PRIVATE)
22+
/**
23+
* Wraps the root output with CAST(datetime → VARCHAR) for PPL wire-format compatibility.
24+
*
25+
* <p>Not a singleton: {@link RelHomogeneousShuttle} inherits a stateful {@code stack} field from
26+
* {@link org.apache.calcite.rel.RelShuttleImpl}, so a fresh instance must be used per plan().
27+
*/
2628
class DatetimeOutputCastRule extends RelHomogeneousShuttle {
2729

28-
static final DatetimeOutputCastRule INSTANCE = new DatetimeOutputCastRule();
29-
3030
@Override
3131
public RelNode visit(RelNode other) {
3232
List<RelDataTypeField> fields = other.getRowType().getFieldList();

api/src/main/java/org/opensearch/sql/api/spec/datetime/DatetimeUdtNormalizeRule.java

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,6 @@
66
package org.opensearch.sql.api.spec.datetime;
77

88
import java.util.Optional;
9-
import lombok.AccessLevel;
10-
import lombok.NoArgsConstructor;
119
import org.apache.calcite.rel.RelHomogeneousShuttle;
1210
import org.apache.calcite.rel.RelNode;
1311
import org.apache.calcite.rel.type.RelDataType;
@@ -22,12 +20,12 @@
2220
/**
2321
* Temporary patch that rewrites datetime UDT return types on RexCall nodes to standard Calcite
2422
* types.
23+
*
24+
* <p>Not a singleton: {@link RelHomogeneousShuttle} inherits a stateful {@code stack} field from
25+
* {@link org.apache.calcite.rel.RelShuttleImpl}, so a fresh instance must be used per plan().
2526
*/
26-
@NoArgsConstructor(access = AccessLevel.PRIVATE)
2727
class DatetimeUdtNormalizeRule extends RelHomogeneousShuttle {
2828

29-
static final DatetimeUdtNormalizeRule INSTANCE = new DatetimeUdtNormalizeRule();
30-
3129
@Override
3230
public RelNode visit(RelNode other) {
3331
RelNode visited = super.visit(other);

api/src/test/java/org/opensearch/sql/api/spec/datetime/DatetimeExtensionTest.java

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -172,6 +172,27 @@ public void testNonDatetimeFieldsNotWrapped() {
172172
""");
173173
}
174174

175+
@Test
176+
public void testSequentialPlanCallsDoNotCorruptShuttleStack() {
177+
// Regression test: DatetimeUdtNormalizeRule extends RelHomogeneousShuttle which inherits a
178+
// stateful Deque<RelNode> stack field. Earlier implementations used a static INSTANCE shared
179+
// across all plan() calls; under workloads with aggregations (especially count + distinct_count
180+
// over datetime columns), the shared stack would desynchronize and visitChild's pop would throw
181+
// NoSuchElementException on subsequent plan calls. Running several distinct plans through the
182+
// same context confirms each invocation gets a fresh shuttle.
183+
for (int i = 0; i < 5; i++) {
184+
planner.plan(
185+
"source = catalog.events"
186+
+ " | stats count() as field_count, distinct_count(created_at) as distinct_count");
187+
planner.plan(
188+
"source = catalog.events"
189+
+ " | eval ts = TIMESTAMP(name)"
190+
+ " | stats count() as field_count, distinct_count(ts) as distinct_count");
191+
planner.plan(
192+
"source = catalog.events | where created_at > \"2024-01-01\" | fields hire_date");
193+
}
194+
}
195+
175196
@Test
176197
public void testOutputCastCanCompileAndExecute() throws Exception {
177198
RelNode plan =

0 commit comments

Comments
 (0)