-
Couldn't load subscription status.
- Fork 1.9k
[Kernel] Introduce IcebergCompatV3 #4595
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Kernel] Introduce IcebergCompatV3 #4595
Conversation
.../java/io/delta/kernel/internal/icebergcompat/IcebergCompatV3MetadataValidatorAndUpdater.java
Show resolved
Hide resolved
.../java/io/delta/kernel/internal/icebergcompat/IcebergCompatV3MetadataValidatorAndUpdater.java
Outdated
Show resolved
Hide resolved
.../java/io/delta/kernel/internal/icebergcompat/IcebergCompatV3MetadataValidatorAndUpdater.java
Show resolved
Hide resolved
018b936 to
efa3249
Compare
efa3249 to
7e10891
Compare
| || dataType instanceof ArrayType | ||
| || dataType instanceof MapType | ||
| || dataType instanceof StructType | ||
| || dataType instanceof VariantType; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this
|
|
||
| @Override | ||
| List<TableFeature> requiredDependencyTableFeatures() { | ||
| return Stream.of(ICEBERG_COMPAT_V3_W_FEATURE, COLUMN_MAPPING_RW_FEATURE).collect(toList()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add row tracking here ?
| field -> { | ||
| DataType dataType = field.getDataType(); | ||
| // IcebergCompatV3 supports variants and all the IcebergCompatV2 supported types | ||
| return !isSupportedDataTypesForV2(dataType) && !(dataType instanceof VariantType); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make the new v3 types in a separate list to facilitate adding more later.
| * Compatibility V3 protocol. | ||
| * | ||
| * @see <a | ||
| * href="https://github.com/delta-io/delta/blob/master/PROTOCOL.md#delta-iceberg-compatibility-v3"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like this link doesn't exists?
| } | ||
|
|
||
| protected static boolean isAdditionalSupportedDataTypesForV3(DataType dataType) { | ||
| return dataType instanceof VariantType; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also check isSupportedDataTypesForV2 and the new type supported?
.../java/io/delta/kernel/internal/icebergcompat/IcebergCompatV3MetadataValidatorAndUpdater.java
Show resolved
Hide resolved
| private static final IcebergCompatV3MetadataValidatorAndUpdater INSTANCE = | ||
| new IcebergCompatV3MetadataValidatorAndUpdater(); | ||
|
|
||
| private static final IcebergCompatRequiredTablePropertyEnforcer ICEBERG_COMPAT_V3_CM_REQUIREMENT = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
create a follow up task to define this check in the base class and reuse it. You can post that as the first PR precuros to this PR.
| if (Boolean.parseBoolean( | ||
| inputContext.newMetadata.getConfiguration().getOrDefault(prop, "false"))) { | ||
| throw DeltaErrors.icebergCompatIncompatibleVersionEnabled( | ||
| INSTANCE.compatFeatureName(), prop); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you check if you use the utility in TableConfig.PROP.fromMetadata here?
.../java/io/delta/kernel/internal/icebergcompat/IcebergCompatV3MetadataValidatorAndUpdater.java
Outdated
Show resolved
Hide resolved
| } | ||
| }; | ||
|
|
||
| private static final IcebergCompatCheck ICEBERG_COMPAT_V3_CHECK_HAS_ALLOWED_PARTITION_TYPES = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment here. create common check in the base class.
| // this to allow checking the partition columns aren't changed | ||
| }; | ||
|
|
||
| private static final IcebergCompatCheck ICEBERG_COMPAT_V3_CHECK_HAS_SUPPORTED_TYPE_WIDENING = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here.
.../java/io/delta/kernel/internal/icebergcompat/IcebergCompatV3MetadataValidatorAndUpdater.java
Show resolved
Hide resolved
| * Base suite containing common test cases for Delta Iceberg compatibility features. | ||
| * This includes tests that apply to both V2 and V3 compatibility modes. | ||
| */ | ||
| trait DeltaIcebergCompatBaseSuite extends DeltaTableWriteSuiteBase with ColumnMappingSuiteBase { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to Reviewers: I move some common test cases from DeltaIcebergCompatV2Suite so that these test cases could be shared with both DeltaIcebergCompatV2Suite and DeltaIcebergCompatV3Suite. Since IcebergCompatV3 does not support being disabled or enabled for existing tables, so the existing tables related suites are kept in DeltaIcebergCompatV2Suite
.../java/io/delta/kernel/internal/icebergcompat/IcebergCompatV3MetadataValidatorAndUpdater.java
Outdated
Show resolved
Hide resolved
...el/kernel-defaults/src/test/scala/io/delta/kernel/defaults/DeltaIcebergCompatBaseSuite.scala
Outdated
Show resolved
Hide resolved
...el/kernel-defaults/src/test/scala/io/delta/kernel/defaults/DeltaIcebergCompatBaseSuite.scala
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| test("subsequent writes to icebergCompatV3 enabled tables doesn't update metadata") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this test case seems a bit over kill to me, what's the case for other iceberg compat?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also have the same test case in IcebergCoompatV2, https://github.com/delta-io/delta/blob/master/kernel/kernel-defaults/src/test/scala/io/delta/kernel/defaults/DeltaIcebergCompatV2Suite.scala#L192-L224
| || Boolean.parseBoolean( | ||
| metadata | ||
| .getConfiguration() | ||
| .getOrDefault(TableConfig.ICEBERG_COMPAT_V3_ENABLED.getKey(), "false"))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please create a utility method in IcebergCompatMetadataValidatorAndUpdader and move this code there. Also use TableConfig.ICEBERG_COMPAT_V3_ENABLED.fromMetadata()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| s"icebergCompat$icebergCompatVersion does not support type widening present in table")) | ||
| } | ||
|
|
||
| Seq("id", "name").foreach { existingCMMode => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this test be also added to IcebergWriterCompatV1MetadataValidatorAndUpdaterSuite?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or is it wrong in the first place where it is ran as part of the WriterCompatV2 suite, but it is always using icebergCompatV2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Qn: does icebergCompatV3 can work with either modes or just id?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test case previously existed only in the icebergCompatV2 suite. Since icebergCompatV2 supports both column mapping modes, whereas icebergWriteCompatV2 supports only the id mode, the test—which only verifies whether the column mapping modes can be enabled with icebergCompatV2—belongs specifically in the icebergCompatV2 suite. Therefore, it’s appropriate to keep it there rather than moving it to the shared base suite.
| } | ||
| } | ||
|
|
||
| Seq("id", "name").foreach { existingCMMode => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why can't move this to the base class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same reason #4595 (comment)
| " not compatible with icebergCompatV3 requirements")) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about the test where row tracking is enabled (if not enabled)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also only icebergCompatV3 is enabled and not others compact versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we getting the coverage for type widening?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also deletion vectors? do we need to test anything there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type widening and deletion vectors(we mark isDeletionVectorsSupported as true here) are in base class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added case to verify row tracking is auto enabled and only icebergCompatV3 is enabled with not others compact versions.
1886ff0 to
a80d79a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
one test on variant support (not supported yet)
replace table case
kernel/kernel-api/src/test/scala/io/delta/kernel/TransactionSuite.scala
Outdated
Show resolved
Hide resolved
| if (isIcebergCompatV2Enabled) { | ||
| IcebergCompatV2MetadataValidatorAndUpdater.validateDataFileStatus(dataFileStatus); | ||
| } else if (isIcebergCompatV3Enabled) { | ||
| IcebergCompatV3MetadataValidatorAndUpdater.validateDataFileStatus(dataFileStatus); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can use isIcebergCompatEnabled and just call IcebergCompatMetadataValidatorAndUpdater.validateDataFileStatus
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would push back this as IcebergCompatMetadataValidatorAndUpdater.validateDataFileStatus also need a String compatFeatureName to better expose the error message, so it would not reduce the code imo.
.../io/delta/kernel/internal/icebergcompat/IcebergCompatV3MetadataValidatorAndUpdateSuite.scala
Outdated
Show resolved
Hide resolved
kernel/kernel-defaults/src/test/scala/io/delta/kernel/defaults/DeltaIcebergCompatV3Suite.scala
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Which Delta project/connector is this regarding?
Description
Introduce IcebergCompatV3 feature, which requires row tracking and column mapping to be enabled. We block the disabling or enabling IcebergCompatV3 on existing tables.
This PR is a stacked PR and based on #4731 and #4732
How was this patch tested?
Unit tests and e2e tests.
Does this PR introduce any user-facing changes?