Repository navigation
Core: Add basic classes for writing table format-version 4 - #13123
Conversation
| @Parameters(name = "formatVersion = {0}") | ||
| public static List<Object> parameters() { | ||
| return Arrays.asList(2, 3); | ||
| return Arrays.asList(2, 3, 4); |
There was a problem hiding this comment.
nit: Maybe we should change this to use the MAX_VERSION or all Versions parameter with a (not 1) or something
There was a problem hiding this comment.
good point, I've updated all of those places to use MAX_VERSION and to skip v1
| new Object[] {1, "testBranch"}, | ||
| new Object[] {2, "main"}, | ||
| new Object[] {2, "testBranch"}); | ||
| return TestHelpers.ALL_VERSIONS.stream() |
RussellSpitzer
left a comment
RussellSpitzer
left a comment
There was a problem hiding this comment.
Love this, especially fixing so many of the tests to hit all versions now programmatically. I think there are few instances still in the code we can make less manual in the upgrade so maybe we should fix those in this PR as well. I puts some comments on them.
c829ec0 to
5c28ed7
Compare
| private TestHelpers() {} | ||
|
|
||
| public static final int MAX_FORMAT_VERSION = 3; | ||
| public static final int MAX_FORMAT_VERSION = 4; |
There was a problem hiding this comment.
Since v4 spec is not there, I am not sure we should enable this in code at the moment.
What if user configures v4, what does it actually mean a v4 table?
There was a problem hiding this comment.
We need this to enable v4 Dev work
There was a problem hiding this comment.
Without the Spec, why do we need to start the work?
Do you mean a POC? In that case, it can be do in a separate branch?
There was a problem hiding this comment.
we did the same with #10760 which was added right around the same time when v3 Spec work happened. Since there will be multiple efforts working on v4 stuff the v4 base classes should be added to main to make development easier for everyone
There was a problem hiding this comment.
We can add a line to the Spec.md that V3 is done and V4 is now open and experimental
There was a problem hiding this comment.
@danielcweeks Is updating SUPPORTED_TABLE_FORMAT_VERSION necessary to unblock v4 dev work?
There was a problem hiding this comment.
@manuzhang yes, otherwise you won't be able to create a v4 table for testing
There was a problem hiding this comment.
Okay, but once 1.10 is released, users will be able to create v4 table even though no implementation has done, right?
There was a problem hiding this comment.
yes that's correct but there just won't be any v4 features available that those users could use. Are you worried that this will lead to issues? As I mentioned in an earlier comment we already did the same thing with v3. We added the v3-specific writers first before v3 spec and implementation work landed on main
There was a problem hiding this comment.
I've talked offline with @manuzhang and he's fine with this since we already did the same thing for v3
|
thanks everyone for the review, I'll go ahead and merge this to unblock v4 work |
No description provided.