Use Calcite's validation system for type checking & coercion#4892
Use Calcite's validation system for type checking & coercion#4892yuancu wants to merge 112 commits intoopensearch-project:mainfrom
Conversation
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughIntroduces extensive Calcite-based PPL validation and type-coercion plumbing, new PPL-specific validator/convertlets/coercion rules, UDT/type utilities, many UDF operand-metadata updates, Rex/Rel shuttles and Rel↔Sql converters, removal of legacy coercion/type-checker code, and large test/expected-output updates across integ and unit tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Rel as RelNode (PPL plan)
participant Shuttle as PplRelToSqlRelShuttle
participant RelToSql as OpenSearchRelToSqlConverter
participant Validator as SqlValidator / PplValidator
participant SqlToRel as OpenSearchSqlToRelConverter
participant Planner as QueryService (convertToCalcitePlan)
Rel->>Shuttle: traverse & normalize RexLiterals
Shuttle->>RelToSql: convert RelNode -> SqlNode
RelToSql->>Validator: validate SqlNode (type coercion, implicit casts)
Validator-->>SqlToRel: produce validated SqlNode
SqlToRel->>Planner: convert validated SqlNode -> RelNode (validated RelNode)
Planner->>Planner: continue planning / optimize using validated RelNode
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
yuancu
force-pushed
the
issues/4636
branch
5 times, most recently
from
e085f81 to
fc6dd27
Compare
…checking Signed-off-by: Yuanchun Shen <yuanchu@amazon.com> # Conflicts: # core/src/main/java/org/opensearch/sql/executor/QueryService.java # Conflicts: # core/src/main/java/org/opensearch/sql/executor/QueryService.java
Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>
Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>
… logics Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>
Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>
Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>
Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>
- 2 more ITs passed in PPLBuiltinFunctionIT Signed-off-by: Yuanchun Shen <yuanchu@amazon.com> # Conflicts: # core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java
- this fix testRand, where desrialization of sarg does not restore its type - todo: update the toRex in ExtendedRelJson to the align with the latest version Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>
…estamp; (time, timestamp) -> timestamp (1240/1599) Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>
…2/1872) Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>
- allow type cast - rewrite call to sql compare to custom ip comapre Signed-off-by: Yuanchun Shen <yuanchu@amazon.com> # Conflicts: # core/src/main/java/org/opensearch/sql/expression/function/PPLBuiltinOperators.java
Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>
Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>
…1356/1599 | 1476/1915) Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>
…d in mvindex's implementation (1580/2015) Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>
…iting (1579/2015) Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>
…pe hint Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>
Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>
Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>
…e inference (1701/2015) Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>
Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>
Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In
`@core/src/test/java/org/opensearch/sql/calcite/validate/OpenSearchSparkSqlDialectTest.java`:
- Around line 30-41: Add two unit tests in OpenSearchSparkSqlDialectTest that
call OpenSearchSparkSqlDialect.DEFAULT.getCastSpec: one that passes a non‑IP
RelDataType (e.g., TYPE_FACTORY.createSqlType(SqlTypeName.INTEGER)) and asserts
the method returns null (or not a SqlDataTypeSpec) for unsupported types, and
another that calls getCastSpec(null) and asserts it throws a
NullPointerException (using assertThrows). Reference the existing test pattern
that inspects SqlDataTypeSpec (castSpec/SqlDataTypeSpec) so the new tests mirror
the IP test but cover non‑IP and null/boundary cases.
In
`@core/src/test/java/org/opensearch/sql/calcite/validate/PplTypeCoercionRuleTest.java`:
- Around line 40-50: The test testVarcharToOtherCoercion's comment is incorrect:
it states "coercible from OTHER" while the code checks
mapping.get(SqlTypeName.OTHER) contains VARCHAR/CHAR (i.e., VARCHAR can be
coerced to OTHER); update the inline comment above the assertion to correctly
read that VARCHAR is coercible to OTHER (or reword to "OTHER should allow
coercion to VARCHAR/CHAR") so the comment matches the logic in
PplTypeCoercionRule.instance() and mapping.get(SqlTypeName.OTHER).
In
`@core/src/test/java/org/opensearch/sql/calcite/validate/shuttles/SkipRelValidationShuttleTest.java`:
- Around line 33-36: Add unit tests to SkipRelValidationShuttleTest to cover
NULL, empty, and invalid cases for the skip predicate logic: add tests that pass
null operands to the shuttle (e.g., simulate a RexCall or RexNode with a null
operand), tests that pass an empty operand list, and negative tests for invalid
aggregate-input shapes (e.g., a single CASE expression or a non-project input)
to assert the shuttle rejects or throws as expected; create clearly named test
methods (e.g., testSkipPredicateWithNullOperand,
testSkipPredicateWithEmptyOperands, testSkipPredicateWithInvalidAggregateInput)
and exercise the same validation paths used by the existing happy-path tests so
you get NULL/boundary/error coverage for SkipRelValidationShuttle and relevant
methods in that class.
In
`@core/src/test/java/org/opensearch/sql/calcite/validate/ValidationUtilsTest.java`:
- Around line 209-232: Add a unit test in ValidationUtilsTest to cover a
null-exception input to ValidationUtils.tolerantValidationException: create a
test method (e.g., testTolerantValidationExceptionNullException) that passes
null as the exception argument and asserts the expected boolean result (likely
false based on other tests); place it alongside the existing tests in the same
class so the null-exception case is covered per the NULL input testing
guideline.
- Around line 98-207: Add null-input unit tests for
ValidationUtils.createUDTWithAttributes: verify that passing sourceType == null
and typeName == null to both overloads (the one taking ExprUDT and the one
taking SqlTypeName) throws a NullPointerException (use
assertThrows(NullPointerException.class, ...)). Add four tests in
ValidationUtilsTest such as testCreateUDTWithAttributes_NullSourceType_ExprUDT,
testCreateUDTWithAttributes_NullTypeName_ExprUDT,
testCreateUDTWithAttributes_NullSourceType_SqlTypeName, and
testCreateUDTWithAttributes_NullTypeName_SqlTypeName that call
ValidationUtils.createUDTWithAttributes(TYPE_FACTORY or null,
null/ExprUDT.EXPR_DATE) and the SqlTypeName variant accordingly and assert the
expected exception.
🧹 Nitpick comments (3)
core/src/test/java/org/opensearch/sql/calcite/validate/shuttles/SqlRewriteShuttleTest.java (2)
36-94: Consider adding NULL input test for identifier visit.Per coding guidelines, NULL input tests should be included for all new functions. Consider adding a test for how
shuttle.visit()handles anullSqlIdentifier input, or document that null inputs are not expected at this layer.`@Test` public void testVisitIdentifierWithNull() { SqlNode result = shuttle.visit((SqlIdentifier) null); // Assert expected behavior (null return, exception, etc.) }
174-189: Consider adding symmetric test for NOT IN with non-SqlNodeList.For completeness and symmetry with the IN tests, consider adding a test that verifies NOT IN with a non-SqlNodeList first operand also remains unchanged.
`@Test` public void testVisitCallNotInWithNonSqlNodeListUnchanged() { SqlIdentifier column = new SqlIdentifier("my_column", SqlParserPos.ZERO); SqlIdentifier subquery = new SqlIdentifier("subquery", SqlParserPos.ZERO); SqlBasicCall notInCall = new SqlBasicCall(SqlStdOperatorTable.NOT_IN, List.of(column, subquery), SqlParserPos.ZERO); SqlNode result = shuttle.visit(notInCall); assertInstanceOf(SqlBasicCall.class, result); SqlBasicCall resultCall = (SqlBasicCall) result; assertEquals(SqlKind.NOT_IN, resultCall.getKind()); assertInstanceOf(SqlIdentifier.class, resultCall.getOperandList().get(0)); }core/src/test/java/org/opensearch/sql/calcite/validate/PplTypeCoercionRuleTest.java (1)
17-67: Consider adding tests forlenientInstance()andassignmentInstance().The
PplTypeCoercionRuleclass exposes three public factory methods:instance(),lenientInstance(), andassignmentInstance(). Currently, onlyinstance()is tested. Adding similar tests for the other two would ensure the PPL-specific mappings are correctly merged with bothSqlTypeCoercionRule.lenientInstance()andSqlTypeAssignmentRule.instance()base mappings.
core/src/test/java/org/opensearch/sql/calcite/validate/OpenSearchSparkSqlDialectTest.java
Show resolved
Hide resolved
core/src/test/java/org/opensearch/sql/calcite/validate/PplTypeCoercionRuleTest.java
Show resolved
Hide resolved
...src/test/java/org/opensearch/sql/calcite/validate/shuttles/SkipRelValidationShuttleTest.java
Show resolved
Hide resolved
core/src/test/java/org/opensearch/sql/calcite/validate/ValidationUtilsTest.java
Show resolved
Hide resolved
core/src/test/java/org/opensearch/sql/calcite/validate/ValidationUtilsTest.java
Show resolved
Hide resolved
Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
`@core/src/test/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactoryTest.java`:
- Around line 173-184: The test method
testLeastRestrictive_ipUdtAndOther_returnsIpUdt is using SqlTypeName.NULL for
the second type which duplicates the IP+NULL scenario; change the second type to
SqlTypeName.OTHER by creating RelDataType otherType =
TYPE_FACTORY.createSqlType(SqlTypeName.OTHER) and pass List.of(ipUdt, otherType)
into TYPE_FACTORY.leastRestrictive so the code exercises the IP+OTHER branch;
update assertions remain the same (use OpenSearchTypeUtil.isIp and
result.isNullable()) and ensure the variable name (otherType) replaces nullType
in that call.
- Around line 20-22: Add an integration-style test method (e.g.,
testLeastRestrictiveViaSqlPipeline) to OpenSearchTypeFactoryTest that drives
leastRestrictive through Calcite's SQL->validation->Rel conversion pipeline:
construct a test schema with the same UDT types used by the existing unit tests,
parse a simple SQL SELECT that uses expressions/columns with those UDTs, run the
SqlValidator (using the test's SqlValidator and
OpenSearchSqlOperatorTable/SqlStdOperatorTable), convert the validated SqlNode
to a RelNode via SqlToRelConverter, and assert that the resulting RelNode types
(or the validator/typeFactory outputs) resolved the common type using
leastRestrictive as expected; reference the class OpenSearchTypeFactoryTest, the
type factory instance used in existing tests, and the Calcite pipeline classes
SqlParser/SqlValidator/SqlToRelConverter to locate where to integrate this new
test.
- Around line 24-278: Add three unit tests to OpenSearchTypeFactoryTest that
cover null/empty inputs for TYPE_FACTORY.leastRestrictive: (1)
testLeastRestrictive_nullInput_returnsNull — call
TYPE_FACTORY.leastRestrictive(null) and assertNull(result); (2)
testLeastRestrictive_listWithNullElement_returnsNull — call
TYPE_FACTORY.leastRestrictive(List.of(TYPE_FACTORY.createUDT(ExprUDT.EXPR_DATE),
null)) and assertNull(result); (3) testLeastRestrictive_emptyList_returnsNull —
call TYPE_FACTORY.leastRestrictive(List.of()) and assertNull(result). Name each
test accordingly and use the existing TYPE_FACTORY and ExprUDT symbols so they
integrate with the current test suite.
In
`@core/src/test/java/org/opensearch/sql/calcite/validate/PplTypeCoercionTest.java`:
- Around line 101-113: The test testImplicitCast_preservesNullability currently
checks nullableResult but misses asserting nonNullableResult's nullability;
update the test in PplTypeCoercionTest (method
testImplicitCast_preservesNullability) to assert that nonNullableResult is
non-nullable by adding an assertion like
assertFalse(nonNullableResult.isNullable()) after the existing checks on
nonNullableResult, using the same typeCoercion.implicitCast invocation to verify
non-nullable input yields non-nullable output.
🧹 Nitpick comments (5)
core/src/test/java/org/opensearch/sql/calcite/utils/OpenSearchTypeUtilTest.java (2)
19-45: Consider adding NULL input tests for type-checking methods.Only
isScalar(line 342) includes a NULL input test. Per the coding guidelines, NULL input tests should be included for all new functions. Consider adding NULL input tests forisUserDefinedType,isNumericOrCharacter,isDatetime,isDate,isTimestamp,isTime,isCharacter,isIp, andisBinaryto verify behavior when passednull.Example NULL input tests to add
`@Test` public void testIsUserDefinedType_withNull_returnsFalse() { assertFalse(OpenSearchTypeUtil.isUserDefinedType(null)); } `@Test` public void testIsNumericOrCharacter_withNull_returnsFalse() { assertFalse(OpenSearchTypeUtil.isNumericOrCharacter(null)); } `@Test` public void testIsDatetime_withNull_returnsFalse() { assertFalse(OpenSearchTypeUtil.isDatetime(null)); } // Similar tests for isDate, isTimestamp, isTime, isCharacter, isIp, isBinaryAs per coding guidelines: "NULL input tests must be included for all new functions".
8-10: Add missing import forjava.util.List.
java.util.List.ofis used inline at lines 369 and 372. Consider adding an import for consistency with the other imports.Suggested import
import static org.junit.jupiter.api.Assertions.assertFalse; 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;Then update lines 369-372:
- java.util.List.of( + List.of( TYPE_FACTORY.createSqlType(SqlTypeName.INTEGER), TYPE_FACTORY.createSqlType(SqlTypeName.VARCHAR)), - java.util.List.of("id", "name")); + List.of("id", "name"));core/src/test/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactoryTest.java (1)
24-25: Align test method names with the required pattern.Current names use underscores; the guideline expects
test<Functionality><Condition>(camel-cased) naming. Please rename consistently (e.g.,testLeastRestrictiveDateUdtsOnlyReturnsDateUdt).As per coding guidelines, "Test naming should follow pattern test".
core/src/test/java/org/opensearch/sql/calcite/validate/shuttles/PplRelToSqlRelShuttleTest.java (2)
94-201: Consider adding boundary condition tests for interval literals.Per testing guidelines, boundary condition tests (min/max values, zero, negative inputs) should be included for new functions. The interval tests use specific positive values but don't cover edge cases:
- Zero interval value (e.g.,
INTERVAL 0 DAY)- Negative interval value (if supported)
- Very large interval values near overflow boundaries
These edge cases could reveal issues with arithmetic operations in the transformation logic.
259-294: Test helper duplicates logic rather than testing actual class.The
createRexShuttlehelper reimplements the exact transformation logic fromPplRelToSqlRelShuttle's internalvisitLiteralmethod rather than delegating to the actual class. While this approach isolates and tests the algorithm correctly, it doesn't verify thatPplRelToSqlRelShuttleitself functions as intended—if the production implementation diverges from this test copy, tests would still pass.Consider refactoring to instantiate
PplRelToSqlRelShuttleand invoke its behavior directly, rather than reimplementing its logic inline in tests.
core/src/test/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactoryTest.java
Show resolved
Hide resolved
core/src/test/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactoryTest.java
Show resolved
Hide resolved
core/src/test/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactoryTest.java
Show resolved
Hide resolved
core/src/test/java/org/opensearch/sql/calcite/validate/PplTypeCoercionTest.java
Show resolved
Hide resolved
Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>
Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>
|
This PR is too large to review, is it possible to separate into multiple smaller ones? |
|
This PR is stalled because it has been open for 2 weeks with no activity. |
PR Code Analyzer ❗AI-powered 'Code-Diff-Analyzer' found issues on commit 5723ced. 'Diff too large, requires skip by maintainers after manual review' Pull Requests Author(s): Please update your Pull Request according to the report above. Repository Maintainer(s): You can Thanks. |
PR Code Analyzer ❗AI-powered 'Code-Diff-Analyzer' found issues on commit 3fd9714. 'Diff too large, requires skip by maintainers after manual review' Pull Requests Author(s): Please update your Pull Request according to the report above. Repository Maintainer(s): You can Thanks. |
PR Code Analyzer ❗AI-powered 'Code-Diff-Analyzer' found issues on commit 5723ced. 'Diff too large, requires skip by maintainers after manual review' Pull Requests Author(s): Please update your Pull Request according to the report above. Repository Maintainer(s): You can Thanks. |
PR Code Analyzer ❗AI-powered 'Code-Diff-Analyzer' found issues on commit 2c831f0. 'Diff too large, requires skip by maintainers after manual review' Pull Requests Author(s): Please update your Pull Request according to the report above. Repository Maintainer(s): You can Thanks. |
…path PRs Merge the validation branch with origin/main, resolving conflicts caused by the revert of dynamic column support (opensearch-project#5139). Spath-related files (DynamicFieldsHelper, AppendFunctionImpl, spath explain YAMLs) are deleted to align with the revert. All other conflicts resolved to preserve the validation round-trip changes from the branch. Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>
- Add missing import for OpenSearchSparkSqlDialect in CalcitePPLNoMvTest (class moved from ppl to core/validate package) - Update CalcitePPLSpathTest.testSpathAutoExtractModeWithEval expected plan to match validation-deferred behavior (no eager SAFE_CAST) - Update CalcitePPLNoMvTest.testNoMvNonExistentField to accept "inferred array element type" error message Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>
The validation round-trip (RelNode → SQL → validate → RelNode) introduces plan changes such as extra LogicalProject nodes, type annotations on MAP keys, and reordered aggregate operands. Update YAML expected outputs and test assertions to match. - Catch Throwable (not just Exception) in validate() for AssertionError from unsupported RelNodes like LogicalGraphLookup - Update 17 YAML expected output files for calcite and calcite_no_pushdown - Add 4 new alternative YAML files for non-deterministic plan variants - Add separate YAML for boolean string literal filter test - Accept "inferred array element type" error in NoMv missing field test Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>
Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>
PR Code Analyzer ❗AI-powered 'Code-Diff-Analyzer' found issues on commit 84f53df. 'Diff too large, requires skip by maintainers after manual review' Pull Requests Author(s): Please update your Pull Request according to the report above. Repository Maintainer(s): You can Thanks. |
PR Split PlanAs suggested by @LantaoJin, this PR has been split into 4 sub-PRs targeting the
Merge order: #5213 and #5214 can be reviewed/merged in parallel → #5215 → #5216 → merge Reviewer guide:
|
Please review the sub-PRs instead:
This PR will be closed once all sub-PRs are merged and |
4 participants
Description
Please review the sub-PRs instead: #4892 (comment)
This PR migrates from our custom type checking mechanism to Apache Calcite's native validation system by introducing a SqlNode validation layer. This addresses the lack of a proper SQL validation phase and resolves compatibility issues with user-defined types (UDTs).
This implements Approach 1 described in #3865.
How It Works
The PR introduces a validation layer in the query execution pipeline:
This approach leverages Calcite's robust type system while preserving OpenSearch's custom type semantics through careful type mapping and restoration.
Benefits
sqrt(x)→pow(x, 0.5)) before generating physical plansWork Items / Implementation Details
Core Infrastructure
deriveType,coerceOperandType, and type inference methodsType System Enhancements
SqlTypeNameenum with OpenSearch UDTs through dynamic type mapping:EXPR_TIME→SqlTypeName.TIME) before validationcommonTypeForBinaryComparisonto enable datetime cross-type comparisons (DATE vs TIME, etc.)SAFE_CASTfor string-to-number conversions to tolerate malformatted data; useCASTfor literal numbersFunction & Operator Handling
array_slice,reduce,mvappend, etc.) with proper type inferencejson_extract,json_set, etc.)atanoverloading, percentile approximations)DISTINCT_COUNT_APPROX,COUNT(*)rewriting, etc.)ADDoperator: String concatenation vs numeric additionATAN: Single-operand vs two-operand versionsGEOIP: String overrides due to UDT erasureSqlKindforDIVIDEandMODUDFsQuery Construct Support
RelHintthrough conversions (addedSqlHintforLogicalAggregate)bucket_nullableflagsIN/NOT INwith tuple inputs via row constructor rewritingDialect & Compatibility
LogicalValuesfor empty row generationEdge Cases & Fixes
SAFE_CASTRelToSqlConverteris instantiated per-use (stateful component)Test Fixes
CalcitePPLDateTimeBuiltinFunctionIT: Interval semanticsCalcitePPLBuiltinFunctionIT: LOG function, sarg deserializationCalciteArrayFunctionIT: Type checkers, reduce function inferenceCalciteMathematicalFunctionIT,CalcitePPLAggregationITCalciteBinCommandIT: Timestamp operations, windowed aggregates in GROUP BYCalciteStreamstatsCommandIT: Sort columns, bucket_nullableCalcitePPLJsonBuiltinFunctionIT: String conversionCode Cleanup
PPLFuncImpTableUDFOperandMetadata.wrapUDTinterfaceCalciteFuncSignature,PPLTypeCheckerEnhancedCoalesceinto built-inCoalesceOptimizations
SAFE_CASTon non-string literal numbers (useCASTfor better performance)dedupoptimization issuesPerformance Impact
Profiled on personal laptop, each test runs twice, then averaged.
Conclusion: No significant performance degradation. ClickBench shows slight overhead (+12.3%) during analyze phase due to validation, but big5 and TPCH show improvements, likely from better query optimization enabled by proper type information.
Related Issues
Resolves #4636, resolves #3865
Check List
--signoffor-s.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.