github-actions[bot] commented on code in PR #69633:
URL: https://github.com/apache/doris/pull/69633#discussion_r4378814350
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergCatalogOps.java:
##########
@@ -494,6 +494,7 @@ public void renameTable(String dbName, String oldName,
String newName) {
@Override
public void setTableProperties(String dbName, String tableName,
Map<String, String> properties) {
withTable(dbName, tableName, table -> {
+
IcebergSchemaUtils.validateRowLineageFormatVersionUpgrade(table, properties);
Review Comment:
[P1] Validate the metadata base actually committed by the upgrade
This precheck can validate a different schema from the one upgraded. For
example, A validates a clean V2 schema; B adds `_row_id` and commits while the
table is still V2; then A commits its property update. Iceberg's
[PropertiesUpdate.apply refreshes the base on every commit
attempt](https://github.com/apache/iceberg/blob/apache-iceberg-1.12.0/core/src/main/java/org/apache/iceberg/PropertiesUpdate.java#L79-L115),
so A upgrades B's schema without rerunning this check. The new read-side guard
then rejects the successfully upgraded table. This can happen on the first
attempt or an automatic retry. Couple validation to the actual optimistic
metadata update so every refreshed commit base is checked, and add
deterministic concurrency coverage.
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergSchemaUtils.java:
##########
@@ -102,12 +104,59 @@ public final class IcebergSchemaUtils {
static final String ICEBERG_LAST_UPDATED_SEQUENCE_NUMBER_COL =
"_last_updated_sequence_number";
static final int ICEBERG_ROW_ID_FIELD_ID = 2147483540;
static final int ICEBERG_LAST_UPDATED_SEQUENCE_NUMBER_FIELD_ID =
2147483539;
+ static final int ICEBERG_ROW_LINEAGE_MIN_VERSION = 3;
private static final Base64.Encoder BASE64_ENCODER = Base64.getEncoder();
private IcebergSchemaUtils() {
}
+ /**
+ * Validates the effective row-lineage schema before a table property
update can make it observable.
+ *
+ * <p>Iceberg keeps row-lineage columns outside the physical table schema,
so an {@code UpdateProperties}
+ * format-version upgrade does not itself validate their names against
user columns. Doris exposes both sets
+ * through one connector schema, which must remain a valid Iceberg {@link
Schema}. Delegate that contract to
+ * Iceberg instead of maintaining a second reserved-name rule here.</p>
+ */
+ static void validateRowLineageFormatVersionUpgrade(Table table,
Map<String, String> properties) {
+ int currentFormatVersion = IcebergWriterHelper.getFormatVersion(table);
+ if (currentFormatVersion >= ICEBERG_ROW_LINEAGE_MIN_VERSION) {
+ return;
+ }
+
+ String requestedFormatVersion =
properties.get(TableProperties.FORMAT_VERSION);
+ if (requestedFormatVersion == null) {
+ return;
+ }
+
+ int targetFormatVersion;
+ try {
+ targetFormatVersion = Integer.parseInt(requestedFormatVersion);
+ } catch (NumberFormatException ignored) {
+ // Leave invalid values to Iceberg's UpdateProperties validation.
+ return;
+ }
+
+ if (targetFormatVersion >= ICEBERG_ROW_LINEAGE_MIN_VERSION) {
+ validateRowLineageSchema(table.schema());
+ }
+ }
+
+ /**
+ * Verifies that the physical schema can be exposed together with
Iceberg's row-lineage metadata columns.
+ * {@link MetadataColumns#schemaWithRowLineage(Schema)} is the authority
for the combined schema, including
+ * its requirement that field names are unique and field ids match.
+ */
+ static void validateRowLineageSchema(Schema schema) {
+ try {
+ MetadataColumns.schemaWithRowLineage(schema);
Review Comment:
[P1] Check lineage names with Doris's case-insensitive semantics
A readable V2 table with a user column `_ROW_ID` passes this check:
Iceberg's [name index preserves
case](https://github.com/apache/iceberg/blob/apache-iceberg-1.12.0/api/src/main/java/org/apache/iceberg/types/IndexByName.java#L202-L218),
so the combined schema accepts `_ROW_ID` and `_row_id`. The upgrade commits,
and `buildTableSchema` then publishes both columns; Doris's
`SchemaCacheValue.validateSchema` lowercases names and rejects the next schema
load as a duplicate. The same applies to mixed-case
`_last_updated_sequence_number`. Apply the case-insensitive lineage-name rule
already used by CREATE and ADD/RENAME at both new validation boundaries, and
cover mixed-case variants of both names.
--
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]
