Core: Fix failure when reading files table with branch #11719
Conversation
|
This makes sense to me. How about other metadata tables? @dramaticlly do you also want take a look? |
|
Is it ready for review? |
Happy to help with other metadata tables experienced similar problem if it's not addressed in this PR |
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
ebyhr
force-pushed
the
ebi/files-table
branch
from
8c47d5e to
a42f1ac
Compare
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
|
@ebyhr can you mark as non-draft if its ready to review? |
ebyhr
force-pushed
the
ebi/files-table
branch
from
a42f1ac to
d889982
Compare
|
Changed to non-draft status. Let me leave other metadata tables to @dramaticlly or other contributors. |
|
Thanks! @dramaticlly can you review as well? |
| @@ -671,6 +671,76 @@ public void testFilesTableTimeTravelWithSchemaEvolution() throws Exception { | |||
| .hasSameSizeAs(expectedFiles); | |||
| } | |||
|
|
|||
| @TestTemplate | |||
| public void testFilesTableVersionAsOfTag() { | |||
There was a problem hiding this comment.
Looks like this test will pass even without the change in this PR. But the next AsOfBranch will fail
There was a problem hiding this comment.
Yes, do you want to remove this test or separate a PR?
| @@ -51,6 +52,20 @@ public static class FilesTableScan extends BaseFilesTableScan { | |||
| super(table, schema, MetadataTableType.FILES, context); | |||
| } | |||
|
|
|||
| @Override | |||
| public TableScan useRef(String name) { | |||
There was a problem hiding this comment.
I think in order to support other metadata tables which also have different schema than its base table schema, we might just pull this up from FilesTableScan to BaseMetadataTableScan instead. So it will works with .partitions and .entries etc. Also I think BaseAllMetadataTableScan overrides this useRef method as well
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
SnapshotScan.useRef returns the base table's schema instead of metadata table's schema:
iceberg/core/src/main/java/org/apache/iceberg/SnapshotScan.java
Line 105 in deeb04b
I would leave other metadata tables to other contributors.
Fixes #11701