vinishjail97 commented on code in PR #938:
URL: https://github.com/apache/incubator-xtable/pull/938#discussion_r4008604934
##########
xtable-utilities/src/main/java/org/apache/xtable/utilities/RunSync.java:
##########
@@ -184,12 +183,28 @@ static void syncTableMetdata(
.build();
try {
conversionController.sync(conversionConfig, conversionSourceProvider);
+ } catch (NotSupportedException e) {
Review Comment:
Same comment as RunCatalogSync.java that skips the sync for all the tables
when one source table is mis-configured.
##########
xtable-utilities/src/main/java/org/apache/xtable/utilities/StrictBooleanStringDeserializer.java:
##########
@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.xtable.utilities;
+
+import java.io.IOException;
+
+import com.fasterxml.jackson.core.JsonParser;
+import com.fasterxml.jackson.core.JsonToken;
+import com.fasterxml.jackson.databind.DeserializationContext;
+import com.fasterxml.jackson.databind.deser.std.StdDeserializer;
+
+/** Deserializes only the literal value {@code true} as enabled. */
+public class StrictBooleanStringDeserializer extends StdDeserializer<String> {
+
+ public StrictBooleanStringDeserializer() {
+ super(String.class);
+ }
+
+ @Override
+ public String deserialize(JsonParser parser, DeserializationContext context)
throws IOException {
+ if (parser.currentToken() == JsonToken.VALUE_NULL) {
+ return null;
+ }
+ return
Boolean.toString(Boolean.TRUE.toString().equalsIgnoreCase(parser.getText()));
Review Comment:
I am not sure the silent coercion helps the user here. A typo such as
`allowUnsupportedDeletionVectors: ture`, or a `yes`, becomes `"false"` with no
signal, and the failure they then see points at
`xtable.delta.source.allow_unsupported_deletion_vectors`, which is not the key
they typed in the YAML.
Would a plain `Boolean` field do the job? Jackson coerces both the YAML
`true` and the quoted `"true"`, and rejects `yes`, `on` and `1` with a parse
error that names the field, which is fail-closed and loud. If you would rather
keep the deserializer, throwing `InvalidFormatException` for anything other
than true/false would give the same effect. Non-blocking.
##########
xtable-core/src/main/java/org/apache/xtable/delta/DeltaDeletionVectorHandler.java:
##########
@@ -0,0 +1,65 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.xtable.delta;
+
+import java.util.function.Consumer;
+
+import lombok.extern.log4j.Log4j2;
+
+import com.google.common.annotations.VisibleForTesting;
+
+import org.apache.xtable.exception.NotSupportedException;
+
+@Log4j2
+public final class DeltaDeletionVectorHandler {
+ private final boolean allowUnsupportedDeletionVectors;
+ private final Consumer<String> warningLogger;
+
+ public DeltaDeletionVectorHandler(boolean allowUnsupportedDeletionVectors) {
+ this(allowUnsupportedDeletionVectors, log::warn);
+ }
+
+ @VisibleForTesting
+ DeltaDeletionVectorHandler(
+ boolean allowUnsupportedDeletionVectors, Consumer<String> warningLogger)
{
+ this.allowUnsupportedDeletionVectors = allowUnsupportedDeletionVectors;
+ this.warningLogger = warningLogger;
+ }
+
+ public void handle(String dataFilePath) {
Review Comment:
Two small ones, grouped so they do not clutter the review.
- `handle(String)` does not say what happens, and the argument is the data
file path rather than the deletion vector path. Something like
`onDeletionVectorFound(dataFilePath)` or `checkDeletionVector(dataFilePath)`
would read more clearly at the call sites.
- After this change the two-argument
`DeltaDataFileExtractor.iterator(Snapshot, InternalSchema)` overload has no
callers left in main or test code, so it could go rather than carry a `null`
handler.
##########
xtable-utilities/src/main/java/org/apache/xtable/utilities/RunCatalogSync.java:
##########
@@ -193,6 +196,8 @@ public static void main(String[] args) throws Exception {
conversionController.syncTableAcrossCatalogs(
conversionConfig,
getConversionSourceProviders(tableFormats, tableFormatConverters,
hadoopConf));
+ } catch (NotSupportedException e) {
Review Comment:
Same question as on `RunSync.syncTableMetdata`: this rethrow stops the loop
at the first source with a deletion vector, so the remaining sources in the
config are skipped and not the desired outcome here?
##########
website/docs/features-and-limitations.md:
##########
@@ -42,3 +42,4 @@ HMS and AWS Glue are the two catalogs supported right now,
support for other cat
### Delta
- When using Delta as the source for an Iceberg target, you may require field
IDs set in the parquet schema. To enable that, follow the instructions for
enabling column mapping
[here](https://docs.delta.io/latest/delta-column-mapping.html).
- When Delta is the source, Generated Columns are not synced to the target
schema. For tables that are partitioned on Generated Columns, there is limited
support. For example, we support date functions like transforming a timestamp
to `yyyy-MM-dd` format. Please file a GitHub issue or pull-request for any
cases that you think should be supported.
+- Delta deletion vectors are not supported by conversion targets. Sync fails
by default when a Delta source contains an active deletion vector. The bundled
utility can set `allowUnsupportedDeletionVectors: "true"` for a dataset to log
a warning and continue, but the target may then expose rows that were deleted
from the source. Before disabling the override, rebuild affected targets with a
full sync so previously ignored deletes are not retained.
Review Comment:
The recovery sentence here (and in `how-to-catalog-sync.md`) is the same one
as in the README, so the `REORG TABLE ... APPLY (PURGE)` point from the README
comment applies to all three places.
##########
xtable-core/src/main/java/org/apache/xtable/delta/DeltaConversionSource.java:
##########
@@ -215,6 +223,17 @@ public CommitsBacklog<Long> getCommitsBacklog(
.build();
}
+ private void validateActiveDeletionVectors() {
+ Snapshot snapshot = deltaLog.snapshot();
+ Iterator<AddFile> activeFiles = snapshot.allFiles().toLocalIterator();
Review Comment:
This scans every active file on every incremental sync, on top of the
per-commit check in `getTableChangeForCommit`. In the legacy reader
`snapshot.allFiles()` is a Spark `Dataset` built by state reconstruction, so
`toLocalIterator()` runs a Spark job per backlog, including for tables that
have never enabled deletion vectors.
Can we gate this scan on the protocol? Delta cannot write a deletion vector
unless the table protocol carries the `deletionVectors` feature, and Delta
2.4.0 exposes that directly as `snapshot.deletionVectorsSupported()`. The
protocol is already materialized by the time we read `snapshot.metadata()` for
the table, so the check adds no extra job and lets the common case skip the
listing entirely.
The Kernel path has the same shape (a full log replay rather than a Spark
job), and `((SnapshotImpl)
snapshot).getProtocol().supportsFeature(TableFeatures.DELETION_VECTORS_RW_FEATURE)`
is available there in Kernel 4.0.0.
##########
README.md:
##########
@@ -81,6 +81,7 @@ datasets:
- `tableBasePath` is the basePath of the table
- `tableDataPath` is an optional field specifying the path to the data files.
If not specified, the tableBasePath will be used. For Iceberg source tables,
you will need to specify the `/data` path.
- `namespace` is an optional field specifying the namespace of the table and
will be used when syncing to a catalog.
+- `allowUnsupportedDeletionVectors` is an optional, temporary Delta-source
safety override. Delta deletion vectors are rejected by default because XTable
cannot translate them to target table metadata. Setting this field to `"true"`
logs a warning and continues while ignoring the deletion vectors, which may
expose rows that were deleted from the source. Before disabling the override,
rebuild affected targets with a full sync so previously ignored deletes are not
retained.
Review Comment:
I think this recovery step is incomplete. A full sync still ignores active
deletion vectors, so the target remains inconsistent. Could we tell users to
run `REORG TABLE ... APPLY (PURGE)`, disable the override, and then run a full
sync?
##########
xtable-core/src/main/java/org/apache/xtable/kernel/DeltaKernelConversionSource.java:
##########
@@ -159,6 +174,17 @@ public TableChange getTableChangeForCommit(Long
versionNumber) {
}
}
+ for (String deletionVector : deletionVectors) {
Review Comment:
Small one for easier understanding - the legacy reader carries a comment
above this loop explaining why the same-path remove/add pair is cancelled (the
DV update rewrites the entry for an unchanged data file). Would you mind
copying it here? Read in isolation, dropping a file from both maps looks like
it loses information.
##########
xtable-core/src/main/java/org/apache/xtable/kernel/DeltaKernelConversionSource.java:
##########
@@ -175,12 +201,14 @@ public TableChange getTableChangeForCommit(Long
versionNumber) {
public CommitsBacklog<Long> getCommitsBacklog(
InstantsForIncrementalSync instantsForIncrementalSync) {
Table table = Table.forPath(engine, basePath);
+ Snapshot latestSnapshot = table.getLatestSnapshot(engine);
+ dataFileExtractor.validateDeletionVectors(latestSnapshot, table, engine,
deletionVectorHandler);
Review Comment:
Same thought as on `DeltaConversionSource.validateActiveDeletionVectors`:
this is a full scan-file listing per incremental sync, and a
`Protocol.supportsFeature(TableFeatures.DELETION_VECTORS_RW_FEATURE)` check on
the latest snapshot would let tables without the feature skip it.
--
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]
