[Kernel][icebergWriterCompatV1] Add a check that map struct keys don't evolve

[nicklan]

Which Delta project/connector is this regarding?

• Spark
• Standalone
• Flink
• Kernel
• Other (fill in here)

Description

Add a check to forbid changes in struct types that are map keys. See docs which state:

Altering a map 'key' column by adding columns is not allowed. Only map values can be updated.

We have empirically determined that drops also trigger an error, so we forbid any changes to structs that are used as map keys.

This check only triggers if IcebergWriterCompatV1 is enabled on the table. This is required by the spec (see here)

How was this patch tested?

Unit Tests

Does this PR introduce any user-facing changes?

No

[allisonport-db]

I wonder if this check should be nested deeper within the schema evolution checks? Then we can use the SchemaChanges class @amogh-jahagirdar added to just check the "updatedColumns"

But I'm also okay with this if it's not any easier/better. Maybe it's fewer schema traversals however? Since I think this method alone involves at least 2

[nicklan]

Actually that's much nicer, thanks for the suggestion I didn't see the SchemaChanges stuff

[allisonport-db]

I was actually picturing nesting this even further in the existing code, I think it can just be an extra check in validateFieldCompatibility. There we already have a branch where we have established a field update where we go from map type -> map type (skips a lot of this code here) and then all we need to add there is the additional check if icebergWriterCompatV1 is enabled & keyType is struct

[allisonport-db]

I think this should be super concise and simple there just something like

    } else if (existingField.getDataType() instanceof MapType
        && newField.getDataType() instanceof MapType) {
      MapType existingMapType = (MapType) existingField.getDataType();
      MapType newMapType = (MapType) newField.getDataType();

      if (icebergWriterCompatV1Enabled && existingMapType.getKeyType() instanceof StructType && newMapType.getKeyType() instanceof StructType) {
               // Require existingMapType.getKeyType() equals newMapType.getKeyType()
      }

      validateFieldCompatibility(existingMapType.getKeyField(), newMapType.getKeyField());
      validateFieldCompatibility(existingMapType.getValueField(), newMapType.getValueField());
    }

[nicklan]

Ohh man, can't believe I missed that those checks already exist. Thanks! That's obviously much better.

[allisonport-db]

I was actually picturing nesting this even further in the existing code, I think it can just be an extra check in validateFieldCompatibility. There we already have a branch where we have established a field update where we go from map type -> map type (skips a lot of this code here) and then all we need to add there is the additional check if icebergWriterCompatV1 is enabled & keyType is struct

[allisonport-db]

I think this should be super concise and simple there just something like

    } else if (existingField.getDataType() instanceof MapType
        && newField.getDataType() instanceof MapType) {
      MapType existingMapType = (MapType) existingField.getDataType();
      MapType newMapType = (MapType) newField.getDataType();

      if (icebergWriterCompatV1Enabled && existingMapType.getKeyType() instanceof StructType && newMapType.getKeyType() instanceof StructType) {
               // Require existingMapType.getKeyType() equals newMapType.getKeyType()
      }

      validateFieldCompatibility(existingMapType.getKeyField(), newMapType.getKeyField());
      validateFieldCompatibility(existingMapType.getValueField(), newMapType.getValueField());
    }

[allisonport-db]

LGTM