Copilot commented on code in PR #7553:
URL: https://github.com/apache/paimon/pull/7553#discussion_r3009321856
##########
paimon-core/src/main/java/org/apache/paimon/table/FallbackReadFileStoreTable.java:
##########
@@ -580,8 +599,10 @@ private class Read implements InnerTableRead {
private final InnerTableRead fallbackRead;
private Read() {
- this.mainRead = wrapped.newRead();
- this.fallbackRead = fallback.newRead();
+ FileStoreTable first = wrappedFirst ? wrapped : other;
+ FileStoreTable second = wrappedFirst ? other : wrapped;
+ this.mainRead = first.newRead();
+ this.fallbackRead = second.newRead();
}
Review Comment:
In primary-branch mode (`wrappedFirst=false`), `fallbackRead`/`isFallback`
correspond to reads from the *current* branch (the non-priority table). The
later log message still says "Reading from fallback branch...", which becomes
misleading. Consider renaming these variables/messages to neutral terms (e.g.,
priority/supplemental) or including the actual branch/table name in the log.
##########
paimon-core/src/main/java/org/apache/paimon/schema/SchemaValidation.java:
##########
@@ -288,14 +288,34 @@ public static void validateTableSchema(TableSchema
schema) {
public static void validateFallbackBranch(SchemaManager schemaManager,
TableSchema schema) {
String fallbackBranch =
schema.options().get(CoreOptions.SCAN_FALLBACK_BRANCH.key());
+ String primaryBranch =
schema.options().get(CoreOptions.SCAN_PRIMARY_BRANCH.key());
+
+ if (!StringUtils.isNullOrWhitespaceOnly(fallbackBranch)
+ && !StringUtils.isNullOrWhitespaceOnly(primaryBranch)) {
+ throw new IllegalArgumentException(
+ String.format(
+ "Cannot set both '%s' and '%s' at the same time.",
+ CoreOptions.SCAN_FALLBACK_BRANCH.key(),
+ CoreOptions.SCAN_PRIMARY_BRANCH.key()));
+ }
+
if (!StringUtils.isNullOrWhitespaceOnly(fallbackBranch)) {
checkArgument(
schemaManager.copyWithBranch(fallbackBranch).latest().isPresent(),
- "Cannot set '%s' = '%s' because the branch '%s' isn't
existed.",
+ "Cannot set '%s' = '%s' because the branch '%s' does not
exist.",
CoreOptions.SCAN_FALLBACK_BRANCH.key(),
fallbackBranch,
fallbackBranch);
}
+
+ if (!StringUtils.isNullOrWhitespaceOnly(primaryBranch)) {
+ checkArgument(
+
schemaManager.copyWithBranch(primaryBranch).latest().isPresent(),
+ "Cannot set '%s' = '%s' because the branch '%s' isn't
existed.",
Review Comment:
Error message grammar is inconsistent with the fallback-branch validation
above ("does not exist"). This string uses "isn't existed", which is
ungrammatical; align it to "does not exist" for clarity and consistency.
```suggestion
"Cannot set '%s' = '%s' because the branch '%s' does not
exist.",
```
##########
paimon-core/src/test/java/org/apache/paimon/table/FallbackReadFileStoreTableTest.java:
##########
@@ -165,43 +158,91 @@ public void testPlanWithDataFilter() throws Exception {
FileStoreTable branchTable = createTableFromBranch(mainTable,
branchName);
- // Fallback branch: partition 1 already has a=10 (inherited), add
a=100.
- // Also add partition 3 (a=30) which is fallback-only.
- writeDataIntoTable(branchTable, 1, rowData(1, 100), rowData(3, 30));
+ // Branch: partition 1 (a=100), partition 3 (a=30)
+ writeDataIntoTable(branchTable, 0, rowData(1, 100), rowData(3, 30));
- FallbackReadFileStoreTable fallbackTable =
- new FallbackReadFileStoreTable(mainTable, branchTable);
+ FallbackReadFileStoreTable table =
+ new FallbackReadFileStoreTable(mainTable, branchTable,
wrappedFirst);
PredicateBuilder builder = new PredicateBuilder(ROW_TYPE);
- // Case 1: WHERE pt = 1 AND a = 100
- // Partition 1 exists in main branch. Even though main has no a=100
data,
- // we should never fall back for it. The result should contain no
fallback splits.
- DataTableScan scan1 = fallbackTable.newScan();
- scan1.withFilter(PredicateBuilder.and(builder.equal(0, 1),
builder.equal(1, 100)));
+ // partition 1 exists in both tables, priority table wins
+ // wrappedFirst=true → main has priority, partition only in branch is 3
+ // wrappedFirst=false → branch has priority, partition only in main is
2
+ int partOnlyInSupplemental = wrappedFirst ? 3 : 2;
+
+ // Case 1: WHERE pt = 1
+ // Partition 1 exists in priority table. Should never be read from
supplemental.
+ DataTableScan scan1 = table.newScan();
+ scan1.withFilter(builder.equal(0, 1));
Review Comment:
`testPlanWithDataFilter` no longer applies any data predicate (it only
filters on partition `pt`). This means the test no longer covers the scenario
described in the Javadoc (where data filters can eliminate all files in a
partition and must not affect partition ownership). Consider reintroducing a
data predicate (e.g., `pt = 1 AND a = 100`) to ensure the plan logic is
validated against data-filtered scans.
```suggestion
// Case 1: WHERE pt = 1 AND a = 100
// Partition 1 exists in priority table. Should never be read from
supplemental.
DataTableScan scan1 = table.newScan();
scan1.withFilter(
builder.and(builder.equal(0, 1), builder.equal(1, 100)));
```
##########
paimon-core/src/main/java/org/apache/paimon/table/AbstractFileStoreTable.java:
##########
@@ -715,6 +715,17 @@ public void deleteBranch(String branchName) {
branchName, fallbackBranch));
}
+ String primaryBranch =
coreOptions().toConfiguration().get(CoreOptions.SCAN_PRIMARY_BRANCH);
+ if (!StringUtils.isNullOrWhitespaceOnly(primaryBranch)
+ && branchName.equals(primaryBranch)) {
+ throw new IllegalArgumentException(
+ String.format(
+ "can not delete the primary branch. "
+ + "branchName to be deleted is %s. you
have set 'scan.primary-branch' = '%s'. "
+ + "you should reset 'scan.primary-branch'
before deleting this branch.",
+ branchName, primaryBranch));
Review Comment:
The newly added exception message is user-facing and has several
grammar/capitalization issues ("can not", sentence casing, repeated "you").
Consider rephrasing to a clearer message (e.g., "Cannot delete branch '%s'
because it is configured as scan.primary-branch; unset scan.primary-branch
first.") and, ideally, aligning with the fallback-branch message for
consistency.
--
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]
