[Kernel] SnapshotStatistics and new Snapshot write-crc-file APIs #5340

scottsand-db · 2025-10-14T23:18:02Z

🥞 Stacked PR

Use this link to review incremental changes.

stack/kernel_snapshot_crc_statistics_and_write [Files changed]

Which Delta project/connector is this regarding?

Description

This PR adds:

Snapshot::getStatistics and SnapshotStatistics APIs
ChecksumWriteMode API
Snapshot::writeChecksum(Engine, ChecksumWriteMode) API
Updates tests

This PR enables the e2e flow of:

connector performs a write
connector asks for the post commit snapshot
connector can look at the statistics and see what is the available checksum write mode
for a post-commit snapshot it will be either SIMPLE or FULL (no one else should have written the crc yet)
the connector can decide if it wants to pay the cost of the SIMPLE or FULL crc write, and then invoke Snapshot::writeChecksum with the mode they are willing to pay

How was this patch tested?

New UTs. Updated existing write-with-crc-suites to run using this new logic, too.

Does this PR introduce any user-facing changes?

New Snapshot APIs.

nicklan

Generally looks great, thanks!

Possible other API:

writeChecksum(Engine engine, ChecksumWriteMode mode)

Now the connector if it doesn't really care can just do:

writeChecksum(engine, statistics.getChecksumWriteMode())

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

scottsand-db · 2025-10-15T23:53:52Z

@nicklan -- SG -- will implement writeChecksum(Engine engine, ChecksumWriteMode mode)

scottsand-db · 2025-10-16T16:06:20Z

kernel/kernel-defaults/src/test/scala/io/delta/kernel/defaults/utils/TestUtils.scala

        crcInfo.getFileSizeHistogram))
  }

  def executeCrcSimple(result: TransactionCommitResult, engine: Engine): TransactionCommitResult = {


The semantic this method wanted was: write the CRC file via simple mode. Instead, the semantic it was implementing was: write the CRC file if the SimpleCrcHook was present. That was incorrect.

…ies with retries

nicklan

Thanks, lgtm!

nicklan · 2025-10-16T23:35:07Z

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

+        logger.info("Skipping writing checksum file: input mode was NONE");
+        return;
+      case SIMPLE:
+        final CRCInfo crcInfo =


if actual == SIMPLE is this guaranteed to be true? i.e. can we replace this check with that one?

Yes if actual == SIMPLE then logReplay.getCrcInfoAtSnapshotVersion() is defined.

Would you like something like the below?

if (actual != SIMPLE) throw new IllegalStateException(...) final CRCInfo crcInfo = logReplay.getCrcInfoAtSnapshotVersion().get()

Using .getOrElse is just a bit more java idiomatic and a bit safer, but I'm okay if you want the above

Actually -- I think I liked my checks/logic from a few commits ago:

We should just check if the file already exists. If so, that's a no-op and we should logger.warn and exit early.

I think this is best:

public void writeChecksum(Engine engine, Snapshot.ChecksumWriteMode mode) throws IOException { final Snapshot.ChecksumWriteMode actual = getStatistics().getChecksumWriteMode(); switch (mode) { case NONE: logger.info("Skipping writing checksum file: input mode was NONE"); case SIMPLE: if (actual == ChecksumWriteMode.NONE) { logger.warn("Not writing checksum in SIMPLE mode: checksum file already exists"); return; } if (actual == ChecksumWriteMode.FULL) { throw new IllegalStateException( "Cannot write checksum in SIMPLE mode: FULL mode required"); } final CRCInfo crcInfo = logReplay.getCrcInfoAtSnapshotVersion().get(); logger.info("Executing checksum write in SIMPLE mode"); new ChecksumWriter(logPath).writeCheckSum(engine, crcInfo); case FULL: if (actual == ChecksumWriteMode.NONE) { logger.warn("Not writing checksum as FULL: checksum file already exists"); return; } if (actual == ChecksumWriteMode.SIMPLE) { logger.warn("Checksum SIMPLE mode was available, but FULL mode was requested"); } logger.info("Executing checksum write in FULL mode"); ChecksumUtils.computeStateAndWriteChecksum(engine, getLogSegment()); default: throw new IllegalStateException("Unknown checksum write mode: " + mode); } }

scottsand-db · 2025-10-17T00:58:19Z

@nicklan -- One consequence of void writeChecksum(Engine engine, Snapshot.ChecksumWriteMode mode) taking in the mode is that the mode can be NONE. It's a bit weird to take in a request to not write a checksum. Is that really better than two public APIs?

Another idea: Snapshot::writeChecksum(engine) and it decides internally what to do. you can poke at the statistics before hand to learn if it would be a FULL or SIMPLE write, or NONE if the file already exists?

The updated (local) code for me is below:

  public void writeChecksum(Engine engine, Snapshot.ChecksumWriteMode mode) throws IOException {
    final Snapshot.ChecksumWriteMode actual = getStatistics().getChecksumWriteMode();

    switch (mode) {
      case NONE:
        logger.info("Skipping writing checksum file: input mode was NONE");
        if (actual != ChecksumWriteMode.NONE) {
          logger.warn("Note that the checksum file does NOT actually exist");
        }
        return;
      case SIMPLE:
        if (actual == ChecksumWriteMode.NONE) {
          logger.warn("Not writing checksum in SIMPLE mode: checksum file already exists");
          return;
        }
        if (actual == ChecksumWriteMode.FULL) {
          throw new IllegalStateException(
              "Cannot write checksum in SIMPLE mode: FULL mode required");
        }

        final CRCInfo crcInfo = logReplay.getCrcInfoAtSnapshotVersion().get();
        logger.info("Executing checksum write in SIMPLE mode");
        new ChecksumWriter(logPath).writeCheckSum(engine, crcInfo);
        return;
      case FULL:
        if (actual == ChecksumWriteMode.NONE) {
          logger.warn("Not writing checksum as FULL mode: checksum file already exists");
          return;
        }
        if (actual == ChecksumWriteMode.SIMPLE) {
          logger.warn("Requested checksum write in FULL mode, but SIMPLE mode is available");
        }
        logger.info("Executing checksum write in FULL mode");
        ChecksumUtils.computeStateAndWriteChecksum(engine, getLogSegment());
        return;
      default:
        throw new IllegalStateException("Unknown checksum write mode: " + mode);
    }
  }

allisonport-db

Mostly just clarifying questions. Tests look great! Feeling a little confused today for some reason 😐

allisonport-db · 2025-10-17T00:18:06Z

kernel/kernel-api/src/main/java/io/delta/kernel/Snapshot.java

+   *   <li><b>FULL:</b> Computes the necessary state if needed by replaying the delta log since the
+   *       latest checksum (if present). This always succeeds but may be expensive for large tables
+   *       when CRC information is not available.


Nit - maybe rephrase this? "This always succeeds" sounds like it cannot fail (I assume it can!). I think this is instead trying to convey that regardless of what the Snapshot has in memory, this will try to execute the crc write.

Hmm, actually, why does this have a mode input at all? Can this not just look internally at what the mode is?

Maybe, is this just to make it explicit opt-in for the more expensive option?

Hmm, actually, why does this have a mode input at all? Can this not just look internally at what the mode is?

Agreed -- if you look at some of the PR comments, I'm thinking along these lines, too.

#5340 (review)

#5340 (comment)

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

allisonport-db · 2025-10-17T00:36:38Z

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

+            logReplay
+                .getCrcInfoAtSnapshotVersion()


Just curious, do you know why we store this here? Had to do some digging to understand how/when we populate this

This has to do with our LogReplay / ProtocolMetadataReplay / DomainMetadata Replay logic.

Fundamentally, the Snapshot should be injected with CRC info (optional) and then use that as needed to defer to static log replay utilities (e.g. for domain metadata).

Instead today, we have some tech debt where Snapshot is injected with a LogReplay, and that LogReplay has the crc info

kernel/kernel-api/src/main/java/io/delta/kernel/internal/replay/LogReplay.java

allisonport-db · 2025-10-17T01:16:59Z

...el/kernel-defaults/src/test/scala/io/delta/kernel/defaults/DeltaTableWriteWithCrcSuite.scala

+ * SIMPLE and uses the post-commit snapshot's writeChecksumSimple method. Note, this requires the
+ * test suite uses [[commitTransaction]] and [[verifyWrittenContent]].
+ */
+trait WriteUtilsWithPostCommitSnapshotCrcSimpleWrite extends AnyFunSuite with WriteUtils {


Ideally we should factor this out like we did with TransactionBuilderSupport :( Not a blocker though

Can you elaborate, sorry?

I think we talked about this in depth before for other test scenarios. But ideally we have abstract definitions for these fxs and then implement them in child traits, instead of just overriding them.

So like TransactionCommitSupport defines these abstractly, and then we can have like BasicTransactionCommitSupport and PostCommitSnapshotCRCCommitSupport etc for each different variation

Makes sense -- that would let us use mixins to inject specific funcitonality -- e.g. write crc, write checkpoint, publish, etc.

...faults/src/test/scala/io/delta/kernel/defaults/SnapshotChecksumStatisticsAndWriteSuite.scala

allisonport-db · 2025-10-17T01:22:59Z

...faults/src/test/scala/io/delta/kernel/defaults/SnapshotChecksumStatisticsAndWriteSuite.scala

+      // Create version 2 without writing its CRC
+      val snapshot1 = TableManager.loadSnapshot(tablePath).build(engine)
+      val txn2 = snapshot1.buildUpdateTableTransaction("xx", Operation.WRITE).build(engine)
+      txn2.commit(engine, emptyIterable())


sorry, why do we have to do this? won't this test check the same thing if we remove this part?

This just tests: writes a few versions (0, 1, 2) and then time travel to version 1 and then we assert that writing the CRC at version 1 is FULL.

So -- if you're asking why do we write v2 -- that just felt like a reasonable thing to test. The alternative is we write versions 0 and 1, and then we time travel to the historical version 0. Could absolutely do that, too. I thought that having a few more versions and time travelling to a middle one was a bit more common.

sorry, why is "time-travel" the important part? If you just load write versions (0, 1) and then load version (1) (not via post-commit snapshot) isn't that the same scenario? or I think maybe that was already tested?

So maybe the answer is = under the hood, this tests the same thing that's already tested, but the time-travel scenario semantically makes sense as a separate test

The alternative is we write versions 0 and 1, and then we time travel to the historical version 0.

Not suggesting this! This scenario makes sense if we are trying to test time-travel, just didn't understand why it was really a distinct different for the underlying implementation

I want to test a historical version (not the latest). All of the other tests are checking that we can write the CRC at the latest version (either loading it explicitly or using the post commit snapshot).

scottsand-db · 2025-10-17T16:11:42Z

@nicklan I think another option is: Snapshot::getStatistics::getChecksumWriteMode returns Optional<ChecksumWriteMode> which is either SIMPLE or FULL. There is no NONE. That's what Optional.empty is for.

Thus, Snapshot::writeChecksum takes in a SnapshotWriteMode (not optional), which is then of course only SIMPLE or FULL. Much simpler. WDYT?

nicklan · 2025-10-17T17:59:31Z

@nicklan I think another option is: Snapshot::getStatistics::getChecksumWriteMode returns Optional<ChecksumWriteMode> which is either SIMPLE or FULL. There is no NONE. That's what Optional.empty is for.

Thus, Snapshot::writeChecksum takes in a SnapshotWriteMode (not optional), which is then of course only SIMPLE or FULL. Much simpler. WDYT?

Yep, this makes sense to me, allows communicating that there should not be a checksum written, but does not allow "requesting that no checksum be written", which doesn't really make sense.

allisonport-db · 2025-10-17T18:16:18Z

I'm still wondering whether writeChecksum really needs to take in a mode, and if so why? (I'm very open to hearing the reasoning here, because I can imagine there's a few possible arguments).

But couldn't you just presume, if you call writeChecksum, the caller is responsible for having checked the mode before-hand? It's already self-defined in the same object. Kind of weird to get a descriptor from your snapshot, and then pass it back as an arg to a method on the same snapshot.

scottsand-db · 2025-10-17T18:18:40Z

@allisonport-db Yup, fair questions. Both of these APIs are acceptable and reasonable. It comes down to a matter of slight preference, that's all.

writeChecksum(mode) means that the caller is explicitly aware of what mode is being used. they know if it will be cheap or expensive. an alternative we want to avoid is that callers invoke writeChecksum() without being aware that it will require a very expensive full log replay.

kernel/kernel-api/src/main/java/io/delta/kernel/Snapshot.java

allisonport-db · 2025-10-17T22:35:16Z

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

+          logger.warn("Requested checksum write in FULL mode, but SIMPLE mode is available");
+        }
        logger.info("Executing checksum write in FULL mode");
        ChecksumUtils.computeStateAndWriteChecksum(engine, getLogSegment());


Can't we just do new ChecksumWriter(logPath).writeCheckSum(engine, crcInfo) here when actual == ChecksumWriteMode.SIMPLE?

Not sure if computeStateAndWriteChecksum shortcuts that when possible?

Maybe we should switch on the actual mode instead?

We can, of course, but I don't think we should. The user has asked for a FULL replay -- that's what we should do. And we log the warning that they are doing this despite the SIMPLE being available

This is also what @nicklan asked for too -- #5340 (comment). I have a slight preference for that way (this current way) but totally understand your perspective, too (that's what I had initially coded up to begin with).

So -- I'm inclined to just go with this and merge this PR sooner than later -- and if you feel strongly to change this feel free to start a convo and we can easily change this later?

allisonport-db · 2025-10-17T22:38:08Z

...faults/src/test/scala/io/delta/kernel/defaults/SnapshotChecksumStatisticsAndWriteSuite.scala

      // ===== THEN =====
      val snapshot0 = result0.getPostCommitSnapshot.get()
-      assert(snapshot0.getStatistics.getChecksumWriteMode == ChecksumWriteMode.SIMPLE) // expected
+      assert(snapshot0.getStatistics.getChecksumWriteMode.get == ChecksumWriteMode.SIMPLE)


nit - use .contains?

This is a Java Optional -- it doesn't have .contains :/

scottsand-db changed the title ~~done first pass w/o tests~~ [Draft] [Kernel] SnapshotStatistics and new Snapshot write-crc-file APIs Oct 14, 2025

scottsand-db marked this pull request as draft October 14, 2025 23:18

scottsand-db requested a review from nicklan October 14, 2025 23:18

nicklan reviewed Oct 15, 2025

View reviewed changes

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

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

scottsand-db mentioned this pull request Oct 16, 2025

[Kernel] Implement catalogManaged table feature in Kernel #4573

Open

scottsand-db commented Oct 16, 2025

View reviewed changes

scottsand-db requested review from allisonport-db, huan233usc, nicklan and raveeram-db October 16, 2025 16:07

scottsand-db marked this pull request as ready for review October 16, 2025 16:07

scottsand-db added 8 commits October 16, 2025 16:30

done first pass w/o tests

b9464ff

update impl; add tests

383064e

rename method back; minor test update

ba8e423

make writing checksum required

a95c5f6

add tests

bb970ef

change API; update tests

48bb56f

add new test; add better logger.warn

471bc04

minor funcitonal code tweak

a8afd0b

scottsand-db force-pushed the stack/kernel_snapshot_crc_statistics_and_write branch from 4642bdd to a8afd0b Compare October 16, 2025 16:30

scottsand-db self-assigned this Oct 16, 2025

scottsand-db changed the title ~~[Draft] [Kernel] SnapshotStatistics and new Snapshot write-crc-file APIs~~ [Kernel] SnapshotStatistics and new Snapshot write-crc-file APIs Oct 16, 2025

fix DeltaTableWriteWithCrcSuite :: create table and configure propert…

18045b8

…ies with retries

nicklan approved these changes Oct 16, 2025

View reviewed changes

allisonport-db reviewed Oct 17, 2025

View reviewed changes

misc logger changes

b512fc2

change API again; make it return optional

669ef2a

scottsand-db requested a review from allisonport-db October 17, 2025 20:27

huan233usc approved these changes Oct 17, 2025

View reviewed changes

allisonport-db reviewed Oct 17, 2025

View reviewed changes

delta-io deleted a comment from allisonport-db Oct 17, 2025

update docs; add test

1d6dbba

scottsand-db merged commit b02611f into delta-io:master Oct 17, 2025
27 checks passed

Uh oh!

[Kernel] SnapshotStatistics and new Snapshot write-crc-file APIs #5340

[Kernel] SnapshotStatistics and new Snapshot write-crc-file APIs #5340

Uh oh!

Conversation

scottsand-db commented Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🥞 Stacked PR

Which Delta project/connector is this regarding?

Description

How was this patch tested?

Does this PR introduce any user-facing changes?

Uh oh!

nicklan left a comment • 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!

Uh oh!

Uh oh!

scottsand-db commented Oct 15, 2025

Uh oh!

scottsand-db Oct 16, 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!

nicklan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scottsand-db Oct 17, 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!

scottsand-db commented Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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!

scottsand-db commented Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicklan commented Oct 17, 2025

Uh oh!

scottsand-db commented Oct 14, 2025 •

edited

Loading

nicklan left a comment •

edited

Loading

scottsand-db Oct 16, 2025 •

edited

Loading

scottsand-db Oct 17, 2025 •

edited

Loading

scottsand-db commented Oct 17, 2025 •

edited

Loading

scottsand-db commented Oct 17, 2025 •

edited

Loading