Copilot commented on code in PR #11715:
URL: https://github.com/apache/gravitino/pull/11715#discussion_r3433853533
##########
catalogs/catalog-jdbc-doris/src/main/java/org/apache/gravitino/catalog/doris/operation/DorisTableOperations.java:
##########
@@ -798,7 +798,8 @@ public Integer calculateDatetimePrecision(String typeName,
int columnSize, int s
String upperTypeName = typeName.toUpperCase();
// Check driver version compatibility first
- boolean isDatetimeType = "DATETIME".equals(upperTypeName);
+ boolean isDatetimeType =
+ "DATETIME".equals(upperTypeName) ||
upperTypeName.startsWith("DATETIME(");
Review Comment:
The MySQL driver compatibility check is currently applied to both "DATETIME"
and "DATETIME(N)". For "DATETIME(N)" you already parse precision from the type
string, so returning null for unsupported driver versions would incorrectly
drop the precision even though it doesn't depend on JDBC columnSize.
##########
catalogs/catalog-jdbc-doris/src/main/java/org/apache/gravitino/catalog/doris/converter/DorisTypeConverter.java:
##########
@@ -55,22 +97,200 @@ public Type toGravitino(JdbcTypeBean typeBean) {
case DOUBLE:
return Types.DoubleType.get();
case DECIMAL:
- return Types.DecimalType.of(typeBean.getColumnSize(),
typeBean.getScale());
+ {
+ // Parse precision/scale from type string when typeBean doesn't have
them
+ int columnSize = typeBean.getColumnSize();
+ int scale = typeBean.getScale();
+ if (columnSize == 0 && parenIndex > 0) {
+ String[] parts = typeName.substring(parenIndex + 1,
typeName.length() - 1).split(",");
+ columnSize = Integer.parseInt(parts[0].trim());
+ scale = parts.length >= 2 ? Integer.parseInt(parts[1].trim()) : 0;
+ }
+ return Types.DecimalType.of(columnSize, scale);
+ }
case DATE:
+ case DATEV2:
return Types.DateType.get();
case DATETIME:
- return Optional.ofNullable(typeBean.getDatetimePrecision())
- .map(Types.TimestampType::withoutTimeZone)
- .orElseGet(Types.TimestampType::withoutTimeZone);
+ // Already handled above the switch; this is a fallback
+ return Types.TimestampType.withoutTimeZone();
case CHAR:
- return Types.FixedCharType.of(typeBean.getColumnSize());
+ {
+ int columnSize = typeBean.getColumnSize();
+ if (columnSize == 0 && parenIndex > 0) {
+ columnSize =
+ Integer.parseInt(typeName.substring(parenIndex + 1,
typeName.length() - 1));
+ }
+ return Types.FixedCharType.of(columnSize > 0 ? columnSize : 1);
+ }
case VARCHAR:
- return Types.VarCharType.of(typeBean.getColumnSize());
+ {
+ int columnSize = typeBean.getColumnSize();
+ if (columnSize == 0 && parenIndex > 0) {
+ columnSize =
+ Integer.parseInt(typeName.substring(parenIndex + 1,
typeName.length() - 1));
+ }
+ return Types.VarCharType.of(columnSize > 0 ? columnSize : 255);
+ }
case STRING:
case TEXT:
return Types.StringType.get();
+ case BINARY:
+ case VARBINARY:
+ return Types.BinaryType.get();
+ case "bigint unsigned":
+ case JSON:
+ case VARIANT:
+ case IPV4:
+ case IPV6:
+ case LARGEINT:
+ case BITMAP:
+ case HLL:
+ return Types.ExternalType.of(typeName);
+ default:
+ return toGravitinoComplexType(typeBean);
+ }
+ }
+
+ /**
+ * Handle complex types (ARRAY, MAP, STRUCT) whose TYPE_NAME includes nested
type parameters, e.g.
+ * "array<text>", "map<text,int>", "struct<x:int,y:int>".
These cannot be
+ * matched by simple case labels.
+ */
+ private static Type toGravitinoComplexType(JdbcTypeBean typeBean) {
+ String typeName = typeBean.getTypeName().toLowerCase();
+
+ if (typeName.startsWith("array<") && typeName.endsWith(">")) {
+ // Parse ARRAY type: array<element_type>
+ String elementTypeStr = typeName.substring(6, typeName.length() - 1);
+ Type elementType = parseSimpleType(elementTypeStr);
+ return Types.ListType.of(elementType, true);
+ } else if (typeName.startsWith("map<") && typeName.endsWith(">")) {
+ // Parse MAP type: map<key_type,value_type>
+ String mapContent = typeName.substring(4, typeName.length() - 1);
+ int commaIndex = findCommaIndex(mapContent);
+ if (commaIndex > 0) {
+ String keyTypeStr = mapContent.substring(0, commaIndex).trim();
+ String valueTypeStr = mapContent.substring(commaIndex + 1).trim();
+ Type keyType = parseSimpleType(keyTypeStr);
+ Type valueType = parseSimpleType(valueTypeStr);
+ return Types.MapType.of(keyType, valueType, true);
+ }
+ } else if (typeName.startsWith("struct<") && typeName.endsWith(">")) {
+ // Parse STRUCT type: struct<field1:type1,field2:type2>
+ // Use depth-aware split to handle nested types like
struct<a:decimal(10,2),b:int>
+ String structContent = typeName.substring(7, typeName.length() - 1);
+ List<Types.StructType.Field> structFields = new ArrayList<>();
+ int start = 0;
+ while (start <= structContent.length()) {
+ int comma = findCommaIndex(structContent.substring(start));
+ int end = comma < 0 ? structContent.length() : start + comma;
+ String field = structContent.substring(start, end).trim();
+ int colonIndex = field.indexOf(':');
+ if (colonIndex > 0) {
+ String fieldName = field.substring(0, colonIndex).trim();
+ String fieldTypeStr = field.substring(colonIndex + 1).trim();
+ Type fieldType = parseSimpleType(fieldTypeStr);
+ structFields.add(Types.StructType.Field.of(fieldName, fieldType,
true, null));
+ }
+ if (comma < 0) break;
+ start += comma + 1;
Review Comment:
Google Java Style requires braces for control statements. Using a
single-line `if` here is inconsistent with the rest of the file and can fail
checkstyle/spotless in this repo.
##########
catalogs/catalog-jdbc-doris/src/main/java/org/apache/gravitino/catalog/doris/operation/DorisTableOperations.java:
##########
@@ -813,6 +814,17 @@ public Integer calculateDatetimePrecision(String typeName,
int columnSize, int s
}
}
+ // Handle datetime(N) format from SHOW CREATE TABLE
+ if (upperTypeName.startsWith("DATETIME(") && upperTypeName.endsWith(")")) {
+ try {
+ String precisionStr = upperTypeName.substring(9,
upperTypeName.length() - 1);
+ return Integer.parseInt(precisionStr);
+ } catch (NumberFormatException e) {
+ LOG.warn("Failed to parse datetime precision from type: {}", typeName);
+ return null;
+ }
Review Comment:
When parsing fails, the warning drops the exception, which makes diagnosing
unexpected type strings harder. Logging the NumberFormatException as the
throwable keeps the message and stack trace without changing behavior.
##########
catalogs/catalog-jdbc-doris/src/main/java/org/apache/gravitino/catalog/doris/operation/DorisTableOperations.java:
##########
@@ -813,6 +814,17 @@ public Integer calculateDatetimePrecision(String typeName,
int columnSize, int s
}
}
+ // Handle datetime(N) format from SHOW CREATE TABLE
+ if (upperTypeName.startsWith("DATETIME(") && upperTypeName.endsWith(")")) {
+ try {
+ String precisionStr = upperTypeName.substring(9,
upperTypeName.length() - 1);
+ return Integer.parseInt(precisionStr);
Review Comment:
This change adds support for parsing precision from "DATETIME(N)", but the
existing unit tests only cover the columnSize-based "DATETIME" path. Add a test
case for the new "DATETIME(N)" branch (e.g., "DATETIME(3)" and an invalid
"DATETIME(x)"), so regressions are caught.
##########
catalogs/catalog-jdbc-doris/src/main/java/org/apache/gravitino/catalog/doris/converter/DorisTypeConverter.java:
##########
@@ -55,22 +97,200 @@ public Type toGravitino(JdbcTypeBean typeBean) {
case DOUBLE:
return Types.DoubleType.get();
case DECIMAL:
- return Types.DecimalType.of(typeBean.getColumnSize(),
typeBean.getScale());
+ {
+ // Parse precision/scale from type string when typeBean doesn't have
them
+ int columnSize = typeBean.getColumnSize();
+ int scale = typeBean.getScale();
+ if (columnSize == 0 && parenIndex > 0) {
+ String[] parts = typeName.substring(parenIndex + 1,
typeName.length() - 1).split(",");
+ columnSize = Integer.parseInt(parts[0].trim());
+ scale = parts.length >= 2 ? Integer.parseInt(parts[1].trim()) : 0;
+ }
+ return Types.DecimalType.of(columnSize, scale);
+ }
case DATE:
+ case DATEV2:
return Types.DateType.get();
case DATETIME:
- return Optional.ofNullable(typeBean.getDatetimePrecision())
- .map(Types.TimestampType::withoutTimeZone)
- .orElseGet(Types.TimestampType::withoutTimeZone);
+ // Already handled above the switch; this is a fallback
+ return Types.TimestampType.withoutTimeZone();
case CHAR:
- return Types.FixedCharType.of(typeBean.getColumnSize());
+ {
+ int columnSize = typeBean.getColumnSize();
+ if (columnSize == 0 && parenIndex > 0) {
+ columnSize =
+ Integer.parseInt(typeName.substring(parenIndex + 1,
typeName.length() - 1));
+ }
+ return Types.FixedCharType.of(columnSize > 0 ? columnSize : 1);
+ }
case VARCHAR:
- return Types.VarCharType.of(typeBean.getColumnSize());
+ {
+ int columnSize = typeBean.getColumnSize();
+ if (columnSize == 0 && parenIndex > 0) {
+ columnSize =
+ Integer.parseInt(typeName.substring(parenIndex + 1,
typeName.length() - 1));
+ }
+ return Types.VarCharType.of(columnSize > 0 ? columnSize : 255);
+ }
case STRING:
case TEXT:
return Types.StringType.get();
+ case BINARY:
+ case VARBINARY:
+ return Types.BinaryType.get();
+ case "bigint unsigned":
+ case JSON:
+ case VARIANT:
+ case IPV4:
+ case IPV6:
+ case LARGEINT:
+ case BITMAP:
+ case HLL:
+ return Types.ExternalType.of(typeName);
+ default:
+ return toGravitinoComplexType(typeBean);
+ }
+ }
+
+ /**
+ * Handle complex types (ARRAY, MAP, STRUCT) whose TYPE_NAME includes nested
type parameters, e.g.
+ * "array<text>", "map<text,int>", "struct<x:int,y:int>".
These cannot be
+ * matched by simple case labels.
+ */
+ private static Type toGravitinoComplexType(JdbcTypeBean typeBean) {
+ String typeName = typeBean.getTypeName().toLowerCase();
+
+ if (typeName.startsWith("array<") && typeName.endsWith(">")) {
+ // Parse ARRAY type: array<element_type>
+ String elementTypeStr = typeName.substring(6, typeName.length() - 1);
+ Type elementType = parseSimpleType(elementTypeStr);
+ return Types.ListType.of(elementType, true);
+ } else if (typeName.startsWith("map<") && typeName.endsWith(">")) {
+ // Parse MAP type: map<key_type,value_type>
+ String mapContent = typeName.substring(4, typeName.length() - 1);
+ int commaIndex = findCommaIndex(mapContent);
+ if (commaIndex > 0) {
+ String keyTypeStr = mapContent.substring(0, commaIndex).trim();
+ String valueTypeStr = mapContent.substring(commaIndex + 1).trim();
+ Type keyType = parseSimpleType(keyTypeStr);
+ Type valueType = parseSimpleType(valueTypeStr);
+ return Types.MapType.of(keyType, valueType, true);
+ }
+ } else if (typeName.startsWith("struct<") && typeName.endsWith(">")) {
+ // Parse STRUCT type: struct<field1:type1,field2:type2>
+ // Use depth-aware split to handle nested types like
struct<a:decimal(10,2),b:int>
+ String structContent = typeName.substring(7, typeName.length() - 1);
+ List<Types.StructType.Field> structFields = new ArrayList<>();
+ int start = 0;
+ while (start <= structContent.length()) {
+ int comma = findCommaIndex(structContent.substring(start));
+ int end = comma < 0 ? structContent.length() : start + comma;
+ String field = structContent.substring(start, end).trim();
+ int colonIndex = field.indexOf(':');
+ if (colonIndex > 0) {
+ String fieldName = field.substring(0, colonIndex).trim();
+ String fieldTypeStr = field.substring(colonIndex + 1).trim();
+ Type fieldType = parseSimpleType(fieldTypeStr);
+ structFields.add(Types.StructType.Field.of(fieldName, fieldType,
true, null));
+ }
+ if (comma < 0) break;
+ start += comma + 1;
+ }
+ if (!structFields.isEmpty()) {
+ return Types.StructType.of(structFields.toArray(new
Types.StructType.Field[0]));
+ }
+ }
+
+ // Fallback to ExternalType for unknown complex types
+ return Types.ExternalType.of(typeBean.getTypeName());
+ }
+
+ /**
+ * Find the index of the comma that separates key and value types in MAP
type, or fields in
+ * STRUCT. Handles nested types like "map<struct<a:int>,int>"
and decimal(10,2) by
+ * tracking both angle-bracket and parenthesis depth.
+ */
+ private static int findCommaIndex(String s) {
+ int depth = 0;
+ for (int i = 0; i < s.length(); i++) {
+ char c = s.charAt(i);
+ if (c == '<' || c == '(') {
+ depth++;
+ } else if (c == '>' || c == ')') {
+ depth--;
+ } else if (c == ',' && depth == 0) {
+ return i;
+ }
+ }
+ return -1;
+ }
+
+ /** Parse simple type string to Gravitino Type. */
+ private static Type parseSimpleType(String typeStr) {
+ // Handle types with precision like int(11), bigint(20)
+ String baseType = typeStr;
+ int parenIndex = typeStr.indexOf('(');
+ if (parenIndex > 0) {
+ baseType = typeStr.substring(0, parenIndex);
+ }
+
+ switch (baseType) {
+ case "boolean":
+ return Types.BooleanType.get();
+ case "tinyint":
+ return Types.ByteType.get();
+ case "smallint":
+ return Types.ShortType.get();
+ case "int":
+ case "integer":
+ return Types.IntegerType.get();
+ case "bigint":
+ return Types.LongType.get();
+ case "float":
+ return Types.FloatType.get();
+ case "double":
+ return Types.DoubleType.get();
+ case "date":
+ case "datev2":
+ return Types.DateType.get();
+ case "datetime":
+ if (parenIndex > 0) {
+ String precisionStr = typeStr.substring(parenIndex + 1,
typeStr.length() - 1);
+ int precision = Integer.parseInt(precisionStr);
+ return Types.TimestampType.withoutTimeZone(precision);
+ }
+ return Types.TimestampType.withoutTimeZone();
+ case "string":
+ case "text":
+ return Types.StringType.get();
+ case "binary":
+ case "varbinary":
+ return Types.BinaryType.get();
+ case "varchar":
+ if (parenIndex > 0) {
+ int length = Integer.parseInt(typeStr.substring(parenIndex + 1,
typeStr.length() - 1));
+ return Types.VarCharType.of(length);
+ }
+ return Types.VarCharType.of(255);
+ case "char":
+ if (parenIndex > 0) {
+ int length = Integer.parseInt(typeStr.substring(parenIndex + 1,
typeStr.length() - 1));
+ return Types.FixedCharType.of(length);
+ }
+ return Types.FixedCharType.of(1);
+ case "decimal":
+ if (parenIndex > 0) {
+ String[] parts = typeStr.substring(parenIndex + 1, typeStr.length()
- 1).split(",");
+ int precision = Integer.parseInt(parts[0].trim());
+ int scale = parts.length >= 2 ? Integer.parseInt(parts[1].trim()) :
0;
+ return Types.DecimalType.of(precision, scale);
+ }
+ return Types.DecimalType.of(10, 0);
default:
- return Types.ExternalType.of(typeBean.getTypeName());
+ // Unknown nested type — preserve as external type rather than
silently mapping to String.
+ // TODO: recursively resolve nested complex types (e.g.
array<array<int>>, map<string,
+ // array<int>>) by delegating to toGravitinoComplexType instead of
falling back here.
Review Comment:
Avoid leaving TODOs without an issue reference. Either implement the
recursion now or link the TODO to an issue/PR so it can be tracked.
##########
catalogs/catalog-jdbc-doris/src/main/java/org/apache/gravitino/catalog/doris/converter/DorisTypeConverter.java:
##########
@@ -123,6 +343,29 @@ public String fromGravitino(Type type) {
return CHAR + "(" + ((Types.FixedCharType) type).length() + ")";
} else if (type instanceof Types.StringType) {
return STRING;
+ } else if (type instanceof Types.BinaryType) {
+ return BINARY;
+ } else if (type instanceof Types.ListType) {
+ Types.ListType listType = (Types.ListType) type;
+ return ARRAY + "<" + fromGravitino(listType.elementType()) + ">";
+ } else if (type instanceof Types.MapType) {
+ Types.MapType mapType = (Types.MapType) type;
+ return MAP
+ + "<"
+ + fromGravitino(mapType.keyType())
+ + ","
+ + fromGravitino(mapType.valueType())
+ + ">";
+ } else if (type instanceof Types.StructType) {
+ Types.StructType structType = (Types.StructType) type;
+ StringBuilder sb = new StringBuilder(STRUCT + "<");
+ for (int i = 0; i < structType.fields().length; i++) {
+ if (i > 0) sb.append(",");
+ Types.StructType.Field field = structType.fields()[i];
Review Comment:
Google Java Style requires braces for control statements. Please wrap this
single-line `if` in braces to match the repo's style and avoid checkstyle
failures.
--
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]
