Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ahkcs wants to merge 3 commits into
base: main
Choose a base branch
from
+370 −3
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
29fa3a0
multisearch fix
ahkcs Feb 17, 2026
81c0d3f
fix IT
ahkcs Feb 17, 2026
bb026a5
add UT
ahkcs Feb 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions core/src/main/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
import org.apache.calcite.sql.SqlCollation;
import org.apache.calcite.sql.type.SqlTypeName;
import org.apache.calcite.sql.type.SqlTypeUtil;
import org.checkerframework.checker.nullness.qual.Nullable;
import org.opensearch.sql.calcite.type.AbstractExprRelDataType;
import org.opensearch.sql.calcite.type.ExprBinaryType;
import org.opensearch.sql.calcite.type.ExprDateType;
Expand Down Expand Up @@ -377,6 +378,44 @@ public static boolean isNumericType(RelDataType fieldType) {
return false;
}

/**
* Preserves OpenSearch UDT types through set operations (UNION, INTERSECT, EXCEPT). When all
* input types share the same {@link AbstractExprRelDataType} with the same {@link
* AbstractExprRelDataType#getUdt() UDT}, 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. When inputs include non-UDT types or different UDTs, this
* method falls back to {@link super#leastRestrictive}.
*
* @param types the list of input {@link RelDataType} instances to find the least restrictive
* common type for
* @return the least restrictive {@link RelDataType} preserving the UDT wrapper when all inputs
* share the same UDT, or {@code null} if no common type exists (as determined by {@link
* super#leastRestrictive})
*/
@Override
public @Nullable RelDataType leastRestrictive(List<RelDataType> types) {
if (types.size() > 1) {
RelDataType first = types.get(0);
if (first instanceof AbstractExprRelDataType<?> firstUdt) {
boolean allSameUdt =
types.stream()
.allMatch(
t ->
t instanceof AbstractExprRelDataType<?> udt
&& udt.getUdt() == firstUdt.getUdt());
if (allSameUdt) {
boolean anyNullable = types.stream().anyMatch(RelDataType::isNullable);
if (anyNullable && !first.isNullable()) {
return firstUdt.createWithNullability(this, true);
Copy link
Collaborator

@Swiddis Swiddis Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Could do this in a single pass with a plain for loop instead of splitting the match checks and null checks.

@Override
public @Nullable RelDataType leastRestrictive(List<RelDataType> types) {
  if (types.size() > 1) {
    RelDataType first = types.get(0);
    if (first instanceof AbstractExprRelDataType<?> firstUdt) {
      boolean anyNullable = false;
      for (RelDataType t : types) {
        if (t instanceof AbstractExprRelDataType<?> udt
            && udt.getUdt() == firstUdt.getUdt()) {
          anyNullable |= t.isNullable();
        } else {
          return super.leastRestrictive(types); // If we hit a non-match, fallback
        }
      }
      if (anyNullable && !first.isNullable()) {
        return firstUdt.createWithNullability(this, true);
      }
      return first;
    }
  }
  return super.leastRestrictive(types);
}

}
return first;
}
}
}
return super.leastRestrictive(types);
}
Comment on lines +381 to +417
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ 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.


/**
* Checks if the RelDataType represents a time-based field (timestamp, date, or time). Supports
* both standard SQL time types (including TIMESTAMP, TIMESTAMP_WITH_LOCAL_TIME_ZONE, DATE, TIME,
Expand Down
131 changes: 131 additions & 0 deletions core/src/test/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactoryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

package org.opensearch.sql.calcite.utils;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertInstanceOf;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.opensearch.sql.calcite.utils.OpenSearchTypeFactory.TYPE_FACTORY;

import java.util.List;
import org.apache.calcite.rel.type.RelDataType;
import org.apache.calcite.sql.type.SqlTypeName;
import org.junit.jupiter.api.Test;
import org.opensearch.sql.calcite.type.AbstractExprRelDataType;
import org.opensearch.sql.calcite.utils.OpenSearchTypeFactory.ExprUDT;

public class OpenSearchTypeFactoryTest {

@Test
public void testLeastRestrictivePreservesUdtWhenAllInputsSameUdt() {
RelDataType ts1 = TYPE_FACTORY.createUDT(ExprUDT.EXPR_TIMESTAMP);
RelDataType ts2 = TYPE_FACTORY.createUDT(ExprUDT.EXPR_TIMESTAMP);

RelDataType result = TYPE_FACTORY.leastRestrictive(List.of(ts1, ts2));

assertNotNull(result);
assertInstanceOf(AbstractExprRelDataType.class, result);
assertEquals(ExprUDT.EXPR_TIMESTAMP, ((AbstractExprRelDataType<?>) result).getUdt());
}

@Test
public void testLeastRestrictivePreservesUdtForDateType() {
RelDataType d1 = TYPE_FACTORY.createUDT(ExprUDT.EXPR_DATE);
RelDataType d2 = TYPE_FACTORY.createUDT(ExprUDT.EXPR_DATE);

RelDataType result = TYPE_FACTORY.leastRestrictive(List.of(d1, d2));

assertNotNull(result);
assertInstanceOf(AbstractExprRelDataType.class, result);
assertEquals(ExprUDT.EXPR_DATE, ((AbstractExprRelDataType<?>) result).getUdt());
}

@Test
public void testLeastRestrictivePreservesUdtForThreeInputs() {
RelDataType ts1 = TYPE_FACTORY.createUDT(ExprUDT.EXPR_TIMESTAMP);
RelDataType ts2 = TYPE_FACTORY.createUDT(ExprUDT.EXPR_TIMESTAMP);
RelDataType ts3 = TYPE_FACTORY.createUDT(ExprUDT.EXPR_TIMESTAMP);

RelDataType result = TYPE_FACTORY.leastRestrictive(List.of(ts1, ts2, ts3));

assertNotNull(result);
assertInstanceOf(AbstractExprRelDataType.class, result);
assertEquals(ExprUDT.EXPR_TIMESTAMP, ((AbstractExprRelDataType<?>) result).getUdt());
}

@Test
public void testLeastRestrictiveReturnsNullableWhenAnyInputIsNullable() {
RelDataType nonNullable = TYPE_FACTORY.createUDT(ExprUDT.EXPR_TIMESTAMP, false);
RelDataType nullable = TYPE_FACTORY.createUDT(ExprUDT.EXPR_TIMESTAMP, true);

RelDataType result = TYPE_FACTORY.leastRestrictive(List.of(nonNullable, nullable));

assertNotNull(result);
assertInstanceOf(AbstractExprRelDataType.class, result);
assertEquals(ExprUDT.EXPR_TIMESTAMP, ((AbstractExprRelDataType<?>) result).getUdt());
assertTrue(result.isNullable());
}

@Test
public void testLeastRestrictiveReturnsNullableWhenFirstNullableSecondNot() {
RelDataType nullable = TYPE_FACTORY.createUDT(ExprUDT.EXPR_TIMESTAMP, true);
RelDataType nonNullable = TYPE_FACTORY.createUDT(ExprUDT.EXPR_TIMESTAMP, false);

RelDataType result = TYPE_FACTORY.leastRestrictive(List.of(nullable, nonNullable));

assertNotNull(result);
assertInstanceOf(AbstractExprRelDataType.class, result);
assertTrue(result.isNullable());
}

@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());
}
}

@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());
}
}

@Test
public void testLeastRestrictiveDelegatesToSuperForSingleType() {
RelDataType single = TYPE_FACTORY.createSqlType(SqlTypeName.INTEGER);

RelDataType result = TYPE_FACTORY.leastRestrictive(List.of(single));

assertNotNull(result);
assertEquals(SqlTypeName.INTEGER, result.getSqlTypeName());
}

@Test
public void testLeastRestrictiveDelegatesToSuperForPlainTypes() {
RelDataType int1 = TYPE_FACTORY.createSqlType(SqlTypeName.INTEGER);
RelDataType int2 = TYPE_FACTORY.createSqlType(SqlTypeName.INTEGER);

RelDataType result = TYPE_FACTORY.leastRestrictive(List.of(int1, int2));

assertNotNull(result);
assertEquals(SqlTypeName.INTEGER, result.getSqlTypeName());
}
}
Loading
Loading