Decouple Calcite PPL planning from ExprType

[penghuo]

Description

Rewrite the Calcite-side PPL planning surface so RelNode/RexNode code, coercion, type checking, and UDF implementations operate on RelDataType directly instead of round-tripping through the v2 ExprType system. UDTs (ExprDateType, ExprTimeType, ExprTimeStampType, ExprIPType, ExprBinaryType) remain in the logical plan unchanged — only the planning-time abstraction is decoupled.

Scope: PPL planning surface only. No changes to runtime UDF execution or the storage engine. IP/BINARY UDTs and temporal UDTs remain in logical plans; this PR only stops passing them through the ExprType conversion layer at planning time.

Key rewrites:

• CoercionUtils — new RelDataType-typed common-type resolver with an internal CoercionTag widening DAG that mirrors v2 semantics (STRING → TIMESTAMP direct edge, UNDEFINED → any-concrete at unit cost). Widening normalizes temporal types to their UDT equivalents; cast targets are always made nullable so safe cast + primitive aggregators do not NPE on parse failure.
• PPLTypeChecker — signatures expressed as List<List<RelDataType>>; adds renderTypeName() for error messages; folds DECIMAL → DOUBLE. isComparable() accepts UDT temporal vs standard temporal of the same kind.
• PPLOperandTypes — exposes RelDataType signature constants (DATE_UDT, INTEGER_T, …).
• PPLFuncImpTable.requiresNumericArgument — uses SqlTypeUtil.isNumeric and SqlTypeName.ANY for the "unknown-type" check.
• ExtendedRexBuilder, AddSubDate/Extract/Format/LastDay/PeriodName/TimestampAdd/TimestampDiff/Weekday/Span/WidthBucket and the IP UDFs — branch on UDT classes and pass RelDataType through their implementors.
• visitCast in CalciteRexNodeVisitor — maps AST DataType directly to RelDataType, removing the DataType.getCoreType() round-trip.
• CurrentFunction / FormatFunction — take an internal Kind / boolean discriminator rather than an ExprCoreType.
• DatetimeExtension — switches from ExprUDT enum to type-class checks.

UDT identity at planning time is now determined via instanceof. Subclasses are preserved through createTypeWithNullability / createTypeWithCharsetAndCollation via a cloneWith hook on ExprSqlType / ExprJavaType so the instanceof checks survive type-factory operations.

Related Issues

N/A

Check List

• New functionality includes testing.
• New functionality has been documented.
• New functionality has javadoc added.
• New functionality has a user manual doc added.
• New PPL command checklist all confirmed.
• API changes companion pull request created.
• Commits are signed per the DCO using --signoff or -s.
• Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 689405c)

Here are some key observations to aid the review process:

🧪 No relevant tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Possible Issue The normalizeTemporalToUdt method returns a nullable UDT when the input type is nullable, but the switch statement does not preserve nullability for the default case. If a nullable non-temporal type is passed, the method returns the original type unchanged, which is correct. However, if a nullable temporal type (TIMESTAMP, DATE, TIME) is passed, the method calls createUDT(udt, type.isNullable()) which correctly preserves nullability. The logic appears sound, but the default case could be more explicit about nullability preservation to avoid future confusion. private static RelDataType normalizeTemporalToUdt(RelDataType type) { if (type instanceof AbstractExprRelDataType<?>) { return type; } SqlTypeName name = type.getSqlTypeName(); ExprUDT udt = switch (name) { case TIMESTAMP -> ExprUDT.EXPR_TIMESTAMP; case DATE -> ExprUDT.EXPR_DATE; case TIME -> ExprUDT.EXPR_TIME; default -> null; }; return udt == null ? type : OpenSearchTypeFactory.TYPE_FACTORY.createUDT(udt, type.isNullable()); } Possible Issue In the distance method with three parameters (line 307), when from == CoercionTag.UNDEFINED and to != CoercionTag.UNKNOWN, the method returns acc + 1. However, if to == CoercionTag.UNKNOWN, the method does not explicitly handle this case before checking parents. Since UNKNOWN has no parents (not in the PARENTS map), the method will return IMPOSSIBLE_WIDENING. This means UNDEFINED cannot widen to UNKNOWN, which may be intentional but differs from the comment stating "UNDEFINED (NULL literal) widens to any concrete type". If UNKNOWN is considered non-concrete, this is correct; otherwise, this is a bug. private static int distance(CoercionTag from, CoercionTag to, int acc) { if (from == to) return acc; if (from == CoercionTag.UNKNOWN) return IMPOSSIBLE_WIDENING; // UNDEFINED (NULL literal) widens to any concrete type at unit cost — mirrors v2 where every // concrete ExprCoreType has UNDEFINED as a parent. if (from == CoercionTag.UNDEFINED && to != CoercionTag.UNKNOWN) return acc + 1; // STRING widens directly to DOUBLE (custom rule mirroring v2 behavior). if (from == CoercionTag.STRING && to == CoercionTag.DOUBLE) return acc + 1; List<CoercionTag> parents = parentsOf(from); if (parents.isEmpty()) return IMPOSSIBLE_WIDENING; int best = IMPOSSIBLE_WIDENING; for (CoercionTag parent : parents) { int d = distance(parent, to, acc + 1); if (d < best) best = d; } return best; } Possible Issue The isComparable method at line 300 checks if both types are numeric and returns true. Then at line 308, it checks if both types are UDTs with the same tag. However, between these checks, there is no handling for the case where one type is a UDT and the other is a plain numeric type. If a UDT wraps a numeric type (though current UDTs are DATE/TIME/TIMESTAMP/IP/BINARY), this could cause incorrect comparability results. The current UDT set does not include numeric UDTs, so this is not a bug now, but the logic is fragile if numeric UDTs are added later. // Numeric types are comparable without the need to cast. if (SqlTypeUtil.isNumeric(type1) && SqlTypeUtil.isNumeric(type2)) { return true; } // Same UDT kind is comparable. Compare via the ExprUDT tag (not the concrete Java class), // because createTypeWithNullability on an AbstractExprRelDataType returns a plain ExprSqlType // that loses the concrete subclass identity. if (type1 instanceof AbstractExprRelDataType<?> udt1 && type2 instanceof AbstractExprRelDataType<?> udt2 && udt1.getUdt() == udt2.getUdt()) { return true; } // Temporal UDT vs standard temporal of the same kind is comparable (widening produced by // findWidestType may leave a standard TIMESTAMP field alongside a UDT temporal literal). if (temporalKind(type1) != null && temporalKind(type1) == temporalKind(type2)) { return true; } // Plain types are comparable when their SqlTypeFamily matches (CHAR vs VARCHAR, INTEGER vs // BIGINT, BOOLEAN vs BOOLEAN, etc.). UDTs are excluded since they share the underlying // VARCHAR family but should not be transparently comparable to plain strings here. if (!(type1 instanceof AbstractExprRelDataType<?>) && !(type2 instanceof AbstractExprRelDataType<?>)) { SqlTypeFamily family1 = type1.getSqlTypeName().getFamily(); SqlTypeFamily family2 = type2.getSqlTypeName().getFamily(); if (family1 != null && family1 == family2) { return true; } } // If one of the arguments is of type 'ANY', return true. return type1.getFamily() == SqlTypeFamily.ANY || type2.getFamily() == SqlTypeFamily.ANY; }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 689405c

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Add recursion depth limit The recursive distance method lacks a depth limit, which could cause stack overflow for malformed or circular parent relationships in the widening DAG. Although the current PARENTS map is acyclic, adding a safeguard prevents potential issues if the DAG is modified incorrectly in the future. core/src/main/java/org/opensearch/sql/expression/function/CoercionUtils.java [307-323] +private static final int MAX_WIDENING_DEPTH = 20; + private static int distance(CoercionTag from, CoercionTag to, int acc) { + if (acc > MAX_WIDENING_DEPTH) return IMPOSSIBLE_WIDENING; if (from == to) return acc; if (from == CoercionTag.UNKNOWN) return IMPOSSIBLE_WIDENING; - // UNDEFINED (NULL literal) widens to any concrete type at unit cost — mirrors v2 where every - // concrete ExprCoreType has UNDEFINED as a parent. if (from == CoercionTag.UNDEFINED && to != CoercionTag.UNKNOWN) return acc + 1; - // STRING widens directly to DOUBLE (custom rule mirroring v2 behavior). if (from == CoercionTag.STRING && to == CoercionTag.DOUBLE) return acc + 1; List<CoercionTag> parents = parentsOf(from); if (parents.isEmpty()) return IMPOSSIBLE_WIDENING; int best = IMPOSSIBLE_WIDENING; for (CoercionTag parent : parents) { int d = distance(parent, to, acc + 1); if (d < best) best = d; } return best; } Suggestion importance[1-10]: 4 __ Why: Adding a recursion depth limit is a defensive programming practice that could prevent stack overflow in case of future DAG modifications. However, the current PARENTS map is acyclic by design, and the suggestion addresses a hypothetical future issue rather than an existing bug. The impact is low since the code is correct as-is.	Low
	Ensure consistent nullability handling The method normalizeTemporalToUdt does not preserve the nullability of the input type when returning the original type. When udt == null, the method returns type directly, but when a UDT is created, it explicitly passes type.isNullable(). This inconsistency could lead to subtle bugs if the caller expects consistent nullability handling. core/src/main/java/org/opensearch/sql/expression/function/CoercionUtils.java [169-184] private static RelDataType normalizeTemporalToUdt(RelDataType type) { if (type instanceof AbstractExprRelDataType<?>) { return type; } SqlTypeName name = type.getSqlTypeName(); ExprUDT udt = switch (name) { case TIMESTAMP -> ExprUDT.EXPR_TIMESTAMP; case DATE -> ExprUDT.EXPR_DATE; case TIME -> ExprUDT.EXPR_TIME; default -> null; }; - return udt == null - ? type - : OpenSearchTypeFactory.TYPE_FACTORY.createUDT(udt, type.isNullable()); + if (udt == null) { + return type; + } + return OpenSearchTypeFactory.TYPE_FACTORY.createUDT(udt, type.isNullable()); } Suggestion importance[1-10]: 3 __ Why: The suggestion misidentifies an inconsistency. The method already handles nullability consistently: when udt == null, it returns the original type unchanged (preserving its nullability), and when a UDT is created, it explicitly passes type.isNullable(). The 'improved_code' is functionally identical to the existing code, just reformatted. This is a minor style improvement at best.	Low
	Extract nullable target creation The cast method creates a nullable target type unconditionally, but the comment states this is to prevent NPEs in downstream aggregations. However, if targetType is already nullable, calling createTypeWithNullability(targetType, true) may be redundant. Verify that this doesn't cause issues with type identity checks elsewhere in the codebase. core/src/main/java/org/opensearch/sql/expression/function/CoercionUtils.java [124-145] private static @Nullable RexNode cast(RexBuilder builder, RelDataType targetType, RexNode arg) { RelDataType argType = arg.getType(); if (!shouldCast(argType, targetType)) { return arg; } // Always make the cast target nullable. Safe cast returns null on parse failure or on a null // source, so a non-nullable declared target defeats null-propagation and leads to primitive // unboxing NPEs in downstream aggregations. + RelDataType nullableTarget = builder.getTypeFactory().createTypeWithNullability(targetType, true); if (distance(argType, targetType) != IMPOSSIBLE_WIDENING) { - return builder.makeCast( - builder.getTypeFactory().createTypeWithNullability(targetType, true), arg, true, true); + return builder.makeCast(nullableTarget, arg, true, true); } return resolveCommonType(argType, targetType) - .map( - common -> - builder.makeCast( - builder.getTypeFactory().createTypeWithNullability(common, true), - arg, - true, - true)) + .map(common -> builder.makeCast( + builder.getTypeFactory().createTypeWithNullability(common, true), + arg, + true, + true)) .orElse(null); } Suggestion importance[1-10]: 2 __ Why: The suggestion extracts nullableTarget to avoid redundant calls to createTypeWithNullability, but this is a minor optimization. The 'improved_code' only extracts the variable for the first branch, not the second (inside resolveCommonType), so it doesn't fully address the stated concern. The impact on readability and performance is minimal.	Low

---

Previous suggestions

Suggestions up to commit 1e91aa8

Category	Suggestion	Impact
Possible issue	Preserve nullability when normalizing temporal types The method normalizeTemporalToUdt does not preserve the nullability of the original type when returning the UDT. The createUDT call passes type.isNullable(), but if the original type is nullable and the UDT creation doesn't respect this flag properly, downstream operations may fail. Verify that createUDT correctly handles nullability or explicitly wrap the result with createTypeWithNullability. core/src/main/java/org/opensearch/sql/expression/function/CoercionUtils.java [169-184] private static RelDataType normalizeTemporalToUdt(RelDataType type) { if (type instanceof AbstractExprRelDataType<?>) { return type; } SqlTypeName name = type.getSqlTypeName(); ExprUDT udt = switch (name) { case TIMESTAMP -> ExprUDT.EXPR_TIMESTAMP; case DATE -> ExprUDT.EXPR_DATE; case TIME -> ExprUDT.EXPR_TIME; default -> null; }; - return udt == null - ? type - : OpenSearchTypeFactory.TYPE_FACTORY.createUDT(udt, type.isNullable()); + if (udt == null) { + return type; + } + RelDataType udtType = OpenSearchTypeFactory.TYPE_FACTORY.createUDT(udt); + return OpenSearchTypeFactory.TYPE_FACTORY.createTypeWithNullability(udtType, type.isNullable()); } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that createUDT should preserve nullability. However, the code already passes type.isNullable() to createUDT, so the issue depends on whether that method respects the flag. The suggestion to explicitly wrap with createTypeWithNullability is a valid safeguard.	Medium
	Preserve target type nullability in casts The cast logic forces all target types to be nullable, but this may not be correct when the original targetType is explicitly non-nullable by design. If the function signature requires a non-nullable result, forcing nullability here could cause type mismatches in downstream operations. Consider preserving the original nullability of targetType or documenting why forcing nullable is always safe. core/src/main/java/org/opensearch/sql/expression/function/CoercionUtils.java [124-145] private static @Nullable RexNode cast(RexBuilder builder, RelDataType targetType, RexNode arg) { RelDataType argType = arg.getType(); if (!shouldCast(argType, targetType)) { return arg; } - // Always make the cast target nullable. Safe cast returns null on parse failure or on a null - // source, so a non-nullable declared target defeats null-propagation and leads to primitive - // unboxing NPEs in downstream aggregations. + // Preserve the original nullability of targetType to avoid type mismatches if (distance(argType, targetType) != IMPOSSIBLE_WIDENING) { - return builder.makeCast( - builder.getTypeFactory().createTypeWithNullability(targetType, true), arg, true, true); + return builder.makeCast(targetType, arg, true, true); } return resolveCommonType(argType, targetType) - .map( - common -> - builder.makeCast( - builder.getTypeFactory().createTypeWithNullability(common, true), - arg, - true, - true)) + .map(common -> builder.makeCast(common, arg, true, true)) .orElse(null); } Suggestion importance[1-10]: 3 __ Why: The PR explicitly forces nullable casts to prevent NPEs in downstream aggregations, as documented in the comment. The suggestion contradicts this intentional design choice without providing evidence that the current approach is incorrect. The comment clearly explains why forcing nullable is necessary.	Low
General	Compare nullability in type matching The method typesMatch does not account for nullability differences between expected and actual. Two types with the same SqlTypeName or ExprUDT but different nullability flags may be treated as matching when they should not be. This could lead to incorrect type validation in UDF signature matching. Consider comparing nullability explicitly or document why it is intentionally ignored. core/src/main/java/org/opensearch/sql/expression/function/PPLTypeChecker.java [580-590] private static boolean typesMatch(RelDataType expected, RelDataType actual) { if (expected instanceof AbstractExprRelDataType<?> expUdt && actual instanceof AbstractExprRelDataType<?> actUdt) { - return expUdt.getUdt() == actUdt.getUdt(); + return expUdt.getUdt() == actUdt.getUdt() && expected.isNullable() == actual.isNullable(); } if (expected instanceof AbstractExprRelDataType<?> || actual instanceof AbstractExprRelDataType<?>) { return false; } - return expected.getSqlTypeName() == actual.getSqlTypeName(); + return expected.getSqlTypeName() == actual.getSqlTypeName() + && expected.isNullable() == actual.isNullable(); } Suggestion importance[1-10]: 5 __ Why: The suggestion raises a valid concern about nullability comparison in typesMatch. However, the method is used for signature matching where nullability differences may be intentionally ignored (as casts handle nullability separately). The impact depends on whether strict nullability matching is required for UDF signatures, which isn't clear from the context.	Low

Suggestions up to commit dc13ee5

Category	Suggestion	Impact
Possible issue	Fix UDT type matching logic The typesMatch method compares UDT types by Java class equality, which may fail if createTypeWithNullability returns a different class instance. Verify that both types are UDTs and compare their getUdt() tags instead of class identity to ensure correct matching. core/src/main/java/org/opensearch/sql/expression/function/PPLTypeChecker.java [575-582] private static boolean typesMatch(RelDataType expected, RelDataType actual) { + if (expected instanceof AbstractExprRelDataType<?> expectedUdt + && actual instanceof AbstractExprRelDataType<?> actualUdt) { + return expectedUdt.getUdt() == actualUdt.getUdt(); + } if (expected instanceof AbstractExprRelDataType<?> || actual instanceof AbstractExprRelDataType<?>) { - return expected.getClass() == actual.getClass(); + return false; } return expected.getSqlTypeName() == actual.getSqlTypeName(); } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that comparing UDT types by Java class equality may fail when createTypeWithNullability returns a different class instance. Comparing via getUdt() tags is more robust and aligns with the pattern used elsewhere in the PR (e.g., isComparable). This is a valid improvement to ensure correct UDT matching.	Medium
General	Prevent stack overflow in recursion The recursive distance method lacks a depth limit, which could lead to stack overflow if the parent graph contains cycles or is unexpectedly deep. Add a maximum recursion depth check to prevent potential stack overflow errors. core/src/main/java/org/opensearch/sql/expression/function/CoercionUtils.java [307-323] +private static final int MAX_DISTANCE_DEPTH = 100; + private static int distance(CoercionTag from, CoercionTag to, int acc) { + if (acc > MAX_DISTANCE_DEPTH) return IMPOSSIBLE_WIDENING; if (from == to) return acc; if (from == CoercionTag.UNKNOWN) return IMPOSSIBLE_WIDENING; if (from == CoercionTag.UNDEFINED && to != CoercionTag.UNKNOWN) return acc + 1; if (from == CoercionTag.STRING && to == CoercionTag.DOUBLE) return acc + 1; List<CoercionTag> parents = parentsOf(from); if (parents.isEmpty()) return IMPOSSIBLE_WIDENING; int best = IMPOSSIBLE_WIDENING; for (CoercionTag parent : parents) { int d = distance(parent, to, acc + 1); if (d < best) best = d; } return best; } Suggestion importance[1-10]: 5 __ Why: The suggestion addresses a potential stack overflow risk in the recursive distance method. However, the PARENTS map is statically defined and acyclic by design, making cycles impossible. The depth limit adds defensive programming value but is unlikely to trigger in practice given the small, fixed widening DAG.	Low
	Add default case to switch The switch expression lacks a default case, which means any future DataType enum values not explicitly handled will cause a compilation error. Add a default case that throws an IllegalArgumentException to ensure forward compatibility and clearer error messages for unsupported types. core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java [917-935] RelDataType nullableType = switch (node.getDataType()) { case TYPE_ERROR -> throw new IllegalArgumentException("Unsupported AST DataType: " + node.getDataType()); case NULL -> TYPE_FACTORY.createSqlType(SqlTypeName.NULL, true); case INTEGER -> TYPE_FACTORY.createSqlType(SqlTypeName.INTEGER, true); - ... + case LONG -> TYPE_FACTORY.createSqlType(SqlTypeName.BIGINT, true); + case SHORT -> TYPE_FACTORY.createSqlType(SqlTypeName.SMALLINT, true); + case FLOAT -> TYPE_FACTORY.createSqlType(SqlTypeName.REAL, true); + case DOUBLE, DECIMAL -> TYPE_FACTORY.createSqlType(SqlTypeName.DOUBLE, true); + case STRING -> TYPE_FACTORY.createSqlType(SqlTypeName.VARCHAR, true); + case BOOLEAN -> TYPE_FACTORY.createSqlType(SqlTypeName.BOOLEAN, true); + case DATE -> TYPE_FACTORY.createUDT(ExprUDT.EXPR_DATE, true); + case TIME -> TYPE_FACTORY.createUDT(ExprUDT.EXPR_TIME, true); + case TIMESTAMP -> TYPE_FACTORY.createUDT(ExprUDT.EXPR_TIMESTAMP, true); + case INTERVAL -> + throw new IllegalArgumentException( + "INTERVAL must carry a unit; cannot map to RelDataType directly."); case IP -> TYPE_FACTORY.createUDT(ExprUDT.EXPR_IP, true); + default -> + throw new IllegalArgumentException("Unsupported AST DataType: " + node.getDataType()); }; Suggestion importance[1-10]: 3 __ Why: The switch expression already handles TYPE_ERROR as the first case, which throws an exception for unsupported types. Adding a default case would be redundant since all enum values are covered, and Java's exhaustiveness checking ensures compilation errors for missing cases. The suggestion provides minimal value.	Low

Suggestions up to commit 4aebb3d

Category	Suggestion	Impact
Possible issue	Prevent unbounded recursion in distance The recursive distance method lacks a depth limit, which could cause a stack overflow if the widening DAG contains a cycle or an unexpectedly deep path. Add a maximum recursion depth check to prevent unbounded recursion. core/src/main/java/org/opensearch/sql/expression/function/CoercionUtils.java [307-323] +private static final int MAX_WIDENING_DEPTH = 20; + private static int distance(CoercionTag from, CoercionTag to, int acc) { + if (acc > MAX_WIDENING_DEPTH) return IMPOSSIBLE_WIDENING; if (from == to) return acc; if (from == CoercionTag.UNKNOWN) return IMPOSSIBLE_WIDENING; if (from == CoercionTag.UNDEFINED && to != CoercionTag.UNKNOWN) return acc + 1; if (from == CoercionTag.STRING && to == CoercionTag.DOUBLE) return acc + 1; List<CoercionTag> parents = parentsOf(from); if (parents.isEmpty()) return IMPOSSIBLE_WIDENING; int best = IMPOSSIBLE_WIDENING; for (CoercionTag parent : parents) { int d = distance(parent, to, acc + 1); if (d < best) best = d; } return best; } Suggestion importance[1-10]: 7 __ Why: The recursive distance method could theoretically cause a stack overflow if the widening DAG is misconfigured or contains an unexpected cycle. Adding a depth limit is a reasonable defensive measure that prevents potential runtime failures. The suggestion is valid and addresses a real (though unlikely) risk.	Medium
	Guard against null SqlTypeName The method calls type.getSqlTypeName() without checking if it returns null, which can occur for certain RelDataType implementations. Add a null check before the switch to prevent a NullPointerException. core/src/main/java/org/opensearch/sql/expression/function/CoercionUtils.java [169-184] private static RelDataType normalizeTemporalToUdt(RelDataType type) { if (type instanceof AbstractExprRelDataType<?>) { return type; } SqlTypeName name = type.getSqlTypeName(); + if (name == null) { + return type; + } ExprUDT udt = switch (name) { case TIMESTAMP -> ExprUDT.EXPR_TIMESTAMP; case DATE -> ExprUDT.EXPR_DATE; case TIME -> ExprUDT.EXPR_TIME; default -> null; }; return udt == null ? type : OpenSearchTypeFactory.TYPE_FACTORY.createUDT(udt, type.isNullable()); } Suggestion importance[1-10]: 6 __ Why: The method getSqlTypeName() can return null for certain RelDataType implementations. Adding a null check before the switch prevents a potential NullPointerException. The suggestion is valid and improves robustness, though the likelihood of encountering a null SqlTypeName in this context may be low.	Low
	Add default case to switch The switch expression lacks a default case, which means any unhandled DataType enum value will cause a compilation error or runtime failure. Add a default case that throws an IllegalArgumentException to ensure all unexpected values are caught explicitly. core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java [917-935] RelDataType nullableType = switch (node.getDataType()) { case TYPE_ERROR -> throw new IllegalArgumentException("Unsupported AST DataType: " + node.getDataType()); case NULL -> TYPE_FACTORY.createSqlType(SqlTypeName.NULL, true); case INTEGER -> TYPE_FACTORY.createSqlType(SqlTypeName.INTEGER, true); - ... + case LONG -> TYPE_FACTORY.createSqlType(SqlTypeName.BIGINT, true); + case SHORT -> TYPE_FACTORY.createSqlType(SqlTypeName.SMALLINT, true); + case FLOAT -> TYPE_FACTORY.createSqlType(SqlTypeName.REAL, true); + case DOUBLE, DECIMAL -> TYPE_FACTORY.createSqlType(SqlTypeName.DOUBLE, true); + case STRING -> TYPE_FACTORY.createSqlType(SqlTypeName.VARCHAR, true); + case BOOLEAN -> TYPE_FACTORY.createSqlType(SqlTypeName.BOOLEAN, true); + case DATE -> TYPE_FACTORY.createUDT(ExprUDT.EXPR_DATE, true); + case TIME -> TYPE_FACTORY.createUDT(ExprUDT.EXPR_TIME, true); + case TIMESTAMP -> TYPE_FACTORY.createUDT(ExprUDT.EXPR_TIMESTAMP, true); + case INTERVAL -> + throw new IllegalArgumentException( + "INTERVAL must carry a unit; cannot map to RelDataType directly."); case IP -> TYPE_FACTORY.createUDT(ExprUDT.EXPR_IP, true); + default -> + throw new IllegalArgumentException("Unhandled AST DataType: " + node.getDataType()); }; Suggestion importance[1-10]: 3 __ Why: The switch expression in visitCast is exhaustive over the DataType enum, so a default case is not strictly necessary. However, adding one could improve maintainability if new enum values are added later. The suggestion is valid but has low impact since the code already handles TYPE_ERROR and INTERVAL explicitly.	Low

Suggestions up to commit 4aebb3d

Category	Suggestion	Impact
General	Handle unexpected UDT types The inner switch on udt.getUdt() lacks a default case. If a new ExprUDT variant is added, the code will fail at runtime with a MatchException. Add a default case to handle unexpected UDT types gracefully. core/src/main/java/org/opensearch/sql/expression/function/CoercionUtils.java [214-223] private static CoercionTag tagOf(RelDataType type) { if (type instanceof AbstractExprRelDataType<?> udt) { return switch (udt.getUdt()) { case EXPR_DATE -> CoercionTag.DATE; case EXPR_TIME -> CoercionTag.TIME; case EXPR_TIMESTAMP -> CoercionTag.TIMESTAMP; case EXPR_IP -> CoercionTag.IP; case EXPR_BINARY -> CoercionTag.BINARY; + default -> CoercionTag.UNKNOWN; }; } SqlTypeName name = type.getSqlTypeName(); if (name == null) return CoercionTag.UNKNOWN; return switch (name) { ... }; } Suggestion importance[1-10]: 6 __ Why: The inner switch on udt.getUdt() lacks a default case, which could cause a MatchException if new ExprUDT variants are added. Adding a default case that returns CoercionTag.UNKNOWN improves robustness and future-proofing.	Low
	Prevent potential stack overflow The recursive distance method lacks a depth limit, which could lead to stack overflow if the widening DAG contains cycles or unexpectedly deep paths. Add a maximum recursion depth check to prevent potential stack overflow errors. core/src/main/java/org/opensearch/sql/expression/function/CoercionUtils.java [307-323] +private static final int MAX_WIDENING_DEPTH = 20; + private static int distance(CoercionTag from, CoercionTag to, int acc) { + if (acc > MAX_WIDENING_DEPTH) return IMPOSSIBLE_WIDENING; if (from == to) return acc; if (from == CoercionTag.UNKNOWN) return IMPOSSIBLE_WIDENING; if (from == CoercionTag.UNDEFINED && to != CoercionTag.UNKNOWN) return acc + 1; if (from == CoercionTag.STRING && to == CoercionTag.DOUBLE) return acc + 1; List<CoercionTag> parents = parentsOf(from); if (parents.isEmpty()) return IMPOSSIBLE_WIDENING; int best = IMPOSSIBLE_WIDENING; for (CoercionTag parent : parents) { int d = distance(parent, to, acc + 1); if (d < best) best = d; } return best; } Suggestion importance[1-10]: 5 __ Why: Adding a depth limit is a reasonable defensive measure. However, the widening DAG is explicitly designed to be acyclic (as documented), so stack overflow is unlikely in practice. This is a minor robustness improvement.	Low
	Add default case to switch The switch expression lacks a default case, which means unhandled enum values will cause a compilation error if new DataType variants are added. Add a default case that throws an IllegalArgumentException to ensure forward compatibility and clearer error messages. core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java [917-935] RelDataType nullableType = switch (node.getDataType()) { case TYPE_ERROR -> throw new IllegalArgumentException("Unsupported AST DataType: " + node.getDataType()); case NULL -> TYPE_FACTORY.createSqlType(SqlTypeName.NULL, true); case INTEGER -> TYPE_FACTORY.createSqlType(SqlTypeName.INTEGER, true); - ... + case LONG -> TYPE_FACTORY.createSqlType(SqlTypeName.BIGINT, true); + case SHORT -> TYPE_FACTORY.createSqlType(SqlTypeName.SMALLINT, true); + case FLOAT -> TYPE_FACTORY.createSqlType(SqlTypeName.REAL, true); + case DOUBLE, DECIMAL -> TYPE_FACTORY.createSqlType(SqlTypeName.DOUBLE, true); + case STRING -> TYPE_FACTORY.createSqlType(SqlTypeName.VARCHAR, true); + case BOOLEAN -> TYPE_FACTORY.createSqlType(SqlTypeName.BOOLEAN, true); + case DATE -> TYPE_FACTORY.createUDT(ExprUDT.EXPR_DATE, true); + case TIME -> TYPE_FACTORY.createUDT(ExprUDT.EXPR_TIME, true); + case TIMESTAMP -> TYPE_FACTORY.createUDT(ExprUDT.EXPR_TIMESTAMP, true); + case INTERVAL -> + throw new IllegalArgumentException( + "INTERVAL must carry a unit; cannot map to RelDataType directly."); case IP -> TYPE_FACTORY.createUDT(ExprUDT.EXPR_IP, true); + default -> + throw new IllegalArgumentException("Unsupported AST DataType: " + node.getDataType()); }; Suggestion importance[1-10]: 3 __ Why: The switch expression already handles TYPE_ERROR explicitly, which serves a similar purpose to a default case. Adding a redundant default case would be a minor improvement for future-proofing, but the current code is not incorrect.	Low

Suggestions up to commit 3fa07c2

Category	Suggestion	Impact
Possible issue	Fix UDT comparison for nullability The typesMatch method compares UDT types by Java class equality, but this fails when one type is nullable and the other is not, because createTypeWithNullability may return a different class. Compare UDTs by their getUdt() tag instead of class identity. core/src/main/java/org/opensearch/sql/expression/function/PPLTypeChecker.java [577-583] private static boolean typesMatch(RelDataType expected, RelDataType actual) { + if (expected instanceof AbstractExprRelDataType<?> expectedUdt + && actual instanceof AbstractExprRelDataType<?> actualUdt) { + return expectedUdt.getUdt() == actualUdt.getUdt(); + } if (expected instanceof AbstractExprRelDataType<?> || actual instanceof AbstractExprRelDataType<?>) { - return expected.getClass() == actual.getClass(); + return false; } return expected.getSqlTypeName() == actual.getSqlTypeName(); } Suggestion importance[1-10]: 8 __ Why: The current typesMatch implementation compares UDT types by Java class, which fails when createTypeWithNullability returns a different class. Comparing by getUdt() tag is more robust and aligns with the PR's intent to handle nullability correctly.	Medium
General	Prevent unbounded recursion depth The recursive distance method lacks a depth limit, which could cause stack overflow if the widening DAG contains cycles or unexpectedly deep paths. Add a maximum recursion depth check to prevent unbounded recursion. core/src/main/java/org/opensearch/sql/expression/function/CoercionUtils.java [307-323] +private static final int MAX_WIDENING_DEPTH = 20; + private static int distance(CoercionTag from, CoercionTag to, int acc) { + if (acc > MAX_WIDENING_DEPTH) return IMPOSSIBLE_WIDENING; if (from == to) return acc; if (from == CoercionTag.UNKNOWN) return IMPOSSIBLE_WIDENING; if (from == CoercionTag.UNDEFINED && to != CoercionTag.UNKNOWN) return acc + 1; if (from == CoercionTag.STRING && to == CoercionTag.DOUBLE) return acc + 1; List<CoercionTag> parents = parentsOf(from); if (parents.isEmpty()) return IMPOSSIBLE_WIDENING; int best = IMPOSSIBLE_WIDENING; for (CoercionTag parent : parents) { int d = distance(parent, to, acc + 1); if (d < best) best = d; } return best; } Suggestion importance[1-10]: 5 __ Why: The distance method is recursive and could theoretically overflow the stack if the widening DAG is misconfigured. Adding a depth limit is a reasonable safeguard, though the current DAG is shallow and acyclic.	Low
	Add default case to switch The switch expression lacks a default case, which means unhandled enum values will cause a compilation error if new DataType values are added. Add a default case that throws an exception to ensure all future enum additions are explicitly handled. core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java [918-936] RelDataType nullableType = switch (node.getDataType()) { case TYPE_ERROR -> throw new IllegalArgumentException("Unsupported AST DataType: " + node.getDataType()); case NULL -> TYPE_FACTORY.createSqlType(SqlTypeName.NULL, true); case INTEGER -> TYPE_FACTORY.createSqlType(SqlTypeName.INTEGER, true); case LONG -> TYPE_FACTORY.createSqlType(SqlTypeName.BIGINT, true); case SHORT -> TYPE_FACTORY.createSqlType(SqlTypeName.SMALLINT, true); case FLOAT -> TYPE_FACTORY.createSqlType(SqlTypeName.REAL, true); case DOUBLE, DECIMAL -> TYPE_FACTORY.createSqlType(SqlTypeName.DOUBLE, true); case STRING -> TYPE_FACTORY.createSqlType(SqlTypeName.VARCHAR, true); case BOOLEAN -> TYPE_FACTORY.createSqlType(SqlTypeName.BOOLEAN, true); case DATE -> TYPE_FACTORY.createUDT(ExprUDT.EXPR_DATE, true); case TIME -> TYPE_FACTORY.createUDT(ExprUDT.EXPR_TIME, true); case TIMESTAMP -> TYPE_FACTORY.createUDT(ExprUDT.EXPR_TIMESTAMP, true); case INTERVAL -> throw new IllegalArgumentException( "INTERVAL must carry a unit; cannot map to RelDataType directly."); case IP -> TYPE_FACTORY.createUDT(ExprUDT.EXPR_IP, true); + default -> + throw new IllegalArgumentException("Unhandled DataType: " + node.getDataType()); }; Suggestion importance[1-10]: 3 __ Why: The switch expression is exhaustive over the DataType enum. Adding a default case is defensive but not critical since the compiler enforces exhaustiveness. Minor improvement for future-proofing.	Low

[github-actions]

Persistent review updated to latest commit a385878

[github-actions]

Persistent review updated to latest commit 3fa07c2

[github-actions]

Persistent review updated to latest commit 4aebb3d

[github-actions]

Persistent review updated to latest commit 4aebb3d

[github-actions]

Persistent review updated to latest commit dc13ee5

[github-actions]

Persistent review updated to latest commit 1e91aa8

[github-actions]

Persistent review updated to latest commit 689405c