mchades commented on code in PR #12105:
URL: https://github.com/apache/gravitino/pull/12105#discussion_r3666310875
##########
spark-connector/spark-common/src/main/java/org/apache/gravitino/spark/connector/SparkTypeConverter.java:
##########
@@ -192,6 +192,11 @@ public DataType toSparkType(Type gravitinoType) {
return DataTypes.createStructType(fields);
} else if (gravitinoType instanceof Types.NullType) {
return DataTypes.NullType;
+ } else if (gravitinoType instanceof Types.ExternalType) {
+ // ExternalType represents types with no Gravitino-native mapping (e.g.
ClickHouse
+ // IPv4/IPv6, Doris LARGEINT/BITMAP, Glue unknown types). Map to
StringType so that
+ // tables containing these columns remain loadable in Spark. Users can
CAST as needed.
+ return DataTypes.StringType;
Review Comment:
I checked the Spark JDBC implementation for the Spark versions supported
here. Spark derives the table schema from `ResultSetMetaData`, consulting
`JdbcDialect.getCatalystType` first and then its generic JDBC mapping
([`JdbcUtils.getSchema`](https://github.com/apache/spark/blob/v3.5.6/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JdbcUtils.scala#L262-L312)).
For example, JDBC `BLOB` is mapped to `BinaryType`, not `StringType` ([generic
mapping](https://github.com/apache/spark/blob/v3.5.6/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JdbcUtils.scala#L178-L220)).
`JDBCTable` then uses this resolved schema for both scan and write builders
([source](https://github.com/apache/spark/blob/v3.5.6/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/jdbc/JDBCTable.scala#L43-L52)).
This means `ExternalType` does not necessarily imply that Spark has no
suitable type: Gravitino may preserve only the external type name while the
Spark JDBC dialect/driver can still resolve a native Spark type. A global
`ExternalType -> StringType` mapping discards that information; for `BLOB`, it
also changes the Spark JDBC access path from bytes to strings.
Could we avoid this global mapping? `SparkJdbcTable` already receives the
loaded `JDBCTable`, so we could pass `jdbcTable.schema()` to
`GravitinoTableInfoHelper` and reuse the matching JDBC field type only when the
Gravitino type is `ExternalType`, while retaining the Gravitino field
nullability, comment, and metadata. If no matching field exists, it would be
safer to fail explicitly rather than silently coercing it to `StringType`.
Please also add coverage for at least `ENUM -> StringType` and `BLOB ->
BinaryType`.
--
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]
