[Kernel] Added Domain Metadata support to Delta Kernel

[qiyuandong-db]

Which Delta project/connector is this regarding?

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

Description

This PR adds support for Domain Metadata to Delta Kernel as described in the Delta Protocal.

In particular, it enables Delta Kernel to handle domain metadata across transaction commit, checkpointing, log replay, and conflict resolution.

How was this patch tested?

Added tests covering all operations involving DomainMetadata in DomainMetadataSuite

Does this PR introduce any user-facing changes?

No.

[johanl-db]

Nice work, this is looking pretty good

[johanl-db]

This will consume the dataActions iterator, reading all actions at once to validate that there aren't duplicates, whereas the previous code wouldn't read from the iterator and just pass it to writeJsonFileAtomically

That means in particular that we read all actions from the iterator twice, which may not necessarily be safe and will likely impact performance.

As an alternative, you could try implementing the check as an iterator itself that will wrap around the dataActions iterator:

ValidateDomainMetadata implements Iterator {
   CloseableIterator<Row> actions;

  Row next() {
     action = actions.next()
     // Do checks, keep track of DomainMetadata actions seen
     return action;
  }
}

[qiyuandong-db]

This is a good idea! I’ve implemented this in my latest commits - please see ValidateDomainMetadataIterator

[johanl-db]

Should we just log an error and keep moving instead? Not sure what we do in the rest of kernel in such a case?

[qiyuandong-db]

This is what we do in the loadTableProtocolAndMetadata() function above.

In some cases, we also throw new UncheckedIOException(...) (e.g., in TransactionImpl::doCommit).

However, it seems we always thrown an exception in cases like this.

[scottsand-db]

Looks great! Thanks for this PR!

Left a few comments :p Mostly style. A few important ones about logic/functionality.

I did not require the tests, will do so after changes are made.

[scottsand-db]

cc @vkorukanti -- I feel like we need to come up with a cleaner template for Kernel Delta Table Features for them to all include common information, like the min reader writer version required, the feature name, etc.

[scottsand-db]

Wouldn't we want to parse the configuration to a Map<String, String>?

[qiyuandong-db]

Do you suggest we should also parse and validate the configuration string as a Map<String, String> at construction, in addition to storing a raw string? Currently I only use the raw string since this is how it is write to/read from log files and this is how it is implemented in DBR.

[scottsand-db]

1. On read path, it can be lazy.
2. On write path, we should be sure we only write valid domain metadata. Btw, where in the spec does it say the configuration has to be a Map<String, String> ?
3. Shouldn't we validate on the write path that the user hasn't written to a System-controlled metadata domain?

[scottsand-db]

we should do this during the P & M log replay

[qiyuandong-db]

According to our discussion, we will address this in a separate PR.

[scottsand-db]

I see. So this is just a thin wrapper over the actions-to-be-committed and validates the DomainMetadata on write.

@vkorukanti -- would we ever want to validate other things on write?

@qiyuandong-db -- I wonder if we should not have a ValidateDomainMetadataIterator but rather a ValidationIterator. This validation iterator lets you pass in different validationFunctions that take a row and decide to or not to throw an error. This would be a more extensible solution. It would let us avoid iterater wrappers on top of iterator wrappers on top of iterator wrappers ...

For example, I'd be fine removing the validation from this PR and coding that ^ up in a followup PR ... let's see what @vkorukanti thinks

[scottsand-db]

Requested some minor changes. Had one pretty big question about the validator iterator. The tests look great! Will stamp after this 🫡