[Kernel] Add ability to store type changes on StructField #4519

emkornfield · 2025-05-09T00:20:03Z

Which Delta project/connector is this regarding?

Description

This change adds a new TypeChange class and the ability store a list of type changes on FieldStruct. This will allow for persisting/updating type changes (and is modelled similar to how collation works due to the complexity of working with maps/arrays).

How was this patch tested?

Add unit tests.

Does this PR introduce any user-facing changes?

This is a API change only.

emkornfield · 2025-05-09T19:52:24Z

kernel/kernel-api/src/test/scala/io/delta/kernel/types/StructFieldSuite.scala

@@ -0,0 +1,176 @@
+/*
+ * Copyright (2023) The Delta Lake Project Authors.


Suggested change

* Copyright (2023) The Delta Lake Project Authors.

* Copyright (2025) The Delta Lake Project Authors.

vkorukanti

Mostly qns

vkorukanti · 2025-05-13T17:16:25Z

kernel/kernel-api/src/main/java/io/delta/kernel/types/StructField.java

+   * where the metadata is attached depend on if the change is for nested arrays/maps or primitive
+   * types.
+   */
+  public static class TypeChange {


may be pull this out, given this is going to be a public API

vkorukanti · 2025-05-13T17:16:56Z

kernel/kernel-api/src/main/java/io/delta/kernel/types/StructField.java

+      DataType dataType,
+      boolean nullable,
+      FieldMetadata metadata,
+      List<TypeChange> typeChanges) {


hmm.. we need a builder for StructField soon. (not related to this PR)

vkorukanti · 2025-05-13T17:17:52Z

kernel/kernel-api/src/main/java/io/delta/kernel/types/StructField.java

@@ -95,6 +162,11 @@ public boolean isNullable() {
    return nullable;
  }

+  /** @return the list of type changes for this field */


Is it a list because the filed can undergo more than one type change? Also is the last one the latest one?

yes, updated docs.

vkorukanti · 2025-05-13T17:23:12Z

kernel/kernel-api/src/main/java/io/delta/kernel/types/StructField.java


    FieldMetadata collationMetadata = fetchCollationMetadata();
    this.metadata =
        new FieldMetadata.Builder().fromMetadata(metadata).fromMetadata(collationMetadata).build();
+    if (!this.typeChanges.isEmpty()


if the map contains a struct either as key or value, is the type change allowed in this case?

Yes, the idea of this assert is it enforces types changes are only recorded on leaf nodes of the schema. We could also assert this in the other way I think that DataType is a primitive type.

I think this is a core design question that we should answer which option we want to pursue here.

By adding these to leaf nodes in the schema we have to add code similar to collations that will construct appropriate FieldMetadata on construction (and do the inverse operation for when parsing) for nested Map/Array types.

The other option would be not add the field at all and solely work with TypeChanges are a function of FieldMetadata (which would require less book-keeping but might be more error prone in the long run. This is how field assignment works for icebergCompatV2

I think the question is which pattern we want to follow (and then migrate all of these common elements to the common pattern).

@allisonport-db this is somewhat related to the bug on populating nested field IDs, not sure if you have thoughts on which approach you prefer.

I am a little confused because I think this is the level where we would want the type changes for map/array? From the protocol https://github.com/delta-io/delta/blob/master/PROTOCOL.md#type-change-metadata

We wouldn't want to support TypeChanges on their element or key/map types though.

I think I maybe understand what you're saying however, we could choose to store them at either level and then serialize them at this level?

I honestly find the use of StructField for the key/value/element really confusing since there is no such thing as a key's fieldMetadata (it does not exist!). For that reason, I'd prefer to store it here at this level. And honestly I've been meaning to take a look at that code and see if there's a better way to make sure there isn't confusing when using StructField in those data types.

But then we need to add the additional "fieldPath" to the TypeChange class right?

Also feel free to disagree, I think choosing where to store this info (and other stuff like the nested ids) is an important discussion to be had

scottsand-db · 2025-05-15T16:16:48Z

kernel/kernel-api/src/main/java/io/delta/kernel/types/StructField.java

    this.name = name;
    this.dataType = dataType;
    this.nullable = nullable;
+    this.typeChanges = typeChanges == null ? Collections.emptyList() : typeChanges;


IMO we should discourage null ever being passed in. I agree this is a safe check but ... would be nice to never have to think about null?

+1 probably can either (1) requireNonNull if we want this to always be present but empty for no type widening or (2) make it optional

allisonport-db

Sorry I was behind on my reviews, left a few comments mostly about the nesting levels for array/map

allisonport-db · 2025-05-17T00:18:05Z

kernel/kernel-api/src/main/java/io/delta/kernel/types/StructField.java

    this.name = name;
    this.dataType = dataType;
    this.nullable = nullable;
+    this.typeChanges = typeChanges == null ? Collections.emptyList() : typeChanges;


+1 probably can either (1) requireNonNull if we want this to always be present but empty for no type widening or (2) make it optional

allisonport-db · 2025-05-17T00:19:59Z

kernel/kernel-api/src/main/java/io/delta/kernel/types/StructField.java


    FieldMetadata collationMetadata = fetchCollationMetadata();
    this.metadata =
        new FieldMetadata.Builder().fromMetadata(metadata).fromMetadata(collationMetadata).build();
+    if (!this.typeChanges.isEmpty()


I am a little confused because I think this is the level where we would want the type changes for map/array? From the protocol https://github.com/delta-io/delta/blob/master/PROTOCOL.md#type-change-metadata

allisonport-db · 2025-05-17T00:21:46Z

kernel/kernel-api/src/main/java/io/delta/kernel/types/StructField.java


    FieldMetadata collationMetadata = fetchCollationMetadata();
    this.metadata =
        new FieldMetadata.Builder().fromMetadata(metadata).fromMetadata(collationMetadata).build();
+    if (!this.typeChanges.isEmpty()


We wouldn't want to support TypeChanges on their element or key/map types though.

I think I maybe understand what you're saying however, we could choose to store them at either level and then serialize them at this level?

I honestly find the use of StructField for the key/value/element really confusing since there is no such thing as a key's fieldMetadata (it does not exist!). For that reason, I'd prefer to store it here at this level. And honestly I've been meaning to take a look at that code and see if there's a better way to make sure there isn't confusing when using StructField in those data types.

allisonport-db · 2025-05-17T00:23:35Z

kernel/kernel-api/src/main/java/io/delta/kernel/types/StructField.java


    FieldMetadata collationMetadata = fetchCollationMetadata();
    this.metadata =
        new FieldMetadata.Builder().fromMetadata(metadata).fromMetadata(collationMetadata).build();
+    if (!this.typeChanges.isEmpty()


But then we need to add the additional "fieldPath" to the TypeChange class right?

allisonport-db · 2025-05-17T00:28:41Z

kernel/kernel-api/src/main/java/io/delta/kernel/types/StructField.java

Oh I guess probably a very important question, how do we expect connectors to communicate type widening schema changes?

(1) We expect them to add the full metadata with the TypeChanges class to the schema?
(2) They just provide a schema with the new type and behind the scenes we add this TypeChanges metadata
- If this one, it seems like once again we come back to this being another good place where it would be preferred to have an internal vs external interface

Maybe we want to support both?

emkornfield added 2 commits May 8, 2025 23:59

[Kernel] Add field to StructField to track type changes.

313f4b2

add validation

1b2676b

emkornfield assigned vkorukanti May 9, 2025

emkornfield mentioned this pull request May 9, 2025

[WIP][Kernel][RFC] Add type change history to StructField #4512

Closed

5 tasks

emkornfield added 2 commits May 9, 2025 17:04

fix test

fff4355

break long lines

2f32731

emkornfield requested a review from vkorukanti May 9, 2025 19:24

emkornfield assigned emkornfield and unassigned vkorukanti May 9, 2025

emkornfield commented May 9, 2025

View reviewed changes

vkorukanti requested changes May 13, 2025

View reviewed changes

Address comments

89eadb6

emkornfield requested a review from vkorukanti May 13, 2025 20:46

emkornfield added 2 commits May 14, 2025 23:42

wrap in code

3f3ba31

fixup

0c067e2

vkorukanti approved these changes May 14, 2025

View reviewed changes

scottsand-db reviewed May 15, 2025

View reviewed changes

scottsand-db approved these changes May 15, 2025

View reviewed changes

scottsand-db merged commit 2dd5a37 into delta-io:master May 15, 2025
22 checks passed

allisonport-db reviewed May 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Kernel] Add ability to store type changes on StructField #4519

[Kernel] Add ability to store type changes on StructField #4519

emkornfield commented May 9, 2025

emkornfield May 9, 2025

vkorukanti left a comment

vkorukanti May 13, 2025

emkornfield May 13, 2025

vkorukanti May 13, 2025

emkornfield May 13, 2025

vkorukanti May 13, 2025

emkornfield May 13, 2025

vkorukanti May 13, 2025

emkornfield May 13, 2025

emkornfield May 13, 2025

emkornfield May 13, 2025

allisonport-db May 17, 2025

allisonport-db May 17, 2025

allisonport-db May 17, 2025

allisonport-db May 17, 2025

scottsand-db May 15, 2025

allisonport-db May 17, 2025

allisonport-db left a comment

allisonport-db May 17, 2025

allisonport-db May 17, 2025

allisonport-db May 17, 2025

allisonport-db May 17, 2025

allisonport-db May 17, 2025

		@@ -0,0 +1,176 @@
		/*
		* Copyright (2023) The Delta Lake Project Authors.

	* Copyright (2023) The Delta Lake Project Authors.
	* Copyright (2025) The Delta Lake Project Authors.

[Kernel] Add ability to store type changes on StructField #4519

[Kernel] Add ability to store type changes on StructField #4519

Conversation

emkornfield commented May 9, 2025

Which Delta project/connector is this regarding?

Description

How was this patch tested?

Does this PR introduce any user-facing changes?

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

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

allisonport-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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment