yuqi1129 commented on code in PR #14588:
URL: https://github.com/apache/gravitino/pull/14588#discussion_r4489552132
##########
trino-connector/src/common/main/java/org/apache/gravitino/trino/connector/catalog/glue/GlueMetadataAdapter.java:
##########
@@ -158,8 +206,27 @@ public GravitinoTable createTable(ConnectorTableMetadata
tableMetadata) {
tableMetadata.getProperties().containsKey(HivePropertyMeta.HIVE_PARTITION_KEY)
? (List<String>)
tableMetadata.getProperties().get(HivePropertyMeta.HIVE_PARTITION_KEY)
: Collections.emptyList();
+ @SuppressWarnings("unchecked")
+ List<String> sortFields =
+
tableMetadata.getProperties().containsKey(HivePropertyMeta.HIVE_SORT_ORDER_KEY)
+ ? (List<String>)
tableMetadata.getProperties().get(HivePropertyMeta.HIVE_SORT_ORDER_KEY)
+ : Collections.emptyList();
- GravitinoTable table = super.createTable(tableMetadata);
+ Object type = tableMetadata.getProperties().get(LAKEHOUSE_TABLE_TYPE);
+ String tableType = type == null ? defaultTableType : type.toString();
+ boolean icebergTable =
GlueConstants.TABLE_FORMAT_ICEBERG.equalsIgnoreCase(tableType);
+
+ GravitinoTable table =
super.createTable(withHiveSortColumns(tableMetadata, icebergTable));
+
+ if (icebergTable) {
+ if (!sortFields.isEmpty()) {
+
table.setSortOrders(ExpressionUtil.sortOrderFiledToExpression(sortFields));
Review Comment:
[Important] The shared parser/formatter does not support the same
`sorted_by` grammar as Trino Iceberg.
Two reproducible cases:
- `WITH (type = 'ICEBERG', sorted_by = ARRAY['id NULLS LAST'])` fails with
`Unparsed expression: id nulls last`. Trino accepts an omitted direction and
defaults it to ASC, but `ExpressionUtil` requires an explicit ASC/DESC before
NULLS.
- For an existing Iceberg table sorted by a valid column named `order-id`,
`getTableMetadata()` emits `order-id DESC` without identifier quotes.
Recreating the table from that metadata fails with `Unparsed expression:
order-id desc`.
I reproduced both failures through the adapter on Trino 469 and 483. The
[native Trino
parser](https://github.com/trinodb/trino/blob/469/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/SortFieldUtils.java)
independently supports optional direction, optional null ordering, and quoted
identifiers.
Could we align both parsing and formatting with that grammar and add CREATE
plus metadata-read/recreate regressions? This should cover omitted ASC and
escaped/quoted column names, rather than special-casing either example.
##########
spark-connector/spark-common/src/main/java/org/apache/gravitino/spark/connector/glue/GravitinoGlueCatalog.java:
##########
@@ -233,6 +241,36 @@ protected SparkTypeConverter getSparkTypeConverter() {
return new SparkHiveTypeConverter();
}
+ /**
+ * {@inheritDoc} A table created without {@code table-format} gets the
catalog's default table
+ * format.
+ */
+ @Override
+ protected SparkTypeConverter getSparkTypeConverter(Map<String, String>
gravitinoTableProperties) {
+ String tableFormat =
+ gravitinoTableProperties.getOrDefault(
+ GlueConstants.TABLE_FORMAT,
+ catalogProperties == null
+ ? null
+ : catalogProperties.get(GlueConstants.DEFAULT_TABLE_FORMAT));
+ return GlueConstants.TABLE_FORMAT_ICEBERG.equalsIgnoreCase(tableFormat)
+ ? ICEBERG_TYPE_CONVERTER
+ : super.getSparkTypeConverter(gravitinoTableProperties);
+ }
+
+ /** {@inheritDoc} Changes of an Iceberg table follow Iceberg type semantics.
*/
+ @Override
+ protected SparkTableChangeConverter getSparkTableChangeConverter(Identifier
ident) {
+ try {
+ if (isIcebergTable(loadGravitinoTable(ident))) {
Review Comment:
[Nit] This adds a synchronous REST table read to every Glue ALTER, including
comment/property-only changes whose conversion does not depend on the table
format. `BaseCatalog.alterTable()` calls this hook unconditionally, and the
existing post-write `loadSparkTable()` still reads the Gravitino table again.
`RelationalCatalog.loadTable()` issues a REST GET rather than serving a local
table cache.
The analogous Trino `GlueMetadataAdapter.checkColumnType()` also invokes its
table supplier for ordinary types such as bigint, even though their validation
cannot differ between Hive and Iceberg.
Could we load the format only for changes/types that need it, or reuse
already available metadata? If this extra request and pre-write failure
dependency are intentional, please document the tradeoff and add request-count
coverage. This is based on tracing the REST call paths; I have not benchmarked
AWS/network latency.
--
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]
