Skip to content

Commit 4145de2

Browse files
zhongyujiangzhongyujiang
committed
review comments.
1 parent 039f21a commit 4145de2

2 files changed

Lines changed: 168 additions & 17 deletions

File tree

‎paimon-core/src/main/java/org/apache/paimon/schema/SchemaManager.java‎

Lines changed: 162 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -268,14 +268,15 @@ public TableSchema commitChanges(List<SchemaChange> changes)
268268
tableRoot.toString(), true, branch)));
269269
LazyField<Identifier> lazyIdentifier =
270270
new LazyField<>(() -> identifierFromPath(tableRoot.toString(), true, branch));
271-
TableSchema newTableSchema =
271+
Optional<TableSchema> newTableSchemaOpt =
272272
generateTableSchema(oldTableSchema, changes, hasSnapshots, lazyIdentifier);
273-
if (oldTableSchema.sameContent(newTableSchema)) {
273+
if (!newTableSchemaOpt.isPresent()) {
274274
LOG.info(
275275
"No schema change detected for table {}. Skipping schema update.",
276276
lazyIdentifier.get());
277277
return oldTableSchema;
278278
}
279+
TableSchema newTableSchema = newTableSchemaOpt.get();
279280
try {
280281
boolean success = commit(newTableSchema);
281282
if (success) {
@@ -287,7 +288,7 @@ public TableSchema commitChanges(List<SchemaChange> changes)
287288
}
288289
}
289290

290-
public static TableSchema generateTableSchema(
291+
public static Optional<TableSchema> generateTableSchema(
291292
TableSchema oldTableSchema,
292293
List<SchemaChange> changes,
293294
LazyField<Boolean> hasSnapshots,
@@ -321,7 +322,98 @@ public static TableSchema generateTableSchema(
321322
List<DataField> newFields = new ArrayList<>(oldTableSchema.fields());
322323
AtomicInteger highestFieldId = new AtomicInteger(oldTableSchema.highestFieldId());
323324
String newComment = oldTableSchema.comment();
325+
326+
// Filter out ineffective changes
327+
List<SchemaChange> effectiveChanges = new ArrayList<>();
324328
for (SchemaChange change : changes) {
329+
if (change instanceof SetOption) {
330+
SetOption setOption = (SetOption) change;
331+
String oldValue = oldOptions.get(setOption.key());
332+
if (oldValue == null || !oldValue.equals(setOption.value())) {
333+
effectiveChanges.add(change);
334+
}
335+
} else if (change instanceof RemoveOption) {
336+
RemoveOption removeOption = (RemoveOption) change;
337+
if (oldOptions.containsKey(removeOption.key())) {
338+
effectiveChanges.add(change);
339+
}
340+
} else if (change instanceof UpdateComment) {
341+
UpdateComment updateComment = (UpdateComment) change;
342+
if (!Objects.equals(oldTableSchema.comment(), updateComment.comment())) {
343+
effectiveChanges.add(change);
344+
}
345+
} else if (change instanceof RenameColumn) {
346+
RenameColumn rename = (RenameColumn) change;
347+
DataField field = findField(oldTableSchema.fields(), rename.fieldNames());
348+
if (field != null && !field.name().equals(rename.newName())) {
349+
effectiveChanges.add(change);
350+
}
351+
} else if (change instanceof UpdateColumnType) {
352+
UpdateColumnType update = (UpdateColumnType) change;
353+
DataField field = findField(oldTableSchema.fields(), update.fieldNames());
354+
if (field != null) {
355+
DataType oldType = field.type();
356+
DataType newType = update.newDataType();
357+
if (update.keepNullability()) {
358+
newType = newType.copy(oldType.isNullable());
359+
}
360+
if (!oldType.equals(newType)) {
361+
effectiveChanges.add(change);
362+
}
363+
}
364+
} else if (change instanceof UpdateColumnNullability) {
365+
UpdateColumnNullability update = (UpdateColumnNullability) change;
366+
DataField field = findField(oldTableSchema.fields(), update.fieldNames());
367+
if (field != null) {
368+
DataType oldType = field.type();
369+
DataType sourceRootType =
370+
getRootType(
371+
oldType,
372+
update.fieldNames().length - 1,
373+
update.fieldNames().length);
374+
if (sourceRootType.isNullable() != update.newNullability()) {
375+
effectiveChanges.add(change);
376+
}
377+
}
378+
} else if (change instanceof UpdateColumnComment) {
379+
UpdateColumnComment update = (UpdateColumnComment) change;
380+
DataField field = findField(oldTableSchema.fields(), update.fieldNames());
381+
if (field != null
382+
&& !Objects.equals(field.description(), update.newDescription())) {
383+
effectiveChanges.add(change);
384+
}
385+
} else if (change instanceof UpdateColumnPosition) {
386+
UpdateColumnPosition update = (UpdateColumnPosition) change;
387+
SchemaChange.Move move = update.move();
388+
String fieldName = move.fieldName();
389+
DataField field = findFieldByName(newFields, fieldName);
390+
if (field != null) {
391+
int currentIndex = -1;
392+
for (int i = 0; i < newFields.size(); i++) {
393+
if (newFields.get(i).name().equals(fieldName)) {
394+
currentIndex = i;
395+
break;
396+
}
397+
}
398+
int newIndex = calculateNewPosition(newFields, move);
399+
if (currentIndex != newIndex) {
400+
effectiveChanges.add(change);
401+
}
402+
}
403+
} else if (change instanceof UpdateColumnDefaultValue) {
404+
UpdateColumnDefaultValue update = (UpdateColumnDefaultValue) change;
405+
DataField field = findField(oldTableSchema.fields(), update.fieldNames());
406+
if (field != null
407+
&& !Objects.equals(field.defaultValue(), update.newDefaultValue())) {
408+
effectiveChanges.add(change);
409+
}
410+
} else {
411+
// AddColumn and DropColumn always change the schema
412+
effectiveChanges.add(change);
413+
}
414+
}
415+
416+
for (SchemaChange change : effectiveChanges) {
325417
if (change instanceof SetOption) {
326418
SetOption setOption = (SetOption) change;
327419
if (hasSnapshots.get()) {
@@ -572,18 +664,75 @@ protected void updateLastColumn(
572664
oldTableSchema.partitionKeys(),
573665
applyNotNestedColumnRename(
574666
oldTableSchema.primaryKeys(),
575-
Iterables.filter(changes, RenameColumn.class)),
576-
applyRenameColumnsToOptions(newOptions, changes),
667+
Iterables.filter(effectiveChanges, RenameColumn.class)),
668+
applyRenameColumnsToOptions(newOptions, effectiveChanges),
577669
newComment);
578670

579-
return new TableSchema(
580-
oldTableSchema.id() + 1,
581-
newSchema.fields(),
582-
highestFieldId.get(),
583-
newSchema.partitionKeys(),
584-
newSchema.primaryKeys(),
585-
newSchema.options(),
586-
newSchema.comment());
671+
TableSchema newTableSchema =
672+
new TableSchema(
673+
oldTableSchema.id() + 1,
674+
newSchema.fields(),
675+
highestFieldId.get(),
676+
newSchema.partitionKeys(),
677+
newSchema.primaryKeys(),
678+
newSchema.options(),
679+
newSchema.comment());
680+
681+
if (oldTableSchema.sameContent(newTableSchema)) {
682+
return Optional.empty();
683+
}
684+
return Optional.of(newTableSchema);
685+
}
686+
687+
private static DataField findField(List<DataField> fields, String[] fieldNames) {
688+
if (fieldNames.length == 0) {
689+
return null;
690+
}
691+
String firstName = fieldNames[0];
692+
for (DataField field : fields) {
693+
if (field.name().equals(firstName)) {
694+
if (fieldNames.length == 1) {
695+
return field;
696+
}
697+
// Handle nested fields
698+
if (field.type() instanceof RowType) {
699+
return findField(
700+
((RowType) field.type()).getFields(),
701+
Arrays.copyOfRange(fieldNames, 1, fieldNames.length));
702+
}
703+
}
704+
}
705+
return null;
706+
}
707+
708+
private static DataField findFieldByName(List<DataField> fields, String fieldName) {
709+
for (DataField field : fields) {
710+
if (field.name().equals(fieldName)) {
711+
return field;
712+
}
713+
}
714+
return null;
715+
}
716+
717+
private static int calculateNewPosition(List<DataField> fields, SchemaChange.Move move) {
718+
if (move.type().equals(SchemaChange.Move.MoveType.FIRST)) {
719+
return 0;
720+
} else if (move.type().equals(SchemaChange.Move.MoveType.LAST)) {
721+
return fields.size() - 1;
722+
} else if (move.type().equals(SchemaChange.Move.MoveType.AFTER)) {
723+
for (int i = 0; i < fields.size(); i++) {
724+
if (fields.get(i).name().equals(move.referenceFieldName())) {
725+
return i + 1;
726+
}
727+
}
728+
} else if (move.type().equals(SchemaChange.Move.MoveType.BEFORE)) {
729+
for (int i = 0; i < fields.size(); i++) {
730+
if (fields.get(i).name().equals(move.referenceFieldName())) {
731+
return i;
732+
}
733+
}
734+
}
735+
return -1;
587736
}
588737

589738
// gets the rootType at the defined depth

‎paimon-core/src/test/java/org/apache/paimon/rest/RESTCatalogServer.java‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2427,10 +2427,12 @@ protected void alterTableImpl(Identifier identifier, List<SchemaChange> changes)
24272427
if (isFormatTable(schema.toSchema())) {
24282428
TableSchema newSchema =
24292429
SchemaManager.generateTableSchema(
2430-
schema,
2431-
changes,
2432-
new LazyField<>(() -> false),
2433-
new LazyField<>(() -> identifier));
2430+
schema,
2431+
changes,
2432+
new LazyField<>(() -> false),
2433+
new LazyField<>(() -> identifier))
2434+
.orElse(schema);
2435+
24342436
TableMetadata newTableMetadata =
24352437
createTableMetadata(
24362438
identifier,

0 commit comments

Comments
 (0)