Fix multisearch UDT type loss through UNION (#5145, #5146, #5147)

[ahkcs]

Summary

• Override leastRestrictive() in OpenSearchTypeFactory to preserve UDT types (e.g., ExprTimestampType) through set operations (UNION ALL) used by multisearch
• Without this fix, Calcite's default leastRestrictive() downgrades UDT-wrapped types to their underlying SQL type (e.g., VARCHAR), breaking downstream operators that rely on type checks
• Add integration tests verifying span, bin, and bin+stats work correctly after multisearch

Issues Resolved

Resolves #5145, resolves #5146, resolves #5147

See investigation details: #5153

Root Cause

OpenSearchTypeFactory (extends JavaTypeFactoryImpl) did not override leastRestrictive(). When Calcite builds UNION ALL for multisearch, it calls leastRestrictive() on column types from both branches. Both branches have ExprTimestampType (a UDT backed by SqlTypeName.VARCHAR), but the default implementation creates a plain BasicSqlType(VARCHAR), losing the UDT wrapper.

This caused:

• [BUG] PPL span-expression used after multisearch fails #5146: SpanFunction.SpanImplementor checks fieldType instanceof ExprSqlType → fails for plain VARCHAR → "Unsupported expr type: STRING"
• [BUG] PPL bin used after multisearch has no effect #5147: BinnableField.isTimeBasedType() can't recognize plain VARCHAR as time-based → bin produces null timestamps
• [BUG] PPL multisearch without further processing fails #5145: Same type loss causes plan preparation failures depending on downstream operators

Fix

When all input types share the same UDT (e.g., all are EXPR_TIMESTAMP), the result retains the UDT wrapper. Nullability is handled correctly — if any input is nullable, the result is nullable. Otherwise, delegates to the default Calcite implementation.

Test Plan

• testMultisearchWithoutFurtherProcessing — basic multisearch returns all 51 rows
• testMultisearchWithSpanExpression — stats avg(value) by span(@timestamp, 5m) after multisearch (36 rows verified)
• testMultisearchBinTimestamp — bin @timestamp span=5m produces non-null timestamps (36 rows verified)
• testMultisearchBinAndStats — bin + stats produces correct time buckets (36 rows verified)
• Existing testMultisearchWithTimestampInterleaving schema updated from "string" to "timestamp" (correct behavior)
• All existing multisearch integration tests pass

[coderabbitai]

📝 Walkthrough

Summary by CodeRabbit

• Bug Fixes

  • Preserve custom/user-defined data types across set operations (UNION/INTERSECT/EXCEPT).
  • Corrected handling of timestamp-typed fields in multisearch and append scenarios.

• Tests

  • Added integration tests for multisearch: timestamp binning, span expressions, projection/aggregation ordering, and stats validations.
  • Added unit tests validating least-restrictive behavior and nullability for user-defined types.

Walkthrough

Adds a leastRestrictive override to preserve OpenSearch UDT wrappers with proper nullability handling, updates PPL/multisearch integration tests (adds reproduction tests for issues #5145/#5146/#5147, with a duplicated test block), and adjusts expected timestamp schema in a PPL append test. Also adds unit tests for the new type behavior.

Changes

Cohort / File(s)	Summary
OpenSearch TypeFactory core/src/main/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactory.java	Added public @nullable RelDataType leastRestrictive(List<RelDataType> types) and import org.checkerframework.checker.nullness.qual.Nullable; to preserve OpenSearch UDT wrappers across set operations with nullability handling.
TypeFactory Unit Tests core/src/test/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactoryTest.java	New test class with multiple tests validating leastRestrictive behavior for UDT preservation, nullability propagation, mixed-type fallbacks, and delegation to superclass for plain types.
Multisearch Integration Tests (added/duplicated) integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteMultisearchCommandIT.java	Changed expected @timestamp type from string to timestamp and added four reproduction tests for issues #5145, #5146, #5147: testMultisearchWithoutFurtherProcessing, testMultisearchWithSpanExpression, testMultisearchBinTimestamp, testMultisearchBinAndStats. The block of four tests is duplicated elsewhere in the same file (duplicate test declarations present).
PPL Append Test (schema expectation update) integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLAppendCommandIT.java	Updated expected schema type for birthdate from string to timestamp in testAppendSchemaMergeWithTimestampUDT. No control-flow changes.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Suggested labels

bug, bugFix, calcite, PPL

Suggested reviewers

• penghuo
• anirudha
• ps48
• LantaoJin
• kavithacm
• derek-ho
• joshuali925
• mengweieric
• Yury-Fridlyand
• MaxKsyunz
• GumpacG

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title directly addresses the main change: fixing UDT type loss through UNION in multisearch, with specific issue references (#5145, #5146, #5147).
Description check	✅ Passed	The description comprehensively covers the changeset: the override of leastRestrictive(), root cause analysis, fix behavior, and test plan with specific test cases.
Linked Issues check	✅ Passed	The PR addresses all three linked issues: #5145 (multisearch without further processing), #5146 (span expressions after multisearch), and #5147 (bin command after multisearch) by preserving UDT types through UNION operations.
Out of Scope Changes check	✅ Passed	All changes are directly related to fixing the UDT type loss issue: leastRestrictive() override in OpenSearchTypeFactory, comprehensive unit tests for the new method, integration tests for multisearch scenarios, and schema type corrections in existing tests.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@core/src/main/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactory.java`:
- Around line 381-409: Update the JavaDoc for the public override
leastRestrictive in OpenSearchTypeFactory to include `@param` types (describe that
it is the list of input RelDataType), `@return` (the least restrictive
RelDataType, preserving UDT wrapper when applicable) and `@throws` (document any
runtime exceptions or pass-through from super.leastRestrictive if relevant);
then add unit tests covering: 1) UDT preservation—when all inputs are
AbstractExprRelDataType with the same getUdt() the result must keep that UDT
wrapper (use AbstractExprRelDataType test doubles), 2) mixed nullability—when
any input is nullable and the first is non-nullable the returned type becomes
nullable via createWithNullability(this, true), and 3) mixed UDT/non-UDT
inputs—when inputs include non-UDT types the method must fall back to
super.leastRestrictive; reference the method name leastRestrictive, the class
OpenSearchTypeFactory, and AbstractExprRelDataType in tests and assertions.

In
`@integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteMultisearchCommandIT.java`:
- Around line 371-542: The three tests testMultisearchWithSpanExpression,
testMultisearchBinTimestamp, and testMultisearchBinAndStats rely on implicit
result ordering and must be made deterministic: modify the query passed to
executeQuery to include an explicit sort (e.g., add "| sort `@timestamp` asc" or
"| sort <field> asc/desc" after the final pipeline stage) so results are
consistently ordered, then update the expected verifyDataRows rows(...)
sequences to match the new sorted order (or alternatively change verifyDataRows
calls to use an order-agnostic assertion helper); update only those tests and
keep the rest of the assertions intact.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Add required JavaDoc tags and unit tests for the new leastRestrictive override.
This new public method is missing @param/@return/@throws tags, and the commit doesn’t add unit tests for the new behavior (UDT preservation, mixed nullability, mixed UDT/non‑UDT inputs). Please add unit tests and update the JavaDoc accordingly.

✍️ Add the required JavaDoc tags

-  /**
-   * Preserves OpenSearch UDT types through set operations (UNION, INTERSECT, EXCEPT). When all
-   * input types share the same UDT (e.g., all are EXPR_TIMESTAMP), the result retains the UDT
-   * wrapper instead of being downgraded to the underlying SQL type (e.g., VARCHAR). This is
-   * critical for operations like multisearch that use UNION ALL, where downstream operators (bin,
-   * span) rely on the UDT type to determine how to process the field.
-   */
+  /**
+   * Preserves OpenSearch UDT types through set operations (UNION, INTERSECT, EXCEPT). When all
+   * input types share the same UDT (e.g., all are EXPR_TIMESTAMP), the result retains the UDT
+   * wrapper instead of being downgraded to the underlying SQL type (e.g., VARCHAR). This is
+   * critical for operations like multisearch that use UNION ALL, where downstream operators (bin,
+   * span) rely on the UDT type to determine how to process the field.
+   *
+   * `@param` types candidate types to reconcile
+   * `@return` least restrictive type that preserves UDT when possible
+   * `@throws` NullPointerException if {`@code` types} is null
+   */

As per coding guidelines: "New functions MUST have unit tests in the same commit" and "Public methods MUST have JavaDoc with `@param`, `@return`, and `@throws`".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@core/src/main/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactory.java`
around lines 381 - 409, Update the JavaDoc for the public override
leastRestrictive in OpenSearchTypeFactory to include `@param` types (describe that
it is the list of input RelDataType), `@return` (the least restrictive
RelDataType, preserving UDT wrapper when applicable) and `@throws` (document any
runtime exceptions or pass-through from super.leastRestrictive if relevant);
then add unit tests covering: 1) UDT preservation—when all inputs are
AbstractExprRelDataType with the same getUdt() the result must keep that UDT
wrapper (use AbstractExprRelDataType test doubles), 2) mixed nullability—when
any input is nullable and the first is non-nullable the returned type becomes
nullable via createWithNullability(this, true), and 3) mixed UDT/non-UDT
inputs—when inputs include non-UDT types the method must fall back to
super.leastRestrictive; reference the method name leastRestrictive, the class
OpenSearchTypeFactory, and AbstractExprRelDataType in tests and assertions.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Add deterministic ordering for the new multisearch reproduction tests.
These tests assert long ordered row lists, but the queries don’t include sort. Result order can vary, which risks flaky tests and breaks test‑independence requirements. Add explicit sorting (and update expected rows accordingly) or use order‑agnostic assertions.

🔧 Example (apply similarly to the other new tests)

-                + " = \"E\"] | stats avg(value) by span(`@timestamp`, 5m)");
+                + " = \"E\"] | stats avg(value) by span(`@timestamp`, 5m) | sort span(`@timestamp`, 5m)");

As per coding guidelines: "Tests must not rely on execution order; ensure test independence".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteMultisearchCommandIT.java`
around lines 371 - 542, The three tests testMultisearchWithSpanExpression,
testMultisearchBinTimestamp, and testMultisearchBinAndStats rely on implicit
result ordering and must be made deterministic: modify the query passed to
executeQuery to include an explicit sort (e.g., add "| sort `@timestamp` asc" or
"| sort <field> asc/desc" after the final pipeline stage) so results are
consistently ordered, then update the expected verifyDataRows rows(...)
sequences to match the new sorted order (or alternatively change verifyDataRows
calls to use an order-agnostic assertion helper); update only those tests and
keep the rest of the assertions intact.

[coderabbitai]

🧹 Nitpick comments (2)

core/src/test/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactoryTest.java (2)

111-131: Consider adding boundary condition tests for empty list and single UDT.

The delegation tests cover single plain type and two plain types, but the boundary cases for empty input and single UDT are not explicitly tested.

♻️ Suggested additional boundary tests

`@Test`
public void testLeastRestrictiveDelegatesToSuperForEmptyList() {
  RelDataType result = TYPE_FACTORY.leastRestrictive(List.of());
  
  // Empty list delegates to super.leastRestrictive which returns null
  assertNull(result);
}

`@Test`
public void testLeastRestrictiveDelegatesToSuperForSingleUdt() {
  RelDataType single = TYPE_FACTORY.createUDT(ExprUDT.EXPR_TIMESTAMP);
  
  RelDataType result = TYPE_FACTORY.leastRestrictive(List.of(single));
  
  // Single type (even UDT) delegates to super due to size check
  assertNotNull(result);
  assertEquals(SqlTypeName.VARCHAR, result.getSqlTypeName());
}

As per coding guidelines: "Include boundary condition tests (min/max values, empty inputs) for all new functions".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@core/src/test/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactoryTest.java`
around lines 111 - 131, Add two boundary unit tests in OpenSearchTypeFactoryTest
to cover empty input and single UDT cases for TYPE_FACTORY.leastRestrictive: add
a test that calls TYPE_FACTORY.leastRestrictive(List.of()) and asserts the
result is null (delegates to super for empty list), and add a test that creates
a single UDT (e.g., TYPE_FACTORY.createUDT(ExprUDT.EXPR_TIMESTAMP)), calls
leastRestrictive with that single UDT and asserts the returned type is not null
and has the expected SqlTypeName (e.g., VARCHAR) to ensure single-UDT delegation
behavior is validated.

---

85-109: Strengthen assertions in fallback tests to avoid silent passes.

The conditional assertions on lines 93-95 and 106-108 will silently pass if result is null, potentially masking regressions. Consider using explicit assertions for the expected behavior.

♻️ Proposed fix to strengthen assertions

  `@Test`
  public void testLeastRestrictiveFallsBackForMixedUdtAndNonUdt() {
    RelDataType udt = TYPE_FACTORY.createUDT(ExprUDT.EXPR_TIMESTAMP);
    RelDataType plain = TYPE_FACTORY.createSqlType(SqlTypeName.VARCHAR);

    RelDataType result = TYPE_FACTORY.leastRestrictive(List.of(udt, plain));

-   // Falls back to super.leastRestrictive which may return a plain type or null
-   if (result != null) {
-     assertEquals(SqlTypeName.VARCHAR, result.getSqlTypeName());
-   }
+   // Falls back to super.leastRestrictive — EXPR_TIMESTAMP has VARCHAR underlying type
+   assertNotNull(result, "Expected non-null result for compatible types");
+   assertEquals(SqlTypeName.VARCHAR, result.getSqlTypeName());
  }

  `@Test`
  public void testLeastRestrictiveFallsBackForDifferentUdts() {
    RelDataType timestamp = TYPE_FACTORY.createUDT(ExprUDT.EXPR_TIMESTAMP);
    RelDataType date = TYPE_FACTORY.createUDT(ExprUDT.EXPR_DATE);

    RelDataType result = TYPE_FACTORY.leastRestrictive(List.of(timestamp, date));

-   // Different UDTs — falls back to super.leastRestrictive
-   if (result != null) {
-     assertEquals(SqlTypeName.VARCHAR, result.getSqlTypeName());
-   }
+   // Different UDTs — falls back to super.leastRestrictive (both have VARCHAR underlying)
+   assertNotNull(result, "Expected non-null result for VARCHAR-based UDTs");
+   assertEquals(SqlTypeName.VARCHAR, result.getSqlTypeName());
  }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@core/src/test/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactoryTest.java`
around lines 85 - 109, Replace the conditional "if (result != null) { ... }"
checks in testLeastRestrictiveFallsBackForMixedUdtAndNonUdt and
testLeastRestrictiveFallsBackForDifferentUdts with explicit assertions:
assertNotNull(result) immediately after calling
TYPE_FACTORY.leastRestrictive(...) and then assertEquals(SqlTypeName.VARCHAR,
result.getSqlTypeName()); this ensures the RelDataType result is non-null and
its SqlTypeName is verified rather than silently passing when result is null.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In
`@core/src/test/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactoryTest.java`:
- Around line 111-131: Add two boundary unit tests in OpenSearchTypeFactoryTest
to cover empty input and single UDT cases for TYPE_FACTORY.leastRestrictive: add
a test that calls TYPE_FACTORY.leastRestrictive(List.of()) and asserts the
result is null (delegates to super for empty list), and add a test that creates
a single UDT (e.g., TYPE_FACTORY.createUDT(ExprUDT.EXPR_TIMESTAMP)), calls
leastRestrictive with that single UDT and asserts the returned type is not null
and has the expected SqlTypeName (e.g., VARCHAR) to ensure single-UDT delegation
behavior is validated.
- Around line 85-109: Replace the conditional "if (result != null) { ... }"
checks in testLeastRestrictiveFallsBackForMixedUdtAndNonUdt and
testLeastRestrictiveFallsBackForDifferentUdts with explicit assertions:
assertNotNull(result) immediately after calling
TYPE_FACTORY.leastRestrictive(...) and then assertEquals(SqlTypeName.VARCHAR,
result.getSqlTypeName()); this ensures the RelDataType result is non-null and
its SqlTypeName is verified rather than silently passing when result is null.