github-actions[bot] commented on code in PR #66627:
URL: https://github.com/apache/doris/pull/66627#discussion_r3751512356
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergNestedColumnEvolution.java:
##########
@@ -88,11 +88,38 @@ public static void addColumn(Table table,
ConnectorColumnPath path, IcebergColum
/** Drops the nested field at {@code path}; its parent must resolve to a
struct that contains the leaf. */
public static void dropColumn(Table table, ConnectorColumnPath path) {
ResolvedColumnPath resolvedPath =
validateNestedStructFieldPath(table.schema(), path, "drop");
+ validateNotUsedByOldPartitionSpec(table, resolvedPath);
UpdateSchema updateSchema = table.updateSchema();
updateSchema.deleteColumn(resolvedPath.getFullPath());
updateSchema.commit();
}
+ static void dropTopLevelColumn(Table table, String columnName) {
+ ResolvedColumnPath resolvedPath = resolveColumnPath(
+ table.schema(), ConnectorColumnPath.of(columnName), "drop");
+ validateNotUsedByOldPartitionSpec(table, resolvedPath);
+ UpdateSchema updateSchema = table.updateSchema();
+ updateSchema.deleteColumn(resolvedPath.getFullPath());
+ updateSchema.commit();
+ }
+
+ private static void validateNotUsedByOldPartitionSpec(Table table,
ResolvedColumnPath columnPath) {
+ int currentSpecId = table.spec().specId();
+ Set<Integer> droppedFieldIds = TypeUtil.indexById(
+ Types.StructType.of(columnPath.getField())).keySet();
+ // Historical specs resolve partition types by source field ID against
the current schema, so deleting
+ // a referenced source field or any ancestor leaves those specs
unreadable even after the field is
+ // removed from the current spec. Index the full subtree because
deleting a struct also deletes its fields.
+ boolean usedByOldSpec = table.specs().values().stream()
+ .filter(spec -> spec.specId() != currentSpecId)
+ .flatMap(spec -> spec.fields().stream())
Review Comment:
[P1] Fence this validation against REST spec rebases
The scan and schema update are separate commits from REST's point of view.
After this scan, another client can add `val` in a non-default spec
(`addNonDefaultSpec`) without changing the default spec, schema, or snapshot
refs. Iceberg 1.10.1's REST requirements for this drop's
`AddSchema`/`SetCurrentSchema` updates do not assert the last assigned
partition ID or retained spec set, so `CatalogHandlers` can rebase the drop
onto that newer metadata. The drop then commits with a retained spec whose
source is gone, and Doris's `$position_deletes` construction fails while
transforming all retained specs. Make the schema commit depend on the
partition-spec generation / last assigned partition ID (and retry the whole
refresh/validate/commit operation on conflict), and add a deterministic REST
concurrency test.
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergNestedColumnEvolution.java:
##########
@@ -88,11 +88,38 @@ public static void addColumn(Table table,
ConnectorColumnPath path, IcebergColum
/** Drops the nested field at {@code path}; its parent must resolve to a
struct that contains the leaf. */
public static void dropColumn(Table table, ConnectorColumnPath path) {
ResolvedColumnPath resolvedPath =
validateNestedStructFieldPath(table.schema(), path, "drop");
+ validateNotUsedByOldPartitionSpec(table, resolvedPath);
UpdateSchema updateSchema = table.updateSchema();
updateSchema.deleteColumn(resolvedPath.getFullPath());
updateSchema.commit();
}
+ static void dropTopLevelColumn(Table table, String columnName) {
+ ResolvedColumnPath resolvedPath = resolveColumnPath(
+ table.schema(), ConnectorColumnPath.of(columnName), "drop");
+ validateNotUsedByOldPartitionSpec(table, resolvedPath);
+ UpdateSchema updateSchema = table.updateSchema();
+ updateSchema.deleteColumn(resolvedPath.getFullPath());
+ updateSchema.commit();
+ }
+
+ private static void validateNotUsedByOldPartitionSpec(Table table,
ResolvedColumnPath columnPath) {
+ int currentSpecId = table.spec().specId();
+ Set<Integer> droppedFieldIds = TypeUtil.indexById(
+ Types.StructType.of(columnPath.getField())).keySet();
+ // Historical specs resolve partition types by source field ID against
the current schema, so deleting
+ // a referenced source field or any ancestor leaves those specs
unreadable even after the field is
+ // removed from the current spec. Index the full subtree because
deleting a struct also deletes its fields.
+ boolean usedByOldSpec = table.specs().values().stream()
+ .filter(spec -> spec.specId() != currentSpecId)
+ .flatMap(spec -> spec.fields().stream())
+ .anyMatch(field -> droppedFieldIds.contains(field.sourceId()));
Review Comment:
[P1] Check current format-v1 void fields too
Iceberg v1 removes a partition field by carrying it into the new default
spec as `alwaysNull`. This filter skips that spec, and Iceberg's default-spec
compatibility check intentionally allows a void field whose source is missing.
Once older non-void specs are cleaned, dropping the source here can therefore
commit. Doris later exposes `$position_deletes`, whose Iceberg 1.10.1
construction transforms every retained spec; the partition schema omits the
missing source, so rebuilding this current void field fails. Include matching
current void fields in this fence and add a format-v1 regression that cleans
the older spec and verifies the metadata-table outcome.
--
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]
