Copilot commented on code in PR #13042:
URL: https://github.com/apache/gravitino/pull/13042#discussion_r3966943603
##########
spark-connector/spark-common/src/main/java/org/apache/gravitino/spark/connector/jdbc/SparkJdbcTypeConverter.java:
##########
@@ -24,14 +24,26 @@
import org.apache.gravitino.spark.connector.SparkTypeConverter;
import org.apache.spark.sql.types.DataType;
import org.apache.spark.sql.types.DataTypes;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
public class SparkJdbcTypeConverter extends SparkTypeConverter {
+ private static final Logger LOG =
LoggerFactory.getLogger(SparkJdbcTypeConverter.class);
+
@Override
public DataType toSparkType(Type gravitinoType) {
if (gravitinoType instanceof Types.VarCharType) {
// Spark's JDBC dialects reject VarcharType, so widen it to StringType.
return DataTypes.StringType;
+ } else if (gravitinoType instanceof Types.ExternalType) {
+ // An external type carries a source type that Gravitino cannot
represent, such as an
+ // unconstrained PostgreSQL numeric. Reading it as a string keeps an
unmapped type from
+ // failing the type conversion of the whole table.
+ LOG.warn(
Review Comment:
Similar to the Trino transformer, this warning is emitted during conversion
and may fire repeatedly for common operations (schema inference / table load),
producing excessive logs. Consider lowering severity (e.g., `debug`) or logging
once per distinct external type to avoid log spam while still providing
observability.
##########
flink-connector/flink-common/src/main/java/org/apache/gravitino/flink/connector/utils/TypeUtils.java:
##########
@@ -246,11 +252,25 @@ public static DataType toFlinkType(Type gravitinoType) {
case EXTERNAL:
Types.ExternalType externalType = (Types.ExternalType) gravitinoType;
String catalogString = externalType.catalogString();
- // Parse the external catalog type string back to Flink LogicalType.
- // This is used to restore types like MULTISET that Gravitino doesn't
natively support.
- LogicalType parsedType =
- LogicalTypeParser.parse(catalogString,
TypeUtils.class.getClassLoader());
- return TypeConversions.fromLogicalToDataType(parsedType);
+ // MULTISET is the only Flink type carried as an external type,
written here by
+ // toGravitinoType and by the Paimon catalog. Every other catalog
string is a type name of
+ // some other data source, and some of those parse as a Flink type by
accident, a
+ // PostgreSQL numeric parses as DECIMAL(10, 0). Only a parsed MULTISET
is therefore
+ // restored, and any other type is read as a string to keep the rest
of the table usable.
+ try {
+ LogicalType parsedType =
+ LogicalTypeParser.parse(catalogString,
TypeUtils.class.getClassLoader());
+ if (parsedType.getTypeRoot() == LogicalTypeRoot.MULTISET) {
+ return TypeConversions.fromLogicalToDataType(parsedType);
+ }
+ LOG.warn("External type {} is not a Flink type, reading it as a
string.", catalogString);
+ } catch (ValidationException e) {
+ LOG.warn(
+ "External type {} cannot be parsed as a Flink type, reading it
as a string.",
+ catalogString,
+ e);
+ }
+ return DataTypes.STRING();
Review Comment:
This currently attempts to parse *every* external type string, even though
only `MULTISET` is eligible for restoration. For frequent external types like
`numeric`/`money`/`json`, this adds unnecessary parsing overhead and (in the
exception path) potentially expensive stack-trace logging. A concrete
improvement is to short-circuit before parsing unless the string looks like a
multiset (e.g., case-insensitive check for `MULTISET<...>`), and avoid logging
stack traces at `warn` for expected parse failures (log message only, or move
stack trace to `debug`).
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/jdbc/postgresql/PostgreSQLDataTypeTransformer.java:
##########
@@ -82,6 +85,15 @@ public io.trino.spi.type.Type getTrinoType(Type type) {
}
// When precision is not set, the default precision is 3 (milliseconds
precision)
return TimeType.TIME_MILLIS;
+ } else if (Name.EXTERNAL == type.name()) {
+ // An external type carries a PostgreSQL type that Gravitino cannot
represent, such as an
+ // unconstrained numeric or money. Reading it as varchar is equivalent
to configuring
+ // unsupported_type_handling=CONVERT_TO_VARCHAR, whose Trino default is
IGNORE, and keeps an
+ // unmapped type from failing the type conversion of the whole table.
+ LOG.warn(
+ "Reading PostgreSQL type %s as varchar, Gravitino cannot represent
it",
+ ((Types.ExternalType) type).catalogString());
+ return io.trino.spi.type.VarcharType.createUnboundedVarcharType();
Review Comment:
This `LOG.warn(...)` is executed during type conversion and can be triggered
on every schema/table load for each external-typed column, which can create
noisy logs in production (especially if many tables/columns use external
types). Consider reducing the log level (e.g., `debug`) or
rate-limiting/logging once per distinct external type per JVM (e.g., track seen
`catalogString`s) to preserve the diagnostic value without flooding logs.
##########
docs/jdbc-postgresql-catalog.md:
##########
@@ -118,6 +118,16 @@ Refer to [Manage Catalogs and
Schemas](./manage-catalogs-and-schemas.md#schema-o
:::info
PostgreSQL doesn't support Gravitino `Fixed` `Struct` `Map` `IntervalDay`
`IntervalYear` `Union` type.
Meanwhile, the data types other than listed above are mapped to Gravitino
**[External Type](./tables-and-views.md#external-type)** that represents an
unresolvable data type.
+
+An unconstrained `Numeric` column, that is one declared without precision and
scale, accepts values of up to
+131072 digits before and 16383 digits after the decimal point, and its
precision and scale vary per row.
+Gravitino `Decimal` caps precision at 38 and is fixed per column, so such a
column is mapped to the External
+Type `numeric` instead. A `Numeric(p, s)` column is mapped to `Decimal(p, s)`
and a `Numeric(p)` column to
+`Decimal(p, 0)` as usual.
+
+PostgreSQL array elements always accept NULL and cannot be declared otherwise,
so an `Array` column is always
Review Comment:
The PostgreSQL type name is typically written as `numeric`/`NUMERIC` (not
`Numeric`), and similarly `array` is a PostgreSQL concept rather than a type
keyword. To reduce ambiguity for readers, consider consistently formatting
PostgreSQL type names as code literals matching PostgreSQL spelling/casing
(e.g., `numeric`, `numeric(p,s)`, and `integer[]`).
--
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]
