[Kernel] Add icebergCompatV2 metadata/protocol checks #4199
Conversation
kernel/kernel-api/src/main/java/io/delta/kernel/internal/TransactionBuilderImpl.java
Show resolved
Hide resolved
| } | ||
| }; | ||
|
|
||
| private static class IcebergCompatMetadataValidatorAndUpdater { |
There was a problem hiding this comment.
so this is our version of IcebergCompat right?
There was a problem hiding this comment.
I think it might help just organization wise to put all the generic interfaces in one file, and then for each iceberg compat table feature define the implementations in their own file. But maybe we can also just group these elements differently not sure.
But I think IcebergCompatMetadataValidatorAndUpdater seems fine to be it's own top-level class
There was a problem hiding this comment.
separated the validator and updated into a separate class
There was a problem hiding this comment.
Did you mean to remove this code that is now in the separate file? Sorry left comments here
kernel/kernel-api/src/main/java/io/delta/kernel/internal/IcebergCompatV2Utils.java
Show resolved
Hide resolved
|
|
||
| /** Wrapper class for table property validation */ | ||
| private static class RequiredDeltaTablePropertyEnforcer { | ||
| public final String propertyName; |
There was a problem hiding this comment.
maybe take a TableConfig instead?
There was a problem hiding this comment.
I started with TableConfig, but ended with Java generics compilation issue. The code has changed a bit since last I tried. Give it a try.
vkorukanti
force-pushed
the
cmIcebergCompatV2
branch
from
d8a4221 to
7d98014
Compare
There was a problem hiding this comment.
overall structure and approach makes sense to me! only open question is about the order of updating the configuration vs the schema (column mapping related)
Can we move the iceberg compat checks earlier such that they only touch the metadata.configuration?
And then after, in one step, we update the schema with the column mapping column metadata (applicable to all column mapping tables not just iceberg compat ones)?
Not sure if there is a specific reason this will not work
| new PostMetadataProcessor() { | ||
| @Override | ||
| Optional<Metadata> postProcess(Metadata newMetadata, boolean isCreatingNewTable) { | ||
| // TODO: assign column mapping field ids |
There was a problem hiding this comment.
Wouldn't we do this elsewhere later on if we just enable column mapping mode in the metadata? (We have to do this for non-IcebergCompatV2 tables anyways right)
There was a problem hiding this comment.
Yes, we assign the field IDs if they are not already assigned. Calling it multiple times is not an issue. Also this stems from the lack of specific order of checking and making metadata updates.
| for (RequiredDeltaTablePropertyEnforcer requiredDeltaTableProperty : | ||
| requiredDeltaTableProperties) { |
There was a problem hiding this comment.
Since these refer to independent properties there's never going to be dependencies between the two right?
There was a problem hiding this comment.
Yes, so far thats the assumption.
| } | ||
| }; | ||
|
|
||
| private static class IcebergCompatMetadataValidatorAndUpdater { |
There was a problem hiding this comment.
Did you mean to remove this code that is now in the separate file? Sorry left comments here
There was a problem hiding this comment.
maybe adjust the names of the two files that are the same names? it's confusing. Maybe the other one will just be IcebergCompatUtils in the future and house pointers to all our versions + validators?
There was a problem hiding this comment.
or maybe you meant to remove the other one I'm not sure good with either
There was a problem hiding this comment.
basically the IcebergCompatMetadataValidatorAndUpdater contains the infra and interfaces and IcebergCompatV2Utils just for the icebergCompatV2 checks
kernel/kernel-api/src/main/java/io/delta/kernel/internal/TransactionImpl.java
Show resolved
Hide resolved
| } else { | ||
| // In all other cases, if the property value is not compatible | ||
| // with the IcebergV1 requirements, we fail | ||
| throw new UnsupportedOperationException( |
There was a problem hiding this comment.
make a DeltaErrors method
| // check for required dependency table features | ||
| for (TableFeature requiredDependencyTableFeature : requiredDependencyTableFeatures) { | ||
| if (!inputContext.newProtocol.supportsFeature(requiredDependencyTableFeature)) { | ||
| throw new UnsupportedOperationException( |
There was a problem hiding this comment.
make a DeltaErrors method
| }); | ||
|
|
||
| if (!matches.isEmpty()) { | ||
| throw new UnsupportedOperationException( |
There was a problem hiding this comment.
make a DeltaErrors method
| || dataType instanceof TimestampType | ||
| || dataType instanceof TimestampNTZType; | ||
| if (!validType) { | ||
| throw new UnsupportedOperationException( |
There was a problem hiding this comment.
make a DeltaErrors method
| if (inputContext.newProtocol.supportsFeature(TableFeatures.TYPE_WIDENING_RW_FEATURE)) { | ||
| // TODO: Currently Kernel has no support for writing with type widening. When it is | ||
| // supported extend this to allow a whitelist of supported type widening in Iceberg | ||
| throw new UnsupportedOperationException( |
There was a problem hiding this comment.
make a DeltaErrors method
| private static final IcebergCompatCheck ICEBERG_COMPAT_V2_CHECK_HAS_NO_DELETION_VECTORS = | ||
| (inputContext) -> { | ||
| if (inputContext.newProtocol.supportsFeature(TableFeatures.DELETION_VECTORS_RW_FEATURE)) { | ||
| throw new UnsupportedOperationException( |
There was a problem hiding this comment.
make a DeltaErrors method
| for (RequiredDeltaTablePropertyEnforcer requiredDeltaTableProperty : | ||
| requiredDeltaTableProperties) { |
There was a problem hiding this comment.
Yes, so far thats the assumption.
There was a problem hiding this comment.
basically the IcebergCompatMetadataValidatorAndUpdater contains the infra and interfaces and IcebergCompatV2Utils just for the icebergCompatV2 checks
Description
WIP: support for validating icebergCompatV2 rmetadata requirements are satisfied
How was this patch tested?
In progress