Core: add a unit test for view version to schema consistency #14334
Conversation
haizhou-zhao
force-pushed
the
view_conflict
branch
from
5b65b0b to
597f159
Compare
core/src/main/java/org/apache/iceberg/view/BaseViewOperations.java
Outdated
Show resolved
Hide resolved
| * Assuming that view ID is incrementing integers, so the last assigned view version ID is always | ||
| * the max ID that has been assigned. | ||
| */ | ||
| class AssertLastAssignedViewVersionID implements UpdateRequirement { |
There was a problem hiding this comment.
here's some historical context on why I didn't add it back then when introducing views: #8147 (comment)
There was a problem hiding this comment.
ack. Discussed offline
| } | ||
| } | ||
|
|
||
| class AssertCurrentViewVersionID implements UpdateRequirement { |
There was a problem hiding this comment.
here's some historical context on why I didn't add it back then when views were introduced: #8147 (comment)
There was a problem hiding this comment.
I guess this comes down to whether we have a good enough argument for making a "replace view" fail
There was a problem hiding this comment.
yes, as mentioned offline, we don't want to fail replace view in these cases
haizhou-zhao
force-pushed
the
view_conflict
branch
2 times, most recently
from
32c9297 to
a43c277
Compare
haizhou-zhao
changed the title
haizhou-zhao
force-pushed
the
view_conflict
branch
from
a43c277 to
56cc926
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. |
| .dialect("trino") | ||
| .build()) | ||
| .build()); | ||
| assertThat(updatedView.schemas().get(updatedView.version(1).schemaId()).sameSchema(SCHEMA)); |
There was a problem hiding this comment.
the assertion isn't doing anything. Also to be consistent with other checks in this class, can you please update it to assertThat(updatedView.schemas().get(updatedView.version(1).schemaId()).asStruct()) .isEqualTo(SCHEMA.asStruct());
| updatedView | ||
| .schemas() | ||
| .get(updatedView.version(2).schemaId()) | ||
| .sameSchema(SCHEMA_WITH_EXTRA_COL)); |
There was a problem hiding this comment.
please update this check and other checks as well
|
|
||
| // idempotency: verify the latest version matches either of the | ||
| // desired schema and sql string, no matter the type of catalog | ||
| assertThat(updatedView.schema().sameSchema(SCHEMA_WITH_EXTRA_COL)); |
There was a problem hiding this comment.
| assertThat(updatedView.schema().sameSchema(SCHEMA_WITH_EXTRA_COL)); | |
| assertThat(updatedView.schema().asStruct()).isEqualTo(SCHEMA_WITH_EXTRA_COL.asStruct()); |
| } | ||
|
|
||
| @Test | ||
| public void concurrentUpdateViewSchema() { |
| assertThat( | ||
| ((SQLViewRepresentation) updatedView.currentVersion().representations().get(0)).sql()) | ||
| .isEqualTo("select id, data, extra from ns.tbl"); | ||
| } |
There was a problem hiding this comment.
we might also want to check the history() and the versions() here
2 participants
Purpose of the change:
Added 2 new unit test: