Fix singleton stack-corruption NPE in DatetimeUdtNormalizeRule

[ahkcs]

What this fixes

Some PPL queries to parquet-backed indices return HTTP 500 with
NoSuchElementException when several queries hit the cluster at the same time
— the failure mode reported on dashboard "field statistics" panels that probe
many fields in parallel. Affected query shape:

source = my_index | stats count() as field_count, distinct_count(<datetime_field>) as distinct_count

The failure is intermittent: same query passes or fails depending on what
other queries run at the same time.

Why it happens

The unified query API runs two Calcite rewrites on every plan — one normalizes
datetime UDT types (DatetimeUdtNormalizeRule), one casts datetime output to
varchar (DatetimeOutputCastRule). Both extend RelHomogeneousShuttle, which
in Calcite inherits a Deque<RelNode> stack field used internally for
push/pop during tree traversal. That stack is a plain non-thread-safe
ArrayDeque.

DatetimeExtension.postAnalysisRules() returned a static INSTANCE of each
rule, so every plan() call shared the same shuttle — and the same stack.
When dashboard issues N field-stats queries at once, N cluster threads call
plan() concurrently. Their push and pop operations on the shared
ArrayDeque interleave, leaving residual entries on the stack. A subsequent
traversal then tries to pop() an entry that isn't there, and the cluster
returns 500 with this stack trace:

java.util.NoSuchElementException
  at java.util.ArrayDeque.pop(ArrayDeque.java:591)
  at org.apache.calcite.rel.RelShuttleImpl.visitChild(RelShuttleImpl.java:67)  ← finally{ stack.pop() }
  at org.apache.calcite.rel.RelShuttleImpl.visit(RelShuttleImpl.java:151)
  at org.opensearch.sql.api.spec.datetime.DatetimeUdtNormalizeRule.visit(DatetimeUdtNormalizeRule.java:33)
  at org.apache.calcite.rel.RelHomogeneousShuttle.visit(RelHomogeneousShuttle.java:43)
  at org.apache.calcite.rel.logical.LogicalAggregate.accept(LogicalAggregate.java:159)

RelShuttleImpl was never designed to be a singleton: the stack is per-shuttle
mutable state. Sharing it across queries is unsafe regardless of timing — the
race just amplifies the chance of a visible failure.

What this PR changes

api/src/main/java/org/opensearch/sql/api/spec/datetime/DatetimeExtension.java
now returns a fresh instance per plan() call:

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

The INSTANCE constants and the Lombok @NoArgsConstructor(PRIVATE) are
removed from both rules. Class JavaDoc on each rule now explicitly warns
against making it a singleton again.

Test plan

Cluster state	testConcurrentStatsDistinctCountOverDatetime	testConcurrentMixedDatetimePlans
Without fix (singleton INSTANCE)	2/80 queries fail with NoSuchElementException (HTTP 500)	3/80 fail with same error
With fix (fresh instances per plan())	80/80 pass	80/80 pass

Reproduced and verified end-to-end via the analytics-engine path with
parquet-backed indices:

./gradlew :integ-test:integTestRemote \
  -Dtests.rest.cluster=localhost:9200 -Dtests.cluster=localhost:9300 \
  -Dtests.clustername=runTask \
  -Dtests.analytics.force_routing=true \
  -Dtests.analytics.parquet_indices=true \
  --tests "*CalciteDatetimeUdtNormalizeRegressionIT"

The IT (CalciteDatetimeUdtNormalizeRegressionIT) fires 80 queries across an
8-thread pool against a parquet-backed DATE_FORMATS index with multiple
datetime columns — mirroring the dashboard field-stats workload.

• :api:test and :core:test pass
• :api:spotlessCheck passes
• Concurrent IT reliably reproduces the failure against unfixed code
• Same IT passes 0/80 failures against fixed code
• Unit test testSequentialPlanCallsDoNotCorruptShuttleStack in
  DatetimeExtensionTest guards against future reintroduction of the
  singleton

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 0c1b701)

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]

PR Code Suggestions ✨

Latest suggestions up to 0c1b701

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Handle timeout exceptions properly The timeout exception is caught but not added to the errors list. Add TimeoutException handling to track queries that exceed the 60-second limit, ensuring all failure modes are properly reported in the assertion message. integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePlannerConcurrencyIT.java [132] -f.get(60, TimeUnit.SECONDS); +try { + f.get(60, TimeUnit.SECONDS); +} catch (java.util.concurrent.TimeoutException e) { + failures.incrementAndGet(); + synchronized (errors) { + errors.add(e); + } +} Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that TimeoutException is not being caught and tracked. This is a valid improvement for comprehensive error reporting in the test, though the test would still fail appropriately without it since the timeout would propagate.	Medium
	Force shutdown on termination timeout If awaitTermination times out, the executor may still have running tasks. Call shutdownNow() if termination fails to forcefully stop any lingering threads and prevent resource leaks. integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePlannerConcurrencyIT.java [151-152] executor.shutdown(); -executor.awaitTermination(30, TimeUnit.SECONDS); +if (!executor.awaitTermination(30, TimeUnit.SECONDS)) { + executor.shutdownNow(); +} Suggestion importance[1-10]: 6 __ Why: Valid suggestion to prevent potential resource leaks if awaitTermination times out. Using shutdownNow() is a best practice for executor cleanup, though in a test context the impact is less critical than in production code.	Low

---

Previous suggestions

Suggestions up to commit 885052e

Category	Suggestion	Impact
Possible issue	Handle TimeoutException in concurrent test The get() call can throw TimeoutException which is not caught, potentially causing the test to fail unexpectedly. Add a catch block for TimeoutException to handle timeout scenarios gracefully and include them in the failure count. integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteDatetimeUdtNormalizeRegressionIT.java [128-137] for (CompletableFuture<Void> f : futures) { try { f.get(60, TimeUnit.SECONDS); } catch (ExecutionException e) { failures.incrementAndGet(); synchronized (errors) { errors.add(e.getCause()); } + } catch (TimeoutException e) { + failures.incrementAndGet(); + synchronized (errors) { + errors.add(e); + } } } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that f.get(60, TimeUnit.SECONDS) can throw TimeoutException, which is currently not caught. This could cause the test to fail unexpectedly without proper error tracking. Adding the catch block improves error handling robustness in the concurrent test scenario.	Medium

Suggestions up to commit 35bf78f

Category	Suggestion	Impact
General	Add assertions for plan execution The test executes planner.plan() calls without capturing or asserting on their results. Add assertions to verify the plans execute successfully without throwing exceptions, ensuring the regression is properly validated. api/src/test/java/org/opensearch/sql/api/spec/datetime/DatetimeExtensionTest.java [183-193] for (int i = 0; i < 5; i++) { - planner.plan( + assertDoesNotThrow(() -> planner.plan( "source = catalog.events" - + " | stats count() as field_count, distinct_count(created_at) as distinct_count"); - planner.plan( + + " | stats count() as field_count, distinct_count(created_at) as distinct_count")); + assertDoesNotThrow(() -> planner.plan( "source = catalog.events" + " | eval ts = TIMESTAMP(name)" - + " | stats count() as field_count, distinct_count(ts) as distinct_count"); - planner.plan( - "source = catalog.events | where created_at > \"2024-01-01\" | fields hire_date"); + + " | stats count() as field_count, distinct_count(ts) as distinct_count")); + assertDoesNotThrow(() -> planner.plan( + "source = catalog.events | where created_at > \"2024-01-01\" | fields hire_date")); } Suggestion importance[1-10]: 5 __ Why: While adding assertDoesNotThrow() would make the test more explicit, the current implementation already validates that no exceptions are thrown - if any planner.plan() call threw an exception, the test would fail. The suggestion improves test clarity but doesn't fix a critical issue.	Low

[github-actions]

Persistent review updated to latest commit 6e7e1f0

[github-actions]

Persistent review updated to latest commit 885052e

[dai-chen]

Thanks for the fix!

[dai-chen]

np: we can rename this as general test for our planner.

[ahkcs]

Renamed to CalcitePlannerConcurrencyIT and reframed the JavaDoc as a general bucket for planner-level concurrency / state-isolation regressions. The current test methods are noted as the motivating regression — future planner-concurrency tests can land alongside. Pushed as 0c1b7017a.

[github-actions]

Persistent review updated to latest commit 0c1b701

[github-actions]

Persistent review updated to latest commit 0c1b701