Delta Kernel Draft PR #729

vaibhavk1992 · 2025-06-30T10:42:16Z

Important Read

Please ensure the GitHub issue is mentioned at the beginning of the PR

What is the purpose of the pull request

(For example: This pull request implements the sync for delta format.)

Brief change log

(for example:)

Fixed JSON parsing error when persisting state
Added unit tests for schema evolution

Verify this pull request

(Please pick either of the following options)

This pull request is a trivial rework / code cleanup without any test coverage.

(or)

This pull request is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

Added integration tests for end-to-end.
Added TestConversionController to verify the change.
Manually verified the change by running a job locally.

rahil-c · 2025-07-01T04:16:34Z

pom.xml

@@ -53,7 +53,7 @@
        <module>xtable-utilities</module>
        <module>xtable-aws</module>
        <module>xtable-hive-metastore</module>
-        <module>xtable-service</module>
+<!--        <module>xtable-service</module>-->


This should be added back, any reason why you had to comment this out?

yes, xtable-service was giving build failures. As xtable-service is independent of the delta kernel changes. Can we just review the delta kernel changes as of now? I just want to get my changes validated once and anyhow the final version of delta kernel changes would have xtable-service module too.

@vaibhavk1992 I think if you rebase with latest main branch you shouldn't see those failures.

rahil-c · 2025-07-01T06:39:07Z

xtable-core/pom.xml

+        <dependency>
+            <groupId>io.delta</groupId>
+            <artifactId>delta-kernel-api</artifactId>
+            <version>4.0.0</version>


Can you add a properly in the root pom called <delta.kernel.version>4.0.0</delta.kernel.version>, instead of using the hardcoded value?

Also curious how you ended up choosing delta kernel version, is there some specific version that needs to align with delta lake version we have in the repo?

rahil-c · 2025-07-01T06:40:42Z

xtable-core/src/main/java/org/apache/xtable/delta/DeltaKernelConversionSourceProvider.java

+public class DeltaKernelConversionSourceProvider extends ConversionSourceProvider<Long> {
+  @Override
+  public DeltaKernelConversionSource getConversionSourceInstance(SourceTable sourceTable) {
+    Configuration hadoopConf = new Configuration();


Is there any reason why you are creating a new hadoopConf, can you instead use the hadoopConf from the parent class similar to what DeltaConversionSourceProvider does.

rahil-c · 2025-07-01T06:42:04Z

xtable-core/src/main/java/org/apache/xtable/delta/DeltaKernelSchemaExtractor.java

+    return INSTANCE;
+  }
+
+  public InternalSchema toInternalSchema_v2(StructType structType) {


I think we can just call this toInternalSchema, since its in its own distinct class right?

rahil-c · 2025-07-01T06:50:48Z

xtable-core/src/main/java/org/apache/xtable/delta/DeltaKernelTableExtractor.java

+      // Get schema from Delta Kernel's snapshot
+      io.delta.kernel.types.StructType schema = snapshot.getSchema();
+
+      System.out.println("Kernelschema: " + schema);


[nit] well need to remove these in final version of the pr.

rahil-c · 2025-07-01T06:51:38Z

xtable-core/src/main/java/org/apache/xtable/delta/DeltaSchemaExtractor.java

- * Converts between Delta and InternalTable schemas. Some items to be aware of:
- *
- * <ul>
- *   <li>Delta schemas are represented as Spark StructTypes which do not have enums so the enum


I think we can live this file as is right?

rahil-c · 2025-07-01T06:53:48Z

xtable-core/src/main/java/org/apache/xtable/kernel/DeltaKernelConversionSource.java

+import org.apache.xtable.spi.extractor.ConversionSource;
+
+@Builder
+public class DeltaKernelConversionSource implements ConversionSource<Long> {


We will need full implementation of all the interface methods, otherwise this will fail during the table format sync. Can you refer to the impl for DeltaConversionSource for these methods?

xtable-core/src/main/java/org/apache/xtable/delta/DeltaKernelDataFileExtractor.java

vinishjail97 · 2025-06-30T15:13:35Z

pom.xml

@@ -53,7 +53,7 @@
        <module>xtable-utilities</module>
        <module>xtable-aws</module>
        <module>xtable-hive-metastore</module>
-        <module>xtable-service</module>
+<!--        <module>xtable-service</module>-->


why comment this?

vinishjail97 · 2025-07-28T14:51:12Z

xtable-core/src/test/resources/junit-platform.properties

@@ -14,6 +14,6 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 #
-junit.jupiter.execution.parallel.enabled=true
+junit.jupiter.execution.parallel.enabled=false


I have run the tests locally by setting this to true and they pass, can we revert this config and see the GH build?

vinishjail97

This is great progress @vaibhavk1992, added some comments.

vinishjail97 · 2025-07-28T14:53:01Z

pom.xml

@@ -53,7 +53,7 @@
        <module>xtable-utilities</module>
        <module>xtable-aws</module>
        <module>xtable-hive-metastore</module>
-        <module>xtable-service</module>
+<!--        <module>xtable-service</module>-->


@vaibhavk1992 I think if you rebase with latest main branch you shouldn't see those failures.

vinishjail97 · 2025-07-28T14:53:21Z

pom.xml

@@ -713,7 +713,7 @@
                </executions>
                <configuration>
                    <skip>${skipUTs}</skip>
-                    <redirectTestOutputToFile>true</redirectTestOutputToFile>


Why revert this?

vinishjail97 · 2025-07-28T14:54:01Z

xtable-utilities/src/test/resources/my_config.yaml

@@ -19,6 +19,6 @@ targetFormats:
  - DELTA
 datasets:
  -
-    tableBasePath:  /Desktop/opensource/iceberg/warehouse/demo/nyc/taxis
-    tableDataPath:  /Desktop/opensource/iceberg/warehouse/demo/nyc/taxis/data
+    tableBasePath:  /Users/vaibhakumar/Desktop/opensource/iceberg/warehouse/demo/nyc/taxis


This one can be reverted too?

vinishjail97 · 2025-07-28T14:58:25Z

xtable-core/src/main/java/org/apache/xtable/delta/DeltaKernelActionsConverter.java

+ * limitations under the License.
+ */
+
+package org.apache.xtable.delta;


Let's move new DeltaKernel* classes to org.apache.xtable.kernel?
Any reason for having kernel in a new package outside delta? We might rename to delta-kernel or something if we keep it outside.

…itions,big fixes

vaibhavk1992 added 6 commits May 14, 2025 10:43

single commit

dfea6a4

adding delta kernel

b75bc7c

adding the test file

16134b3

adding workable code for iteration over data

3929e95

adding Kernel 4.0 code

c6379b5

adding the working code with xtable that check getcurrenttable

6deb5f7

vaibhavk1992 marked this pull request as draft June 30, 2025 10:42

vaibhavk1992 marked this pull request as ready for review June 30, 2025 15:40

vaibhavk1992 added 2 commits June 30, 2025 21:13

Merge branch 'main' into test-4

05d9984

adding the dependecies

c7ba4b9

rahil-c reviewed Jul 1, 2025

View reviewed changes

vaibhavk1992 added 4 commits July 19, 2025 22:15

adding getcurrentsnapshot code

0ff36a5

spotless fix

18ab9d6

spotless fix 2

e906091

spotless fix 2

e00241c

the-other-tim-brown reviewed Jul 20, 2025

View reviewed changes

xtable-core/src/main/java/org/apache/xtable/delta/DeltaKernelDataFileExtractor.java Outdated Show resolved Hide resolved

xtable-core/src/main/java/org/apache/xtable/delta/DeltaKernelDataFileExtractor.java Show resolved Hide resolved

fixed partitioned test case

3fdfd31

vinishjail97 reviewed Jul 28, 2025

View reviewed changes

setting junit parallel execution to true

e0102e3

vinishjail97 reviewed Jul 28, 2025

View reviewed changes

vaibhavk1992 added 3 commits August 5, 2025 10:08

testInsertsUpsertsAndDeletes test case addition,internal datatype add…

381722a

…itions,big fixes

added the fix for table basepath listing wrong paths

809bfe8

added the fix for table basepath listing wrong paths

40172f2

Delta Kernel Draft PR #729

Are you sure you want to change the base?

Delta Kernel Draft PR #729

Uh oh!

Conversation

vaibhavk1992 commented Jun 30, 2025

Important Read

What is the purpose of the pull request

Brief change log

Verify this pull request

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rahil-c Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vinishjail97 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rahil-c Jul 1, 2025 •

edited

Loading