Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -321,14 +321,18 @@ public static TableSchema generateTableSchema(
for (SchemaChange change : changes) {
if (change instanceof SetOption) {
SetOption setOption = (SetOption) change;
if (hasSnapshots.get()) {
checkAlterTableOption(
oldOptions,
setOption.key(),
oldOptions.get(setOption.key()),
setOption.value());
String oldValue = oldOptions.get(setOption.key());
String newValue = setOption.value();
boolean unchanged =
Objects.equals(oldValue, newValue)
|| isUnchangedNormalizedKey(
setOption.key(), oldValue, newValue, oldTableSchema);
if (hasSnapshots.get() && !unchanged) {
checkAlterTableOption(oldOptions, setOption.key(), oldValue, newValue);
}
if (!unchanged) {
newOptions.put(setOption.key(), setOption.value());
}
newOptions.put(setOption.key(), setOption.value());
} else if (change instanceof RemoveOption) {
RemoveOption removeOption = (RemoveOption) change;
if (hasSnapshots.get()) {
Expand Down Expand Up @@ -1251,6 +1255,32 @@ public void rollbackTo(
}
}

/**
* Checks whether a key whose old value is null actually hasn't changed. This handles keys like
* 'primary-key' and 'partition' that are stripped from options during schema normalization and
* stored in dedicated schema fields instead.
*/
public static boolean isUnchangedNormalizedKey(
String key, @Nullable String oldValue, String newValue, TableSchema tableSchema) {
if (oldValue != null) {
return false;
}
if (CoreOptions.PRIMARY_KEY.key().equals(key)) {
return normalizeKeyList(newValue).equals(tableSchema.primaryKeys());
}
if (CoreOptions.PARTITION.key().equals(key)) {
return normalizeKeyList(newValue).equals(tableSchema.partitionKeys());
}
return false;
}

private static List<String> normalizeKeyList(String value) {
return Arrays.stream(value.split(","))
.map(String::trim)
.filter(s -> !s.isEmpty())
.collect(Collectors.toList());
}

public static void checkAlterTableOption(
Map<String, String> options, String key, @Nullable String oldValue, String newValue) {
if (CoreOptions.IMMUTABLE_OPTIONS.contains(key)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,9 @@ private void checkImmutability(Map<String, String> dynamicOptions) {
dynamicOptions.forEach(
(k, newValue) -> {
String oldValue = oldOptions.get(k);
if (!Objects.equals(oldValue, newValue)) {
if (!Objects.equals(oldValue, newValue)
&& !SchemaManager.isUnchangedNormalizedKey(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skipping the immutability check here is not enough because copyInternal still merges this dynamic option into the copied TableSchema options. For a primary-key table, table.copy(Collections.singletonMap("primary-key", "f0,f1")) will now succeed, but the returned table has both schema.primaryKeys() and schema.options().get("primary-key"). Converting that schema back through TableSchema.toSchema() then throws "Cannot define primary key on DDL and table options at the same time." Please also avoid carrying unchanged normalized keys into the copied schema options, similar to what generateTableSchema does above, so the copied table schema remains normalized.

k, oldValue, newValue, tableSchema)) {
SchemaManager.checkAlterTableOption(oldOptions, k, oldValue, newValue);
}
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -460,6 +460,30 @@ public void testAlterImmutableOptionsOnEmptyTable() throws Exception {
.hasMessage("Change 'merge-engine' is not supported yet.");
}

@Test
public void testCopyWithPrimaryKeyInOptions() throws Exception {
// Table created with primary-key in options — normalizePrimaryKeys strips it from the
// options map and stores it in the dedicated primaryKeys field. When the same value
// reappears in dynamicOptions (e.g. Spark 4.x merging Table.properties() into scan
// options), the immutability check should recognize it hasn't actually changed.
Map<String, String> tableOptions = new HashMap<>();
tableOptions.put("primary-key", "f0,f1");
Schema schema =
new Schema(
rowType.getFields(),
Collections.emptyList(),
Collections.emptyList(),
tableOptions,
"");
Path tableRoot = new Path(tempDir.toString(), "table");
SchemaManager manager = new SchemaManager(LocalFileIO.create(), tableRoot);
manager.createTable(schema);

FileStoreTable table = FileStoreTableFactory.create(LocalFileIO.create(), tableRoot);
assertThatCode(() -> table.copy(Collections.singletonMap("primary-key", "f0,f1")))
.doesNotThrowAnyException();
}

@Test
public void testDropPrimaryKeyOnEmptyTable() throws Exception {
Path tableRoot = new Path(tempDir.toString(), "table");
Expand Down
Loading