szehon-ho commented on code in PR #16028:
URL: https://github.com/apache/iceberg/pull/16028#discussion_r3376887185
##########
spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/Spark3Util.java:
##########
@@ -109,6 +112,20 @@ public class Spark3Util {
private Spark3Util() {}
+ public static TableInfo tableInfo(
Review Comment:
nit: this `StructType` overload is only referenced from classes within
`org.apache.iceberg.spark` (the catalogs and `RollbackStagedTable`), so it can
be package-private. Only the `Column[]` overload below needs to stay `public`,
since it's the one called from
`org.apache.iceberg.spark.actions.BaseTableCreationSparkAction`. Keeps the
public surface of `Spark3Util` minimal.
##########
spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/source/BaseSparkTable.java:
##########
@@ -96,6 +103,14 @@ public StructType schema() {
return lazySparkSchema;
}
+ @Override
+ public Column[] columns() {
Review Comment:
nit (optional): `columns()` recomputes `SparkSchemaUtil.convert(schema)`,
which `schema()` already caches in `lazySparkSchema`. Since we can't reuse the
deprecated `schema()` here, consider extracting a small private non-deprecated
helper that both call, to avoid converting the schema twice:
```java
private StructType sparkSchema() {
if (lazySparkSchema == null) {
this.lazySparkSchema = SparkSchemaUtil.convert(schema);
}
return lazySparkSchema;
}
```
Then `columns()` becomes
`CatalogV2Util.structTypeToV2Columns(sparkSchema())` and the deprecated
`schema()` just returns `sparkSchema()`. Same applies to `SparkChangelogTable`.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
