vinishjail97 commented on code in PR #903:
URL: https://github.com/apache/incubator-xtable/pull/903#discussion_r4223774358
##########
xtable-core/src/main/java/org/apache/xtable/kernel/DeltaKernelConversionTarget.java:
##########
@@ -229,6 +244,30 @@ public void beginSync(InternalTable table) {
@Override
public void syncSchema(InternalSchema schema) {
+ if (transactionState.isTableExists()) {
+ // Compare in Kernel's own StructType, not InternalSchema, to avoid
false positives from
+ // any asymmetry in the InternalSchema<->StructType round trip.
transactionState's current
+ // latestSchema still holds the schema loaded from the existing snapshot
at this point,
+ // since it is only overwritten by the setLatestSchema() call below.
+ StructType existingSchema = transactionState.getLatestSchema();
+ StructType newSchema = schemaExtractor.fromInternalSchema(schema);
+ if (!newSchema.equals(existingSchema)) {
Review Comment:
An unchanged resync of a Standalone-written table fails here for Hudi and
Iceberg sources. `fromInternalSchema` adds `delta.columnMapping.id` for Iceberg
and flips nested nullability for Hudi, so the schemas never match. Would
comparing names and types only work here?
##########
xtable-core/src/main/java/org/apache/xtable/delta/DeltaConversionTargetConfig.java:
##########
@@ -26,24 +26,27 @@
* Configuration of the Delta target format for the sync process.
*
* <p>Controls which Delta writer implementation backs a Delta {@code
ConversionTarget}. By default
- * syncs use the Delta Standalone based {@link DeltaConversionTarget}. Setting
{@link #USE_KERNEL}
- * to {@code true} in the target table's additional properties routes the sync
through the Delta
- * Kernel based {@code DeltaKernelConversionTarget} instead.
+ * syncs use the Delta Kernel based {@code DeltaKernelConversionTarget}, per
+ * https://github.com/apache/incubator-xtable/issues/886. Setting {@link
#USE_KERNEL} to {@code
+ * false} in the target table's additional properties falls back to the Delta
Standalone based
+ * {@link DeltaConversionTarget} instead -- the escape hatch for a regression
that only exists on
+ * the Kernel path, e.g. schema evolution on an existing table (Kernel fails
fast on that rather
+ * than silently committing a stale schema; see delta-io/delta#4305).
*/
@Value
public class DeltaConversionTargetConfig {
/**
- * When {@code true}, Delta syncs are written using the Delta Kernel
implementation instead of the
- * default Delta Standalone implementation. Defaults to {@code false}.
+ * When {@code false}, Delta syncs fall back to the Delta Standalone
implementation instead of the
+ * default Delta Kernel implementation. Defaults to {@code true}.
*/
public static final String USE_KERNEL = "xtable.delta.target.use_kernel";
boolean useKernel;
public static DeltaConversionTargetConfig fromProperties(Properties
properties) {
boolean useKernel =
- properties != null
- && Boolean.parseBoolean(properties.getProperty(USE_KERNEL,
Boolean.FALSE.toString()));
+ properties == null
+ || Boolean.parseBoolean(properties.getProperty(USE_KERNEL,
Boolean.TRUE.toString()));
Review Comment:
With this default, a `timestamp_micros_nullable_field:DAY:yyyy/MM/dd`
partition fails with `requires writer table feature "[generatedColumns]"`. CI
misses it because the existing DELTA cases are pinned to Standalone.
##########
xtable-hudi-support/xtable-hudi-support-extensions/src/main/java/org/apache/xtable/hudi/sync/XTableSyncTool.java:
##########
@@ -85,10 +85,13 @@ public void syncHoodieTable() {
? Duration.ofHours(
config.getInt(XTableSyncConfig.XTABLE_TARGET_METADATA_RETENTION_HOURS))
: null;
+ // Pin explicitly rather than relying on DeltaConversionTargetConfig's own
default, so this
+ // flag's documented behavior (defaults to Delta Standalone) holds
regardless of which
+ // implementation that default currently points at.
Properties targetProperties = new Properties();
- if (config.getBooleanOrDefault(XTableSyncConfig.XTABLE_DELTA_USE_KERNEL)) {
- targetProperties.setProperty(DeltaConversionTargetConfig.USE_KERNEL,
Boolean.TRUE.toString());
- }
+ targetProperties.setProperty(
+ DeltaConversionTargetConfig.USE_KERNEL,
Review Comment:
Nit: this tool stays on Standalone, the CLI yaml flips only the source, and
the service flips both. Could these be aligned, or noted in the release notes?
##########
xtable-core/src/test/java/org/apache/xtable/ITConversionController.java:
##########
@@ -1346,15 +1545,26 @@ private static ConversionConfig getTableSyncConfig(
List<TargetTable> targetTables =
targetTableFormats.stream()
.map(
- formatName ->
- TargetTable.builder()
- .name(tableName)
- .formatName(formatName)
- // set the metadata path to the data path as the
default (required by Hudi)
- .basePath(table.getDataPath())
- .metadataRetention(metadataRetention)
- .additionalProperties(hudiTargetProperties(formatName,
hudiTargetVersion))
- .build())
+ formatName -> {
+ TypedProperties targetProperties =
+ hudiTargetProperties(formatName, hudiTargetVersion);
+ if (formatName.equals(DELTA)) {
+ // Pin explicitly rather than relying on
DeltaConversionTargetConfig's
+ // default, so the Standalone cases stay Standalone
regardless of which
+ // implementation the default currently points at.
+ targetProperties.setProperty(
+ DeltaConversionTargetConfig.USE_KERNEL,
+ String.valueOf(useDeltaKernelTarget));
Review Comment:
Would it work to leave the DELTA cases on the default, so the matrix tests
what ships?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
