Copilot commented on code in PR #7553:
URL: https://github.com/apache/paimon/pull/7553#discussion_r3013349674
##########
paimon-core/src/main/java/org/apache/paimon/table/FallbackReadFileStoreTable.java:
##########
@@ -66,73 +66,82 @@
import java.util.Optional;
import java.util.OptionalLong;
import java.util.Set;
+import java.util.function.Function;
import java.util.stream.Collectors;
/**
- * A {@link FileStoreTable} which mainly read from the current branch.
However, if the current
- * branch does not have a partition, it will read that partition from the
fallback branch.
+ * A {@link FileStoreTable} which reads from two branches with partition-level
merging. When {@code
+ * wrappedFirst} is true, the wrapped table has read priority (fallback mode).
When {@code
+ * wrappedFirst} is false, the other table has read priority (primary mode).
Write operations are
+ * always delegated to the wrapped table (current branch).
*/
public class FallbackReadFileStoreTable extends DelegatedFileStoreTable {
private static final Logger LOG =
LoggerFactory.getLogger(FallbackReadFileStoreTable.class);
- private final FileStoreTable fallback;
+ private final FileStoreTable other;
+ private final boolean wrappedFirst;
- public FallbackReadFileStoreTable(FileStoreTable wrapped, FileStoreTable
fallback) {
+ public FallbackReadFileStoreTable(
+ FileStoreTable wrapped, FileStoreTable other, boolean
wrappedFirst) {
super(wrapped);
- this.fallback = fallback;
+ this.other = other;
+ this.wrappedFirst = wrappedFirst;
Preconditions.checkArgument(!(wrapped instanceof
FallbackReadFileStoreTable));
- if (fallback instanceof FallbackReadFileStoreTable) {
+ if (other instanceof FallbackReadFileStoreTable) {
// ChainGroupReadTable need to be wrapped again
- if (!(fallback instanceof ChainGroupReadTable)) {
+ if (!(other instanceof ChainGroupReadTable)) {
throw new IllegalArgumentException(
"This is a bug, perhaps there is a recursive call.");
}
}
}
- public FileStoreTable fallback() {
- return fallback;
+ public FileStoreTable other() {
+ return other;
}
Review Comment:
`fallback()` was renamed/removed in favor of `other()`. Because
`FallbackReadFileStoreTable` is public, this is a binary/source incompatible
change for any external users relying on `fallback()`. Consider keeping a
deprecated `fallback()` accessor delegating to `other()` (and/or a clearer
accessor like `supplemental()`), at least for one release cycle.
```suggestion
/**
* @deprecated Use {@link #other()} instead.
*/
@Deprecated
public FileStoreTable fallback() {
return other();
}
```
##########
paimon-core/src/main/java/org/apache/paimon/table/AbstractFileStoreTable.java:
##########
@@ -709,10 +709,19 @@ public void deleteBranch(String branchName) {
&& branchName.equals(fallbackBranch)) {
throw new IllegalArgumentException(
String.format(
- "can not delete the fallback branch. "
- + "branchName to be deleted is %s. you
have set 'scan.fallback-branch' = '%s'. "
- + "you should reset 'scan.fallback-branch'
before deleting this branch.",
- branchName, fallbackBranch));
+ "Cannot delete branch '%s' because it is
configured as"
+ + " 'scan.fallback-branch'. Unset
'scan.fallback-branch' first.",
+ branchName));
+ }
+
+ String primaryBranch =
coreOptions().toConfiguration().get(CoreOptions.SCAN_PRIMARY_BRANCH);
+ if (!StringUtils.isNullOrWhitespaceOnly(primaryBranch)
+ && branchName.equals(primaryBranch)) {
+ throw new IllegalArgumentException(
+ String.format(
Review Comment:
The new guard preventing deletion of the `scan.primary-branch` is not
covered by tests (there are existing tests around branch deletion /
fallback-branch). Please add a test asserting `deleteBranch(primaryBranch)`
fails with the expected message when `scan.primary-branch` is set, similar to
the existing fallback-branch coverage.
##########
paimon-api/src/main/java/org/apache/paimon/schema/Schema.java:
##########
@@ -186,11 +187,11 @@ private static List<DataField> normalizeFields(
private List<String> normalizePrimaryKeys(List<String> primaryKeys) {
if (options.containsKey(CoreOptions.PRIMARY_KEY.key())) {
- if (!primaryKeys.isEmpty()) {
+ String pk = options.get(CoreOptions.PRIMARY_KEY.key());
+ if (!primaryKeys.isEmpty() && !StringUtils.isEmpty(pk)) {
Review Comment:
If table options contain `primary-key` with an empty value (e.g. `''`), the
new condition `!primaryKeys.isEmpty() && !StringUtils.isEmpty(pk)` will skip
the conflict check and then overwrite the DDL-defined primary key with an empty
list (because `pk.split(",")` yields only empty tokens). That silently drops
the primary key definition. It would be safer to treat null/blank `pk` as
“option not set” (ignore it and keep the DDL `primaryKeys`) instead of
normalizing it into an empty PK list.
```suggestion
// Treat null/blank primary-key option as "not set" and keep
DDL-defined primary keys.
if (StringUtils.isEmpty(pk)) {
return primaryKeys;
}
if (!primaryKeys.isEmpty()) {
```
##########
paimon-core/src/main/java/org/apache/paimon/table/FallbackReadFileStoreTable.java:
##########
@@ -153,97 +162,107 @@ protected FileStoreTable switchWrappedToBranch(String
branchName) {
wrapped.catalogEnvironment());
}
- protected Map<String, String> rewriteFallbackOptions(Map<String, String>
options) {
+ protected Map<String, String> rewriteOtherOptions(Map<String, String>
options) {
Map<String, String> result = new HashMap<>(options);
- // branch of fallback table should never change
+ // branch of other table should never change
String branchKey = CoreOptions.BRANCH.key();
if (options.containsKey(branchKey)) {
- result.put(branchKey, fallback.options().get(branchKey));
+ result.put(branchKey, other.options().get(branchKey));
}
- // snapshot ids may be different between the main branch and the
fallback branch,
+ // snapshot ids may be different between the main branch and the other
branch,
// so we need to convert main branch snapshot id to millisecond,
- // then convert millisecond to fallback branch snapshot id
+ // then convert millisecond to other branch snapshot id
String scanSnapshotIdOptionKey = CoreOptions.SCAN_SNAPSHOT_ID.key();
String scanSnapshotId = options.get(scanSnapshotIdOptionKey);
if (scanSnapshotId != null) {
long id = Long.parseLong(scanSnapshotId);
long millis = wrapped.snapshotManager().snapshot(id).timeMillis();
- Snapshot fallbackSnapshot =
fallback.snapshotManager().earlierOrEqualTimeMills(millis);
- long fallbackId;
- if (fallbackSnapshot == null) {
- fallbackId = Snapshot.FIRST_SNAPSHOT_ID;
+ Snapshot otherSnapshot =
other.snapshotManager().earlierOrEqualTimeMills(millis);
+ long otherId;
+ if (otherSnapshot == null) {
+ otherId = Snapshot.FIRST_SNAPSHOT_ID;
} else {
- fallbackId = fallbackSnapshot.id();
+ otherId = otherSnapshot.id();
}
- result.put(scanSnapshotIdOptionKey, String.valueOf(fallbackId));
+ result.put(scanSnapshotIdOptionKey, String.valueOf(otherId));
}
- // bucket number of main branch and fallback branch are very likely
different,
- // so we remove bucket in options to use fallback branch's bucket
number
+ // bucket number of main branch and other branch are very likely
different,
+ // so we remove bucket in options to use other branch's bucket number
result.remove(CoreOptions.BUCKET.key());
return result;
}
@Override
public DataTableScan newScan() {
+ return newScan(FileStoreTable::newScan);
+ }
+
+ public DataTableScan newScan(Function<FileStoreTable, DataTableScan>
scanCreator) {
validateSchema();
+ FileStoreTable first = wrappedFirst ? wrapped : other;
+ FileStoreTable second = wrappedFirst ? other : wrapped;
return new FallbackReadScan(
Review Comment:
`newScan(Function<FileStoreTable, DataTableScan>)` correctly allows callers
(e.g. system tables) to customize how scans are created, but `FallbackReadScan`
still creates its partition-listing scans via `wrappedTable.newScan()` /
`fallbackTable.newScan()` internally. That bypasses the provided `scanCreator`
and can make partition-ownership decisions inconsistent with the actual
`mainScan`/`fallbackScan` behavior (notably for `ReadOptimizedTable`). Consider
threading `scanCreator` through to `FallbackReadScan` and using it for the
partition listing scans too.
--
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]
