[Kernel] Add column mapping metadata update functionality #3393

nastra · 2024-07-18T12:13:17Z

Which Delta project/connector is this regarding?

Description

This adds column mapping metadata when the column mapping feature is enabled on a table and fixes #3383

How was this patch tested?

New tests have been added to verify the behavior

Does this PR introduce any user-facing changes?

raveeram-db · 2024-07-18T19:05:11Z

Thanks for the PR @nastra ! Change LGTM so far. cc: @scottsand-db

scottsand-db

Looks great! Left some comments!

kernel/kernel-api/src/main/java/io/delta/kernel/internal/TransactionBuilderImpl.java

kernel/kernel-api/src/main/java/io/delta/kernel/internal/TransactionImpl.java

kernel/kernel-api/src/main/java/io/delta/kernel/internal/util/ColumnMapping.java

kernel/kernel-defaults/src/test/scala/io/delta/kernel/defaults/DeltaTableWritesSuite.scala

scottsand-db · 2024-07-23T17:47:45Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/util/ColumnMapping.java

+    ////////////////////////////
+
+    /** Visible for testing */
+    static int findMaxColumnId(StructType schema) {


this doesn't handle nested schemas, right? are you planning on adding that in a subsequent PR?

I worry that adding it in a subsequent PR will leave us open to forgetting spots to add it ...

yeah I initially discussed this with @raveeram-db to add nested schema support in a separate PR. However, I can also add it as part of this PR if necessary

I've opened #3427 to handled nested schema

scottsand-db · 2024-07-23T17:52:14Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/util/ColumnMapping.java

+        if (field.getMetadata().contains(COLUMN_MAPPING_PHYSICAL_NAME_KEY)) {
+            return (String) field
+                    .getMetadata()
+                    .get(COLUMN_MAPPING_PHYSICAL_NAME_KEY);


did you create that issue about getString on FieldMetadata class? we can leave a TODO here

based on #3393 (comment) I was waiting for a response whether we should do it in the first place, so I haven't opened an issue yet

I've opened #3419 to address that

vkorukanti

Looks great! Thanks for working on this.

Qn: looks like the data writing path is not yet handled which means we can create tables with column mapping enabled, but the data written is not correct. Can we split out the physical name/id mapping info method and complete the implementation (i.e handling the nested and array/map types)?

vkorukanti · 2024-07-23T23:02:38Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/util/ColumnMapping.java

+    private static boolean allowMappingModeChange(String oldMode, String newMode) {
+        // only upgrade from none to name mapping is allowed
+        return oldMode.equals(newMode) ||
+                (COLUMN_MAPPING_MODE_NONE.equals(oldMode) &&


wondering if we should just define an enum and let the TableConfig return the enum value. Otherwise we need to deal with the case sensitive issues.

Let me know, I can make a separate PR to change it to use the enum.

would that enum be in ColumnMapping or in TableConfig? No worries, I can add that enum. However, that feels like it would be better to do in a follow-up PR in order to not increase the diff here, wdyt?

enum in either places is fine.

vkorukanti · 2024-07-24T06:12:27Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/util/ColumnMapping.java

+    private static Metadata assignColumnIdAndPhysicalName(Metadata metadata, boolean isNewTable) {
+        StructType schema = metadata.getSchema();
+        int maxColumnId = Math.max(
+                Integer.parseInt(metadata.getConfiguration()


Can we add the MAX_ID property to TableConfig and use it to parse? btw Delta-Spark is parsing it as Long.

do you know why delta-spark actually parses this as Long instead of an Int? Schema Ids should be ints

the only reason I haven't added the MAX_COLUMN_ID to TableConfig because it's an internal table property that should be set by users and TableConfig seems to be made for properties that can be explicitly set by users.
I think TableConfig would need a concept of whether a property should be settable by a user or not, wdyt?

I agree, we need a property in the TableConfig to say it is changeable by the use or not. It can be a separate PR.

vkorukanti · 2024-07-24T06:13:26Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/util/ColumnMapping.java

+    ////////////////////////////
+
+    /** Visible for testing */
+    static int findMaxColumnId(StructType schema) {


kernel/kernel-api/src/main/java/io/delta/kernel/internal/util/ColumnMapping.java

vkorukanti · 2024-07-24T06:21:08Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/util/ColumnMapping.java

+                        .putString(COLUMN_MAPPING_PHYSICAL_NAME_KEY, physicalName)
+                        .build());
+            }
+            newSchema = newSchema.add(updatedField);


shouldn't we need to handle the nested types and array/map elements?

kernel/kernel-api/src/main/java/io/delta/kernel/internal/util/ColumnMapping.java

nastra · 2024-07-24T11:02:37Z

Looks great! Thanks for working on this.

Qn: looks like the data writing path is not yet handled which means we can create tables with column mapping enabled, but the data written is not correct. Can we split out the physical name/id mapping info method and complete the implementation (i.e handling the nested and array/map types)?

@vkorukanti are you referring to what's being handled as part of #3384 w.r.t the data write path?

vkorukanti

Two minor follow ups and in addition to the data path change. This PR LGTM.

vkorukanti · 2024-07-29T15:49:55Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/util/ColumnMapping.java

+    private static boolean allowMappingModeChange(String oldMode, String newMode) {
+        // only upgrade from none to name mapping is allowed
+        return oldMode.equals(newMode) ||
+                (COLUMN_MAPPING_MODE_NONE.equals(oldMode) &&


enum in either places is fine.

vkorukanti · 2024-07-29T15:50:36Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/util/ColumnMapping.java

+    private static Metadata assignColumnIdAndPhysicalName(Metadata metadata, boolean isNewTable) {
+        StructType schema = metadata.getSchema();
+        int maxColumnId = Math.max(
+                Integer.parseInt(metadata.getConfiguration()


I agree, we need a property in the TableConfig to say it is changeable by the use or not. It can be a separate PR.

vkorukanti · 2024-07-29T15:53:24Z

Looks great! Thanks for working on this.
Qn: looks like the data writing path is not yet handled which means we can create tables with column mapping enabled, but the data written is not correct. Can we split out the physical name/id mapping info method and complete the implementation (i.e handling the nested and array/map types)?

@vkorukanti are you referring to what's being handled as part of #3384 w.r.t the data write path?

Yes, that's the issue.

nastra force-pushed the column-mapping-meta-update branch 7 times, most recently from 0f5ac79 to 3169b2e Compare July 18, 2024 14:53

vkorukanti self-requested a review July 18, 2024 16:07

scottsand-db requested changes Jul 18, 2024

View reviewed changes

nastra force-pushed the column-mapping-meta-update branch 3 times, most recently from 7df2b70 to bcd839a Compare July 19, 2024 08:33

nastra requested a review from scottsand-db July 19, 2024 10:34

nastra force-pushed the column-mapping-meta-update branch 4 times, most recently from f6ba330 to 599bd06 Compare July 22, 2024 06:36

scottsand-db reviewed Jul 23, 2024

View reviewed changes

vkorukanti reviewed Jul 24, 2024

View reviewed changes

kernel/kernel-api/src/main/java/io/delta/kernel/internal/util/ColumnMapping.java Outdated Show resolved Hide resolved

nastra force-pushed the column-mapping-meta-update branch from 599bd06 to be1dc76 Compare July 24, 2024 14:57

[Kernel] Add column mapping metadata update functionality

620389a

nastra force-pushed the column-mapping-meta-update branch from be1dc76 to bb55534 Compare July 24, 2024 15:13

review feedback

2652313

nastra force-pushed the column-mapping-meta-update branch from bb55534 to 2652313 Compare July 24, 2024 15:15

nastra mentioned this pull request Jul 26, 2024

[Feature Request][Kernel] Handle nested schemas when assigning id / physical name for column mapping mode #3427

Closed

8 tasks

nastra requested review from scottsand-db and vkorukanti July 26, 2024 17:54

vkorukanti approved these changes Jul 29, 2024

View reviewed changes

vkorukanti merged commit ec3f6be into delta-io:master Jul 29, 2024
10 checks passed

nastra deleted the column-mapping-meta-update branch July 29, 2024 18:19

vkorukanti mentioned this pull request Aug 5, 2024

[WIP][Kernel] Support for writing into column mapping enabled tables #3475

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Kernel] Add column mapping metadata update functionality #3393

[Kernel] Add column mapping metadata update functionality #3393

nastra commented Jul 18, 2024 •

edited

Loading

raveeram-db commented Jul 18, 2024

scottsand-db left a comment

scottsand-db Jul 23, 2024

vkorukanti Jul 24, 2024

nastra Jul 24, 2024 •

edited

Loading

nastra Jul 26, 2024

scottsand-db Jul 23, 2024

nastra Jul 24, 2024 •

edited

Loading

nastra Jul 24, 2024

vkorukanti left a comment

vkorukanti Jul 23, 2024

vkorukanti Jul 23, 2024

nastra Jul 24, 2024

vkorukanti Jul 29, 2024

vkorukanti Jul 24, 2024

nastra Jul 24, 2024

nastra Jul 24, 2024

vkorukanti Jul 29, 2024

vkorukanti Jul 24, 2024

vkorukanti Jul 24, 2024

nastra commented Jul 24, 2024

vkorukanti left a comment •

edited

Loading

vkorukanti Jul 29, 2024

vkorukanti Jul 29, 2024

vkorukanti commented Jul 29, 2024

[Kernel] Add column mapping metadata update functionality #3393

[Kernel] Add column mapping metadata update functionality #3393

Conversation

nastra commented Jul 18, 2024 • edited Loading

Which Delta project/connector is this regarding?

Description

How was this patch tested?

Does this PR introduce any user-facing changes?

raveeram-db commented Jul 18, 2024

scottsand-db left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nastra Jul 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nastra Jul 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vkorukanti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nastra commented Jul 24, 2024

vkorukanti left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vkorukanti commented Jul 29, 2024

nastra commented Jul 18, 2024 •

edited

Loading

nastra Jul 24, 2024 •

edited

Loading

nastra Jul 24, 2024 •

edited

Loading

vkorukanti left a comment •

edited

Loading