TheR1sing3un commented on code in PR #7738: URL: https://github.com/apache/paimon/pull/7738#discussion_r3162181831
########## paimon-python/pypaimon/common/identifier.py: ########## @@ -21,37 +21,82 @@ from pypaimon.common.json_util import json_field SYSTEM_TABLE_SPLITTER = '$' -SYSTEM_BRANCH_PREFIX = 'branch-' +SYSTEM_BRANCH_PREFIX = 'branch_' DEFAULT_MAIN_BRANCH = 'main' +UNKNOWN_DATABASE = 'unknown' -@dataclass +@dataclass(init=False) Review Comment: Thanks for catching that @JingsongLi — yes, you're right. Let me enumerate what changes shape so I can address each: 1. `Identifier(database, object, branch=...)` constructor kwarg removed. 2. `identifier.branch` attribute removed. 3. `Identifier.create(db, object)` second positional arg renamed from `object` to `table`; passing a pre-encoded object containing `$` now caches it as the table name rather than splitting it. 4. `SYSTEM_BRANCH_PREFIX` changed from `'branch-'` to `'branch_'` (this one is a bug-fix — Java has always used `branch_`, so anything Python wrote with `branch-` was already not round-trippable through the Java REST server). 5. The `branch` JSON field is no longer emitted (Java `@JsonIgnoreProperties(ignoreUnknown = true)` was already dropping it on the wire, so this never carried information end-to-end, but the Python wire shape does change). (4) and (5) I'd defend as correctness fixes — the pre-fix behaviour was inconsistent with Java in ways that silently corrupted cross-language scenarios, and I don't see a backward-compat path that preserves the buggy behaviour while letting branched system tables work against a Java REST server. (1) (2) (3) are the real API-shape break. I agree those need a soft-deprecation path. I'll push a follow-up commit on this PR that: * Re-accepts `Identifier(..., branch=...)` and routes it through the new encoding internally, emitting `DeprecationWarning`. * Re-exposes `identifier.branch` as a property that delegates to `get_branch_name()`, also with `DeprecationWarning`. * Re-accepts the old `Identifier.create(db, object)` two-arg form (detected by `$` in the second arg) and routes through the new constructor, with `DeprecationWarning`. * Adds tests that lock in both the warning *and* the equivalent-result behaviour, so we don't accidentally drop the shim before users have a chance to migrate. * Adds a migration note to the PR description. The plan would be to remove the shim in the next minor version. Does that sound reasonable? -- 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]
