Use compacted log files during log replay by nicklan · Pull Request #4477 · delta-io/delta

nicklan · 2025-04-29T20:23:19Z

Which Delta project/connector is this regarding?

Description

This PR builds on #4457 and adds functionality to omit the original json files and just use the compacted files. It introduces a new way to get the list of files from the LogSegment that returns the compacted files in place of the original jsons.

For now we just always use these files, and can plumb through an option as a follow-on if its requested.

How was this patch tested?

Unit tests
Integration tests on a new table with compactions in the log

Does this PR introduce any user-facing changes?

No, users do not (currently) need to be aware of this.

allisonport-db

Source code looks good just had some comments on testing

allisonport-db · 2025-04-30T23:48:11Z

      expectedAnswer = (0 until 150).map(i => TestRow(i.toLong)))
  }

+  test("table with checkpoint and log compaction") {


should we do something to test that we are in fact reading the compaction files for this? I think there's no way to know how we are loading this data right?

I think we already have utils for this that can save what files we read I can help find them if needed

also as discussed we should (1) test with the compactions written by Kernel (2) test with compaction files with all the possible actions in it (removes, domains metadatas, etc)

This is now tested. I only tested adds/removes. The LogCompactionWriterSuite verifies that all supported actions are preserved, and we read these compactions as "normal" json files, so I believe all actual functionality is tested. I'd be quite a lot more test code to check all these actions, so I've opted not to for now. If you think it's important, let me know why, and I can look at adding them in.

Why is it a lot more code to also check the other actions?

For example for DM, I think you can just set a domain in one of the commits and then to check it just use snapshot.getDomain in the check?

And then for like a metadata action, just change a table property?

I would be more inclined to prefer 1 test that has a compaction file with everything (all actions) in it and checks everything but it doesn't need to be repeated with all the different cases you've added below. I think those are fairly well covered by the LogSegmentSuite

allisonport-db · 2025-04-30T23:50:56Z

+    this.allFilesWithCompactionsReversed = new Lazy<>(() -> loadAllFilesWithCompactionsReversed());
+  }
+
+  private List<FileStatus> loadAllFilesWithCompactionsReversed() {


Can you point me to the corresponding code for this in delta spark?

allisonport-db · 2025-04-30T23:52:40Z

    assert(logSegment.toString === expectedToString)
  }
+
+  test("allFilesWithCompactionsReversed -- 3 - 5 in middle") {


is it possible for log compaction files to overlap with each other? what would happen then?

we can have multiple compaction files also right? let's test that? (back to back and separated?)

also what about a test where the log compaction file spans the entire version range (i.e. only that should be returned)?

should we include checkpoints in some of the test cases?

Yep, these are all added now. For checkpoints I only test in the actual read test, but it's pretty comprehensive there.

scottsand-db

Looks great! Left some questions / comments / food for thought. Heading out soon -- need to review in more detail next work week

scottsand-db · 2025-05-03T01:19:22Z

+        checkArgument(
+            compactions.stream()
+                .allMatch(
+                    fs -> {
+                      Tuple2<Long, Long> versions = FileNames.logCompactionVersions(fs.getPath());
+                      return versions._2 <= version;
+                    }),
+            "compactions must have end version <= version");


Can this be moved up next to the compactions must have start version less than end version check?

scottsand-db · 2025-05-03T01:21:47Z

+    // - set a "hi/lo" goalpost around the next compactions
+    // - for each delta, if its version is:
+    //   - greater than the current compaction high point, include it, move to next delta
+    //   - less than (but not equal to) the current low point, skip it, move to next delta


the current compaction low point

scottsand-db · 2025-05-03T01:26:30Z

+        FileStatus currentDelta = deltaIt.next();
+        long deltaVersion = FileNames.deltaVersion(currentDelta.getPath());
+        if (deltaVersion == currentCompactionLo) {
+          // we're about to cross out of the compaction. insert the compaction and advance to the


I think this requires (fairly so) that the delta versions are contiguous. i.e. it requires that 10.json exists for us to then accept 10.20.json into the ret

Q: What would change if you added the compaction at the "start" as opposed to at the end (or, eagerly instead of lazily)?

Could we future proof this for some crazy future world where there are gaps in the delta log (suppose a future catalogManagedV2 table feature allows for unordered backfill) ?

Not a blocker, but wanted to ask this / call it out

Well, if there are gaps I think the logic gets way more complex. If we assume we handle all the missing stuff somehow, I guess the "fix" here would just be (deltaVersion <= currentCompactionLo), so maybe not worth trying to "fix" things until we know what we actually need to do.

scottsand-db · 2025-05-03T01:29:51Z

+            () ->
+                compactions.stream()
+                    .sorted(
+                        Comparator.comparing((FileStatus a) -> new Path(a.getPath()).getName())


Can we sort by end version instead? Not lexicographically?

It would be more optimal, for example, to process the below in reverse endVersion order

80.99.json (start of the reverse list) 90.95.json 81.90.json

Lexicographical order should always be the endVersion order, except when an x.y has a bigger "range" (num commits inside it) than others around it.

edit: I suppose a counter example here would be a 98.100.json? a tiny compaction file .. where using a sort by end order isn't optimal?

edit2: perhaps if we assume constant size of compactions, then lexicographical order is equally as good?

In general throughout our code in Kernel, we should probably clarify / specify exactly what "order" means (lexicographical vs numerical) :/

Another example, btw, is due to staged commit files, where the _staged_commits makes them come at the bottom of any sort order (which isn't great - we will need to fix this for catalogManaged tables)

perhaps if we assume constant size of compactions, then lexicographical order is equally as good?

This was my assumption, but I realize it could be different, say if different clients prefer different intervals. So I've switched to end version as it's equal or better (at minor increase in parsing cost)

scottsand-db

Looks great! Logic in the LogSegment class is very clear, thank you!

Added some comments on the tests

scottsand-db · 2025-05-05T15:48:07Z

+    // - for each delta, if its version is:
+    //   - greater than the current compaction high point, include it, move to next delta
+    //   - less than (but not equal to) the current low point, skip it, move to next delta
+    //   - equal to the current compaction low point, we're above to transition out of the


scottsand-db · 2025-05-05T15:50:26Z

+    int compactionPos = 0;
+    long currentCompactionHi = -1;
+    long currentCompactionLo = -1;


compactionPos --> currentCompactionPos (for consistency?)

and if this compactionPos is valid (not after the end of the list) that means we WILL be accepting it, right? i.e. it only ever points to a valid compaction, and we will be adding it to our output once we see the delta at the compaction's min version?

maybe add a comment that says how this compaction is always valid (i.e. it is not a compaction we are "considering" adding?)

not a blocker -- I defer to you

scottsand-db · 2025-05-05T15:54:18Z

+
+    // expect to get 6, 3-5.compact, 2, 1, 0
+    val expected = List(
+      FileStatus.of(FileNames.deltaFile(logPath, 6), 6, 6 * 10),


Add deltaFileStatus(version: Long) and compactedFileStatus (singular) helpers?

so just

deltaFileStatus(6), compactedFileStatus(3, 5), deltaFileStatus(2), deltaFileStatus(1), deltaFileStatus(0)

allisonport-db · 2025-05-05T17:56:49Z

Does it make sense to put these tests in a separate test suite?

Yep, have moved it now, good call

allisonport-db · 2025-05-05T19:45:03Z

+      }
+      engine.resetMetrics()
+
+      checkTable(


We are using the log compactions for three different operations right? (1) snapshot loading (P&M only) (2) log replay (Scans) (3) loading the metadata domain map

Should we test that we use it for each of these? It seems like maybe before these recent commits we actually had missed (2)?

I think this doesn't necessarily need to be too complex of a test, we can test all of these things using 1 single test table built with just a few versions (and reset the metrics in between each check).

Alternatively maybe we can accomplish this instead by not converting the actualCompactionsRead to set and instead checking we actually read a specific compaction file twice for example when snapshot laoding + scanning

Yep. I've moved this to a new test file now, and added a test with DM and a property change.

I didn't include removes in that test because I don't think kernel can do that just yet (easily), but that's well covered by the other tests.

allisonport-db · 2025-05-05T19:51:24Z

      expectedAnswer = (0 until 150).map(i => TestRow(i.toLong)))
  }

+  test("table with checkpoint and log compaction") {


Why is it a lot more code to also check the other actions?

For example for DM, I think you can just set a domain in one of the commits and then to check it just use snapshot.getDomain in the check?

And then for like a metadata action, just change a table property?

allisonport-db · 2025-05-05T19:53:10Z

      expectedAnswer = (0 until 150).map(i => TestRow(i.toLong)))
  }

+  test("table with checkpoint and log compaction") {


I would be more inclined to prefer 1 test that has a compaction file with everything (all actions) in it and checks everything but it doesn't need to be repeated with all the different cases you've added below. I think those are fairly well covered by the LogSegmentSuite

scottsand-db

LGTM -- after please implementing the one last change to move the helper to MockFileSystemUtils so that we are standardized / can re-use it easily.

Great tests and great implementation, thanks!

allisonport-db

LGTM

allisonport-db · 2025-05-06T17:16:15Z

+        snapshot.getMetadata().getConfiguration.get(TableConfig.CHECKPOINT_POLICY.getKey)
+      assert(checkpointProp == "v2")
+
+      // this is the read that the snapshot did


these checks for each operation are awesome!

nicklan added 2 commits April 30, 2025 09:37

apply stash

dd55380

move to own function

21047c2

nicklan force-pushed the use-compacted-log-files branch from cdb26f8 to 21047c2 Compare April 30, 2025 16:41

add comment

144a74a

nicklan changed the title ~~[Stacked] Use compacted log files during log replay~~ Use compacted log files during log replay Apr 30, 2025

nicklan added 3 commits April 30, 2025 12:00

remove option

9db54f7

use end version

6c516c7

Add some end-to-end tests

802c506

nicklan requested review from allisonport-db and scottsand-db April 30, 2025 21:06

nicklan marked this pull request as ready for review April 30, 2025 21:06

allisonport-db suggested changes Apr 30, 2025

View reviewed changes

scottsand-db reviewed May 1, 2025

View reviewed changes

Comment thread kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/LogSegment.java

scottsand-db reviewed May 1, 2025

View reviewed changes

Comment thread kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/LogSegment.java Outdated

nicklan added 5 commits May 2, 2025 10:49

Rewrite resolution logic with hi/lo logic

25842b9

Add tests reading generated and compacted data

977a05b

more tests

c64817b

include testing removes

1e76f73

check that compactions are in range

1d6c69e

nicklan requested review from allisonport-db and scottsand-db May 3, 2025 01:11

scottsand-db reviewed May 3, 2025

View reviewed changes

scottsand-db requested changes May 5, 2025

View reviewed changes

allisonport-db reviewed May 5, 2025

View reviewed changes

nicklan added 6 commits May 5, 2025 13:21

comment + use end + combine check

a8abaa4

address comments

bdba724

Merge branch 'master' into use-compacted-log-files

aea7faa

remove new on-disk test

4bfe127

Make LogCompactionSuite a new suite

a11d6ea

add test with different action types

6704d80

Ensure data is correct

82c6d9e

nicklan requested review from allisonport-db and scottsand-db May 6, 2025 00:28

keep compactions in range

b192aff

scottsand-db approved these changes May 6, 2025

View reviewed changes

Comment thread kernel/kernel-api/src/test/scala/io/delta/kernel/internal/snapshot/LogSegmentSuite.scala Outdated

move to MockFileSystemClientUtils

e81df04

allisonport-db approved these changes May 6, 2025

View reviewed changes

nicklan merged commit fcf839f into delta-io:master May 6, 2025

Uh oh!

Conversation

nicklan commented Apr 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which Delta project/connector is this regarding?

Description

How was this patch tested?

Does this PR introduce any user-facing changes?

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!

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!

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!

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scottsand-db May 3, 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!

scottsand-db May 3, 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 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!

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!

nicklan commented Apr 29, 2025 •

edited

Loading

scottsand-db left a comment •

edited

Loading

scottsand-db May 3, 2025 •

edited

Loading

scottsand-db May 3, 2025 •

edited

Loading