vinishjail97 commented on code in PR #938:
URL: https://github.com/apache/incubator-xtable/pull/938#discussion_r4052461210
##########
xtable-core/src/main/java/org/apache/xtable/delta/DeltaConversionSource.java:
##########
@@ -215,6 +224,22 @@ public CommitsBacklog<Long> getCommitsBacklog(
.build();
}
+ @VisibleForTesting
+ void validateActiveDeletionVectors(Snapshot snapshot) {
+ if (!snapshot.deletionVectorsSupported()) {
Review Comment:
When the override is enabled this scan cannot fail the sync, so all it does
is emit one warning per file carrying a deletion vector, and it still runs the
`allFiles()` Spark job on every incremental sync. Would it be reasonable to
gate on the override as well, say an `isRejecting()` accessor on
`DeltaDeletionVectorHandler` next to `deletionVectorsSupported()`, so
permissive tables skip the listing and the per-commit path carries the warning?
The same applies to
`DeltaKernelConversionSource.validateActiveDeletionVectors`. Non-blocking.
##########
xtable-core/src/test/java/org/apache/xtable/delta/ITDeltaDeleteVectorConvert.java:
##########
@@ -182,6 +248,48 @@ public void testInsertsUpsertsAndDeletes() {
ValidationTestHelper.validateTableChanges(allActiveFiles, allTableChanges);
}
+ private void assertRejectsDeletionVectors(
Review Comment:
Both call sites pass `timestampAfterDelete` here, so the backlog is empty
and the rejection has to come from the active-snapshot check rather than from
`getTableChangeForCommit`. That is the case the protocol gate could regress,
and the parameter name currently says the opposite.
```suggestion
private void assertRejectsDeletionVectors(
ConversionSource<Long> conversionSource, Long lastSyncTimestamp) {
```
##########
xtable-core/src/main/java/org/apache/xtable/delta/DeltaDataFileExtractor.java:
##########
@@ -45,13 +45,11 @@ public class DeltaDataFileExtractor {
@Builder.Default
private final DeltaActionsConverter actionsConverter =
DeltaActionsConverter.getInstance();
- /**
- * Initializes an iterator for Delta Lake files.
- *
- * @return Delta table file iterator
- */
- public DataFileIterator iterator(Snapshot deltaSnapshot, InternalSchema
schema) {
- return new DeltaDataFileIterator(deltaSnapshot, schema, true);
+ public DataFileIterator iterator(
Review Comment:
Nit: the Javadoc on this public method was dropped in the rename. Could it
come back, with a line on what the handler argument does?
##########
xtable-utilities/src/main/java/org/apache/xtable/utilities/StrictBooleanStringDeserializer.java:
##########
@@ -0,0 +1,50 @@
+/*
+ * 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 java.util.Locale;
+
+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;
+import com.fasterxml.jackson.databind.exc.InvalidFormatException;
+
+/** Deserializes boolean literals as canonical strings and rejects all other
values. */
+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;
+ }
+ String value = parser.getText();
+ if (Boolean.TRUE.toString().equalsIgnoreCase(value)
+ || Boolean.FALSE.toString().equalsIgnoreCase(value)) {
+ return value.toLowerCase(Locale.ROOT);
+ }
+ throw InvalidFormatException.from(
+ parser, "Expected true or false for allowUnsupportedDeletionVectors",
value, String.class);
Review Comment:
Nit: the class name is generic and it is now wired to two config classes,
but the message names one field. `parser.currentName()` would keep the message
correct if it is reused for another flag later.
##########
xtable-core/src/main/java/org/apache/xtable/kernel/DeltaKernelConversionSource.java:
##########
@@ -159,6 +177,22 @@ public TableChange getTableChangeForCommit(Long
versionNumber) {
}
}
+ // In Delta Lake if delete vector information is added for an existing
data file, as a result of
+ // a delete operation, then a new RemoveFile action is added to the commit
log to remove the old
+ // entry which is replaced by a new entry, AddFile with delete vector
information. Since the
+ // same data file is removed and added, we need to remove it from the
added and removed file
+ // maps which are used to track actual added and removed data files.
+ for (String dataFilePath : dataFilesWithDeletionVectors) {
Review Comment:
This block and its comment are identical to the one in
`DeltaConversionSource.getTableChangeForCommit`, and they touch only
`Map<String, InternalDataFile>` and `Set<String>`, so nothing in them is
Kernel- or Spark-specific. The two copies have already drifted, since the
legacy one keeps a `// validate that a Remove action is also added for the data
file` line that is missing here.
Would it be worth lifting it into a static helper next to
`DeltaDeletionVectorHandler`, which this class already imports? Non-blocking.
--
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]
