You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2022/03/04 19:26:10 UTC
[GitHub] [ignite] alex-plekhanov commented on a change in pull request #9826: IGNITE-16075 : Calcite engine. Support UUID data type
alex-plekhanov commented on a change in pull request #9826:
URL: https://github.com/apache/ignite/pull/9826#discussion_r819530611
##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/externalize/RelJsonWriter.java
##########
@@ -144,9 +144,8 @@ private void explain_(RelNode rel, List<Pair<String, Object>> values) {
}
// omit 'inputs: ["3"]' if "3" is the preceding rel
final List<Object> list = explainInputs(rel.getInputs());
- if (list.size() != 1 || !list.get(0).equals(previousId)) {
+ if (list.size() != 1 || !list.get(0).equals(previousId))
Review comment:
Redundant change
##########
File path: modules/calcite/src/main/codegen/includes/parserImpls.ftl
##########
@@ -114,6 +116,17 @@ SqlDataTypeSpec IntervalType() :
}
}
+SqlDataTypeSpec UUIDType() :
Review comment:
Let's use camel case for class names and methods (`UuidType`) here and in other places
##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/util/TypeUtils.java
##########
@@ -343,12 +346,19 @@ else if (Duration.class.equals(storageType)) {
else
throw new IgniteException("Expected DAY-TIME interval literal");
}
- else if (Period.class.equals(storageType))
+ else if (Period.class.equals(storageType)) {
if (literal instanceof SqlIntervalLiteral &&
literal.getValueAs(SqlIntervalLiteral.IntervalValue.class).getIntervalQualifier().isYearMonth())
internalVal = literal.getValueAs(Long.class).intValue();
else
throw new IgniteException("Expected YEAR-MONTH interval literal");
+ }
+ else if (UUID.class.equals(storageType)) {
Review comment:
This code only executed for `INSERT DEFAULT` statement, we need tests to make sure it works correctly (see `TableDmlIntegrationTest#testInsertDefaultValue`, `JdbcCrossEngineTest#testInsertDefaultValue`). Looks like we need some additional logic in `CacheTableDescriptorImpl#newColumnDefaultValue` to convert `UUID` to string literal and make RexNode with `CAST` operator.
##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/type/IgniteTypeFactory.java
##########
@@ -259,11 +266,26 @@ else if (type instanceof BasicSqlType || type instanceof IntervalSqlType) {
return createTypeWithNullability(createSqlIntervalType(INTERVAL_QUALIFIER_DAY_TIME), true);
else if (clazz == Period.class)
return createTypeWithNullability(createSqlIntervalType(INTERVAL_QUALIFIER_YEAR_MONTH), true);
+ else if (clazz == UUID.class)
+ return createTypeWithNullability(createUUIDType(), true);
}
return super.toSql(type);
}
+ /** @return UUID SQL type. */
+ public UUIDType createUUIDType() {
+ return new UUIDType(true);
Review comment:
`return canonize(...)`
##########
File path: modules/calcite/src/main/codegen/includes/parserImpls.ftl
##########
@@ -114,6 +116,17 @@ SqlDataTypeSpec IntervalType() :
}
}
+SqlDataTypeSpec UUIDType() :
+{
+ final Span s;
+}
+{
+ <UUID> { s = span(); }
+ {
+ return new SqlDataTypeSpec(new IgniteSqlUUIDTypeNameSpec(s.end(this)), s.pos());
Review comment:
Do we really need new type `IgniteSqlUUIDTypeNameSpec`? Looks like `SqlUserDefinedTypeNameSpec("UUID", s.end(this))` is enough.
##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/rel/IgniteIndexScan.java
##########
@@ -130,7 +130,7 @@ private IgniteIndexScan(
return visitor.visit(this);
}
- /** {@inheritDoc} */
+ /** {@inheritDoc} */
Review comment:
Redundant change
##########
File path: modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/sql/SqlDdlParserTest.java
##########
@@ -57,6 +57,16 @@
* Test suite to verify parsing of the DDL command.
*/
public class SqlDdlParserTest extends GridCommonAbstractTest {
+ /**
+ * Very simple case where only table name and a few columns are presented.
+ */
+ @Test
+ public void test() throws SqlParseException {
Review comment:
Test should have reasonable method name and javadoc
There is no checks currently in the test
##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/util/TypeUtils.java
##########
@@ -111,7 +114,7 @@ public static boolean needCast(RelDataTypeFactory factory, RelDataType fromType,
}
// Do not make a cast when we don't know specific type (ANY) of the origin node.
- if (toType.getSqlTypeName() == SqlTypeName.ANY
+ if ((toType.getSqlTypeName() == SqlTypeName.ANY && toType.getClass() != UUIDType.class)
Review comment:
What if both types are UUIDType? In this case true will be returned, but actually cast is not required.
I propose something like this:
```
if (toType.getSqlTypeName() == SqlTypeName.ANY || fromType.getSqlTypeName() == SqlTypeName.ANY)
return toType.getClass() != fromType.getClass();
```
##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/util/TypeUtils.java
##########
@@ -111,7 +114,7 @@ public static boolean needCast(RelDataTypeFactory factory, RelDataType fromType,
}
// Do not make a cast when we don't know specific type (ANY) of the origin node.
- if (toType.getSqlTypeName() == SqlTypeName.ANY
+ if ((toType.getSqlTypeName() == SqlTypeName.ANY && toType.getClass() != UUIDType.class)
Review comment:
What if both types are UUIDType? In this case true will be returned, but actually cast is not required.
I propose something like this:
```
if (toType.getSqlTypeName() == SqlTypeName.ANY || fromType.getSqlTypeName() == SqlTypeName.ANY)
return toType.getClass() != fromType.getClass();
```
##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/externalize/RelJson.java
##########
@@ -689,7 +697,7 @@ private Object toJson(AggregateCall node) {
/** */
private Object toJson(RelDataType node) {
- if (node instanceof JavaType) {
+ if (node instanceof JavaType && node.getSqlTypeName() != SqlTypeName.ANY) { // Java type except UDT.
Review comment:
Looks redundant now
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org