github-actions[bot] commented on code in PR #66627:
URL: https://github.com/apache/doris/pull/66627#discussion_r3762381692
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergNestedColumnEvolution.java:
##########
@@ -87,10 +99,234 @@ 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");
- UpdateSchema updateSchema = table.updateSchema();
- updateSchema.deleteColumn(resolvedPath.getFullPath());
- updateSchema.commit();
+ dropColumnSafely(table, path, true);
+ }
+
+ static void dropTopLevelColumn(Table table, String columnName) {
+ dropColumnSafely(table, ConnectorColumnPath.of(columnName), false);
+ }
+
+ private static void dropColumnSafely(
+ Table table, ConnectorColumnPath path, boolean nested) {
+ TableOperations operations = ((HasTableOperations) table).operations();
+ TableMetadata loaded = operations.current();
+ if (loaded == null) {
+ throw new DorisConnectorException("Cannot drop column from an
unloaded Iceberg table: " + table.name());
+ }
+ ResolvedColumnPath loadedPath = resolveDropPath(loaded.schema(), path,
nested);
+ Set<Integer> loadedSubtreeIds = fieldSubtreeIds(loadedPath);
+
+ TableMetadata base = operations.refresh();
+ ResolvedColumnPath resolvedPath = validatePinnedDropIdentity(
+ table.name(), path, nested, loaded, loadedSubtreeIds, base);
+ validateNotUsedByRetainedPartitionSpec(base, resolvedPath);
+
+ if (isRestTableOperations(operations)) {
+ commitRestDropWithSpecFence(operations, table.name(), base,
resolvedPath);
+ return;
+ }
+
+ // Use Iceberg's standard commit so column-scoped writer properties
and name mappings are transformed.
+ // Direct catalogs atomically reject any metadata change after the
refresh through their metadata CAS.
+ new BaseTable(operations, table.name()).updateSchema()
+ .deleteColumn(resolvedPath.getFullPath()).commit();
+ }
+
+ private static void commitRestDropWithSpecFence(
+ TableOperations operations, String tableName, TableMetadata base,
ResolvedColumnPath resolvedPath) {
+ if (base.formatVersion() < 2) {
+ throw new DorisConnectorException(
+ "Cannot safely drop a column from a format-v1 Iceberg REST
table: " + tableName);
+ }
+
+ Set<Integer> droppedFieldIds = fieldSubtreeIds(resolvedPath);
+ String fenceSource = findRestFenceSource(base, droppedFieldIds);
+ if (fenceSource == null) {
+ throw new DorisConnectorException(
+ "Cannot safely fence a column drop in Iceberg REST table:
" + tableName);
+ }
+ String fenceName = newRestFenceName(base);
+
+ // Validation proved that no retained spec references the drop target,
so any concurrent new reference
+ // must allocate a partition field ID. Adding a distinct void field
makes REST assert that ID counter.
+ CapturingTableOperations schemaCapture = new
CapturingTableOperations(operations, base);
+ new BaseTable(schemaCapture, tableName).updateSchema()
+ .deleteColumn(resolvedPath.getFullPath()).commit();
+
+ CapturingTableOperations fenceCapture = new
CapturingTableOperations(operations, base);
+ new BaseTable(fenceCapture, tableName).updateSpec()
+ .addField(fenceName, Expressions.transform(fenceSource,
Transforms.alwaysNull()))
Review Comment:
[P2] Avoid pinning the fence source against later DROP
On an unpartitioned REST v2 table with columns `(a, b, c, d)`, dropping `d`
selects `a` here and persists a non-default `void(a)` spec. A later `DROP a` is
then rejected by `validateNotUsedByRetainedPartitionSpec` solely because of
this synthetic fence, even though `a` was not user-partitioned. Dropping other
columns first also keeps selecting `a`; the unique names cause each fence to
retain another spec/partition-field ID. Thus one successful DROP changes which
unrelated columns can subsequently be dropped and repeated DROPs grow metadata.
Please use a fence that does not durably reference a user field (or fail this
path closed), and add a sequential REST DROP regression.
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergNestedColumnEvolution.java:
##########
@@ -87,10 +99,234 @@ 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");
- UpdateSchema updateSchema = table.updateSchema();
- updateSchema.deleteColumn(resolvedPath.getFullPath());
- updateSchema.commit();
+ dropColumnSafely(table, path, true);
+ }
+
+ static void dropTopLevelColumn(Table table, String columnName) {
+ dropColumnSafely(table, ConnectorColumnPath.of(columnName), false);
+ }
+
+ private static void dropColumnSafely(
+ Table table, ConnectorColumnPath path, boolean nested) {
+ TableOperations operations = ((HasTableOperations) table).operations();
+ TableMetadata loaded = operations.current();
+ if (loaded == null) {
+ throw new DorisConnectorException("Cannot drop column from an
unloaded Iceberg table: " + table.name());
+ }
+ ResolvedColumnPath loadedPath = resolveDropPath(loaded.schema(), path,
nested);
+ Set<Integer> loadedSubtreeIds = fieldSubtreeIds(loadedPath);
+
+ TableMetadata base = operations.refresh();
+ ResolvedColumnPath resolvedPath = validatePinnedDropIdentity(
+ table.name(), path, nested, loaded, loadedSubtreeIds, base);
+ validateNotUsedByRetainedPartitionSpec(base, resolvedPath);
+
+ if (isRestTableOperations(operations)) {
+ commitRestDropWithSpecFence(operations, table.name(), base,
resolvedPath);
+ return;
+ }
+
+ // Use Iceberg's standard commit so column-scoped writer properties
and name mappings are transformed.
+ // Direct catalogs atomically reject any metadata change after the
refresh through their metadata CAS.
+ new BaseTable(operations, table.name()).updateSchema()
+ .deleteColumn(resolvedPath.getFullPath()).commit();
+ }
+
+ private static void commitRestDropWithSpecFence(
+ TableOperations operations, String tableName, TableMetadata base,
ResolvedColumnPath resolvedPath) {
+ if (base.formatVersion() < 2) {
+ throw new DorisConnectorException(
+ "Cannot safely drop a column from a format-v1 Iceberg REST
table: " + tableName);
+ }
+
+ Set<Integer> droppedFieldIds = fieldSubtreeIds(resolvedPath);
+ String fenceSource = findRestFenceSource(base, droppedFieldIds);
+ if (fenceSource == null) {
+ throw new DorisConnectorException(
+ "Cannot safely fence a column drop in Iceberg REST table:
" + tableName);
+ }
+ String fenceName = newRestFenceName(base);
+
+ // Validation proved that no retained spec references the drop target,
so any concurrent new reference
+ // must allocate a partition field ID. Adding a distinct void field
makes REST assert that ID counter.
+ CapturingTableOperations schemaCapture = new
CapturingTableOperations(operations, base);
+ new BaseTable(schemaCapture, tableName).updateSchema()
+ .deleteColumn(resolvedPath.getFullPath()).commit();
+
+ CapturingTableOperations fenceCapture = new
CapturingTableOperations(operations, base);
+ new BaseTable(fenceCapture, tableName).updateSpec()
Review Comment:
[P2] Preserve forward-compatible specs during REST DROP
This `updateSpec()` call constructs Iceberg 1.10.1's
`BaseUpdatePartitionSpec`, whose constructor throws whenever the current spec
contains an `UnknownTransform`. Consequently, a REST v2 table written by a
newer engine cannot drop an unrelated column through Doris if its current spec
has a transform this client does not recognize; the schema-only/direct path
preserves that spec without invoking this mutation validator. Please generate
the REST requirement/fence without routing through the current-spec mutation
validator, and add a regression with an unrelated unknown transform.
--
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]
