Skip to content

[core] Fix copyWithLatestSchema option precedence#8550

Open
thswlsqls wants to merge 2 commits into
apache:masterfrom
thswlsqls:fix/copy-with-latest-schema-options
Open

[core] Fix copyWithLatestSchema option precedence#8550
thswlsqls wants to merge 2 commits into
apache:masterfrom
thswlsqls:fix/copy-with-latest-schema-options

Conversation

@thswlsqls

Copy link
Copy Markdown
Contributor

Purpose

fix #4957

  • copyWithLatestSchema() now lets the latest committed schema options override stale options from the current table copy, so altered option values survive a refresh.
  • Removes the obsolete TODO that described the previous broken precedence.

Tests

  • Added AppendOnlySimpleTableTest#testCopyWithLatestSchemaPicksUpAlteredOptionValues: alter snapshot.num-retained.max to 10, then assert copyWithLatestSchema() reflects it.
  • Failed before the fix with expected: 10 but was: 100; passes after.
  • mvn -pl paimon-core -am clean install ran the normal checks (checkstyle/spotless/rat); only PostgresqlCatalogTest was skipped for local Docker absence.

@JingsongLi

Copy link
Copy Markdown
Contributor

After reversing the merge order of schema options, 'copyWithLatestStyle()' will lose the runtime override of 'table. copy (dynamicOptions)'. Temporary regression test stable failure: Expected snapshot. num-retained. max=200, but actually recovered to persistent value of 100. This method is called multiple times in the Flink CDC schema refresh and retry link, which belongs to actual regression; It is necessary to distinguish between persistent options and dynamic options, and it is not possible to simply reverse the overall merging order.

The previous fix reversed the merge order so the latest committed schema
options win, which fixed altered persistent values not being picked up
(apache#4957) but dropped runtime overrides applied via copy(dynamicOptions):
those live only in the current table copy's schema, so letting the
committed schema win clobbered them (e.g. snapshot.num-retained.max fell
back from a runtime override to the committed value). copyWithLatestSchema
is called on the Flink CDC schema refresh/retry path, so this was a real
regression.

Rebase onto the latest committed options, then re-apply only the dynamic
overrides -- the entries whose value differs from the committed schema
this copy is based on. Both altered persistent values and runtime
overrides now survive a refresh. Added a test covering the dynamic-option
case alongside the existing altered-value test.

Generated-by: Claude Code
@thswlsqls

Copy link
Copy Markdown
Contributor Author

You're right — real regression. Dynamic options from copy(dynamicOptions) live only in tableSchema, not latestSchema, so swapping the merge order let persistent values clobber the runtime override. Fixed by starting from latestSchema and re-applying only the dynamic overrides on top; pushed with a test.

@JingsongLi

Copy link
Copy Markdown
Contributor

copyWithLatestSchema() cannot preserve the semantics of dynamically deleted options. Given a base schema of {x=1}, if x is dynamically deleted during a copy, and the schema is then refreshed to {x=2}, restoring x=2 will result in an error because the deletion of null is treated as a missing key, and cannot be inferred from the diff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] function copyWithLatestSchema() is not correct

2 participants