korlov42 commented on code in PR #1777:
URL: https://github.com/apache/ignite-3/pull/1777#discussion_r1136655075
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/IgniteTypeCoercion.java:
##########
@@ -291,6 +291,33 @@ private RelDataType syncAttributes(
return syncedType;
}
+ /** {@inheritDoc} **/
+ @Override
+ public @Nullable RelDataType commonTypeForBinaryComparison(@Nullable
RelDataType type1, @Nullable RelDataType type2) {
+ if (type1 == null || type2 == null) {
+ return null;
+ }
+
+ if (type1 instanceof IgniteCustomType) {
+ return tryCustomTypeCoercionRules(type1, type2);
+ } else if (type2 instanceof IgniteCustomType) {
+ return tryCustomTypeCoercionRules(type2, type1);
+ } else {
+ return super.commonTypeForBinaryComparison(type1, type2);
+ }
+ }
+
+ private @Nullable RelDataType tryCustomTypeCoercionRules(RelDataType
type1, RelDataType type2) {
+ IgniteCustomType to = (IgniteCustomType) type1;
Review Comment:
this is potential ClassCastException. Let's change the signature in a way to
make it clear that only CustomType should be passed as first argument. You have
already done all necessary verifications on a caller site to make type casting
safe
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/type/IgniteTypeFactory.java:
##########
@@ -375,28 +373,31 @@ public Type getResultClass(RelDataType type) {
assert resultType instanceof BasicSqlType : "leastRestrictive is
expected to return a new instance of a type: " + resultType;
IgniteCustomType firstCustomType = null;
- SqlTypeFamily sqlTypeFamily = null;
+ boolean hasAnyType = false;
for (var type : types) {
if (type instanceof IgniteCustomType) {
- var customType = (IgniteCustomType) type;
-
if (firstCustomType == null) {
firstCustomType = (IgniteCustomType) type;
- } else if
(!Objects.equals(firstCustomType.getCustomTypeName(),
customType.getCustomTypeName())) {
- // IgniteCustomType: Conversion between custom data
types is not supported.
- return null;
+ } else {
+ IgniteCustomType customType = (IgniteCustomType) type;
+ if
(!Objects.equals(firstCustomType.getCustomTypeName(),
customType.getCustomTypeName())) {
+ // IgniteCustomType: Conversion between custom
data types is not supported.
+ return null;
+ } else {
+ return firstCustomType;
Review Comment:
you stop analysing types as soon as you found another matching custom type,
ignoring the rest of the types. This doesn't look correct to me. Please take a
look on the test below:
```
// LeastRestrictiveTypesTest.java
@Test
public void test() {
RelDataType uuidType = TYPE_FACTORY.createCustomType(UuidType.NAME);
RelDataType intType =
TYPE_FACTORY.createSqlType(SqlTypeName.INTEGER);
RelDataType resultType =
TYPE_FACTORY.leastRestrictive(List.of(uuidType, uuidType, intType));
assertNull(resultType);
}
```
I would expect there is no least restrictive type between UUID and Integer,
but currently UUID is returned
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/IgniteTypeCoercion.java:
##########
@@ -291,6 +291,33 @@ private RelDataType syncAttributes(
return syncedType;
}
+ /** {@inheritDoc} **/
+ @Override
+ public @Nullable RelDataType commonTypeForBinaryComparison(@Nullable
RelDataType type1, @Nullable RelDataType type2) {
+ if (type1 == null || type2 == null) {
+ return null;
+ }
+
+ if (type1 instanceof IgniteCustomType) {
+ return tryCustomTypeCoercionRules(type1, type2);
+ } else if (type2 instanceof IgniteCustomType) {
+ return tryCustomTypeCoercionRules(type2, type1);
+ } else {
+ return super.commonTypeForBinaryComparison(type1, type2);
+ }
+ }
+
+ private @Nullable RelDataType tryCustomTypeCoercionRules(RelDataType
type1, RelDataType type2) {
+ IgniteCustomType to = (IgniteCustomType) type1;
+ // IgniteCustomType: If type1 is a custom data type that can be
converted to type2,
Review Comment:
> If type1 is a custom data type that can be converted to type2,
this sounds like we need to verify that type1 is can be converted to type2.
In fact, we are going to make conversion vice versa (type2 -> type1)
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItImplicitCastsTest.java:
##########
@@ -46,7 +54,10 @@ public void dropTables() {
public void testFilter(ColumnPair columnPair) {
prepareTables(columnPair);
- assertQuery("SELECT T11.c2 FROM T11 WHERE T11.c2 > 1.0").check();
+ String value = columnPair.lhsLiteral(0);
+ String query = format("SELECT T11.c2 FROM T11 WHERE T11.c2 > CAST({}
AS {})", value, columnPair.rhs);
Review Comment:
`CAST({} AS {})` -- this doesn't look like _implicit_ cast anymore
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/planner/ImplicitCastsTest.java:
##########
@@ -82,62 +82,30 @@ public void testNestedLoop(RelDataType lhs, RelDataType
rhs, ExpectedTypes expec
/** Filter clause - casts are added to condition operands. **/
@ParameterizedTest
@MethodSource("filterTypes")
- public void testFilter(RelDataType lhs, ExpectedTypes expected) throws
Exception {
+ public void testFilter(RelDataType lhs, RelDataType rhs, ExpectedTypes
expected) throws Exception {
IgniteSchema igniteSchema = new IgniteSchema("PUBLIC");
addTable(igniteSchema, "A1", "COL1", lhs);
- assertPlan("SELECT * FROM A1 WHERE COL1 > 1", igniteSchema,
isInstanceOf(IgniteTableScan.class)
+ // Parameter types are not checked during the validation phase.
+ List<Object> params = List.of("anything");
+
+ assertPlan("SELECT * FROM A1 WHERE COL1 > CAST(? AS " + rhs + ")",
igniteSchema, isInstanceOf(IgniteTableScan.class)
Review Comment:
`CAST(? AS " + rhs + ")"` -- this is not _implicit_ cast
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
