API: Add compatibility checks for Schemas with default values#11434
Conversation
|
|
||
| @ParameterizedTest | ||
| @ValueSource(ints = {1, 2, 3}) | ||
| public void testSupportedInitialDefault(int formatVersion) { |
There was a problem hiding this comment.
testSupportedWriterDefault?
There was a problem hiding this comment.
| public void testSupportedInitialDefault(int formatVersion) { | |
| public void testSupportedWriterDefault(int formatVersion) { |
| @ParameterizedTest | ||
| @ValueSource(ints = {1, 2, 3}) | ||
| public void testSupportedInitialDefault(int formatVersion) { | ||
| // only the initial default is a forward-incompatible change |
There was a problem hiding this comment.
nit: We could make this self contained to the test by saying
Writer defaults are universally compatible
Or something?
|
Tests failing are because Metadata.json also has validations for this and this check is failing first |
| problems.put( | ||
| field.fieldId(), | ||
| String.format( | ||
| "Invalid type for %s: %s is not supported until v%s", |
There was a problem hiding this comment.
Existing checks in TableMetadata look like
"Invalid type in v%s schema: struct.ts_nanos timestamptz_ns is not supported until v3"
RussellSpitzer
left a comment
RussellSpitzer
left a comment
There was a problem hiding this comment.
Looks good to me, dm'd a patch for the one broken test (Expected error message has changed)
And one required change for the test name in this PR which is I believe misnamed
|
Updated. Thanks, @RussellSpitzer! |
Co-authored-by: Fokko Driesprong <fokko@apache.org>
singhpk234
left a comment
singhpk234
left a comment
There was a problem hiding this comment.
LGTM, thanks @RussellSpitzer
| public static void checkCompatibility(Schema schema, int formatVersion) { | ||
| // check the type in each field | ||
| // accumulate errors as a treemap to keep them in a reasonable order | ||
| Map<Integer, String> problems = Maps.newTreeMap(); |
There was a problem hiding this comment.
[optional] if we are going for another revision as fieldId() is int :
| Map<Integer, String> problems = Maps.newTreeMap(); | |
| Map<int, String> problems = Maps.newTreeMap(); |
There was a problem hiding this comment.
Primitives are not allowed as Map Keys :(
There was a problem hiding this comment.
Or any type parameter I think ... I feel like this is a 1337code question
Fokko
left a comment
Fokko
left a comment
There was a problem hiding this comment.
Looks good! 🚀 Let's get 1.7.0 out :)
|
Thanks everyone - @rdblue (for original fix), @Fokko , @anuragmantri , @kevinjqliu , @singhpk234 , @amogh-jahagirdar For reviews! |
…#11434) Co-authored-by: Russell Spitzer <russell.spitzer@gmail.com> Co-authored-by: Fokko Driesprong <fokko@apache.org>
7 participants
This updates
Schema.checkCompatibility:initial-defaultis not set for v1 and v2 tables, which would break forward compatibilitycheckCompatibilityinTestSchemaI think that this should get into 1.7.0 to avoid accidentally leaking initial defaults in v1 and v2 tables.