mattcasters commented on code in PR #8795:
URL: https://github.com/apache/hop/pull/8795#discussion_r4229727129
##########
plugins/engines/spark/src/main/java/org/apache/hop/spark/table/SparkLakeTableSupport.java:
##########
@@ -107,15 +108,63 @@ public static String toTableLocationUri(String path) {
}
/**
- * Spark SQL multi-part identifier for an Iceberg path-based table under the
hop path catalog:
- * {@code hop_iceberg.`file:///path/to/table`}.
+ * An Iceberg table at a path, seen through the Hadoop catalog whose
warehouse is the table's
+ * parent folder. A Hadoop catalog keeps a table of the default namespace in
{@code
+ * <warehouse>/<table>}, so the table is read and written exactly at its
path.
+ *
+ * <p>A quoted path in a single shared catalog, like {@code
hop_iceberg.`file:///data/orders`},
+ * doesn't work: Iceberg only treats an identifier as a location when Spark
creates it from {@code
+ * DataFrameReader.load(path)}. Otherwise the Hadoop catalog takes the whole
URI as a table name
+ * and stores the table under its own warehouse.
*/
- @SuppressWarnings("javabugs:S2259") // toTableLocationUri() never returns
null for a resolved path
+ public record IcebergPathTable(String catalogName, String warehouse, String
tableName) {
+
+ /** Spark SQL identifier of the table, e.g. {@code
hop_iceberg_1a2b3c4d5e6f.`orders`}. */
+ public String sqlIdentifier() {
+ return catalogName + "." + procedureTableRef();
+ }
+
+ /** The table as named in an Iceberg procedure call of {@link
#catalogName()}. */
+ public String procedureTableRef() {
+ return "`" + tableName.replace("`", "``") + "`";
+ }
+ }
+
+ /**
+ * The path catalog and table name for an Iceberg table at {@code path}.
Tables in the same folder
+ * share a catalog.
+ */
+ public static IcebergPathTable icebergPathTable(String path) {
+ String uri = StringUtils.removeEnd(toTableLocationUri(path), "/");
+ int slash = uri == null ? -1 : uri.lastIndexOf('/');
+ String parent = slash < 0 ? "" : uri.substring(0, slash);
+ String name = slash < 0 ? "" : uri.substring(slash + 1);
+ if (name.isEmpty() || parent.isEmpty() || parent.endsWith(":") ||
parent.endsWith("/")) {
Review Comment:
**[bug]** The “needs a parent folder” check treats every parent that ends
with `:` or `/` as invalid. That rejects paths which do have a parent directory:
- A table in a Windows drive root (`C:\orders`, `C:/orders`, or
`file:///C:/orders`) becomes parent `file:///C:` and throws
`IllegalArgumentException`. `C:\data\orders` still works, because that parent
is `file:///C:/data`.
- A URI with an empty segment before the table name
(`s3a://bucket/lake//orders`, `file:///data//orders`) becomes a parent that
ends with `/` and throws, even though it is the same location as the
single-slash path.
`file:///t` and `s3a://bucket` are correctly rejected, and those cases are
tested. `endsWith(":")` / `endsWith("/")` are wider than that rule, and the
failure is an unchecked `IllegalArgumentException` rather than a `HopException`
naming the transform.
**Suggestion:** Normalize the URI before the split (collapse empty segments,
resolve `.` and `..`). Treat a Windows drive root (`file:///C:`) as a valid
warehouse. Only reject a scheme-only parent (`file:`, `file://`, `s3a:`,
`s3a:/`). Add tests for `file:///C:/orders` and `s3a://bucket/lake//orders`.
##########
plugins/engines/spark/src/main/java/org/apache/hop/spark/table/SparkLakeTableSupport.java:
##########
@@ -107,15 +108,63 @@ public static String toTableLocationUri(String path) {
}
/**
- * Spark SQL multi-part identifier for an Iceberg path-based table under the
hop path catalog:
- * {@code hop_iceberg.`file:///path/to/table`}.
+ * An Iceberg table at a path, seen through the Hadoop catalog whose
warehouse is the table's
+ * parent folder. A Hadoop catalog keeps a table of the default namespace in
{@code
+ * <warehouse>/<table>}, so the table is read and written exactly at its
path.
+ *
+ * <p>A quoted path in a single shared catalog, like {@code
hop_iceberg.`file:///data/orders`},
+ * doesn't work: Iceberg only treats an identifier as a location when Spark
creates it from {@code
+ * DataFrameReader.load(path)}. Otherwise the Hadoop catalog takes the whole
URI as a table name
+ * and stores the table under its own warehouse.
*/
- @SuppressWarnings("javabugs:S2259") // toTableLocationUri() never returns
null for a resolved path
+ public record IcebergPathTable(String catalogName, String warehouse, String
tableName) {
+
+ /** Spark SQL identifier of the table, e.g. {@code
hop_iceberg_1a2b3c4d5e6f.`orders`}. */
+ public String sqlIdentifier() {
+ return catalogName + "." + procedureTableRef();
+ }
+
+ /** The table as named in an Iceberg procedure call of {@link
#catalogName()}. */
+ public String procedureTableRef() {
+ return "`" + tableName.replace("`", "``") + "`";
+ }
+ }
+
+ /**
+ * The path catalog and table name for an Iceberg table at {@code path}.
Tables in the same folder
+ * share a catalog.
+ */
+ public static IcebergPathTable icebergPathTable(String path) {
+ String uri = StringUtils.removeEnd(toTableLocationUri(path), "/");
Review Comment:
**[suggestion]** The catalog name is a hash of the parent string after
removing only one trailing slash, and scheme URIs are not canonicalized
(`toTableLocationUri` returns them unchanged). `File.toURI()` yields
`file:/data/lake` while `Path.toUri()` yields `file:///data/lake`. Those become
two `hop_iceberg_<hash>` catalogs whose warehouses are the same directory.
`SparkCatalog` wraps the Hadoop catalog in `CachingCatalog` by default, so a
write through one spelling does not refresh the other catalog’s cached snapshot.
**Suggestion:** Canonicalize the location before hashing and before setting
`spark.sql.catalog.<name>.warehouse`: lowercase the scheme, rewrite `file` URIs
to `file:///…`, collapse empty segments, and resolve `.` / `..`. Then one
folder is one catalog.
--
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]
