Simplify log replay visitor and avoid materializing Add/Remove actions #494

scovich · 2024-11-14T21:55:27Z

What changes are proposed in this pull request?

The existing log replay logic materialized full Add and Remove actions in order to examine the four fields of each that comprise the file comparison key. There was also a lot of indirection and duplication because of an old (now-unused) flavor of log replay. It also created a new expression evaluator for each batch instead of reusing it for the whole iteration.

Streamline the logic to only visit the columns of interest and generally reduce code bloat.

How was this change tested?

Existing log replay unit tests.

…ions

codecov · 2024-11-14T22:14:12Z

Codecov Report

Attention: Patch coverage is 72.28261% with 102 lines in your changes missing coverage. Please review.

Project coverage is 79.70%. Comparing base (67cc099) to head (1b76ce5).

Files with missing lines	Patch %	Lines
kernel/src/engine/arrow_data.rs	66.08%	23 Missing and 16 partials ⚠️
kernel/src/actions/visitors.rs	47.05%	34 Missing and 2 partials ⚠️
kernel/src/scan/log_replay.rs	85.32%	6 Missing and 10 partials ⚠️
kernel/src/scan/state.rs	54.54%	4 Missing and 1 partial ⚠️
kernel/src/actions/mod.rs	70.00%	2 Missing and 1 partial ⚠️
kernel/src/actions/deletion_vector.rs	90.00%	1 Missing ⚠️
kernel/src/actions/set_transaction.rs	0.00%	0 Missing and 1 partial ⚠️
kernel/src/scan/data_skipping.rs	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #494      +/-   ##
==========================================
- Coverage   79.82%   79.70%   -0.12%     
==========================================
  Files          57       57              
  Lines       12591    12676      +85     
  Branches    12591    12676      +85     
==========================================
+ Hits        10051    10104      +53     
- Misses       2006     2045      +39     
+ Partials      534      527       -7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

scovich · 2024-11-14T22:14:41Z

kernel/src/scan/log_replay.rs

        }
    }

    fn visit<'a>(&mut self, row_count: usize, getters: &[&'a dyn GetData<'a>]) -> DeltaResult<()> {
-        let expected_getters = if self.is_log_batch { 29 } else { 15 };


nice ~4x improvement from column pruning

scovich · 2024-11-14T22:24:07Z

kernel/src/scan/log_replay.rs

                self.get_add_transform_expr(),
                SCAN_ROW_DATATYPE.clone(),
            )
            .evaluate(actions)?;
        Ok((result, selection_vector))
    }
-
-    // work shared between process_batch and process_scan_batch


Note: process_batch was deleted dead code, so we can fold the logic of this method into its single call site

zachschuermann

(reviewed 4 commits 416e7bd..1b76ce5) LGTM! very excited for this to land, thanks @scovich!!! i asked for docs in quite a few places, hopefully i'm not going overboard on doc requests (please tell me if i am). this has awesome APIs and expect a decent perf boost :) - in that vein I should have some benchmarks spun up in the new few weeks for us!

kernel/src/actions/visitors.rs

zachschuermann · 2024-11-21T17:21:10Z

kernel/src/scan/log_replay.rs

        }
    }

    fn visit<'a>(&mut self, row_count: usize, getters: &[&'a dyn GetData<'a>]) -> DeltaResult<()> {
-        let expected_getters = if self.is_log_batch { 29 } else { 15 };


kernel/src/scan/log_replay.rs

zachschuermann · 2024-11-21T19:20:32Z

kernel/src/scan/log_replay.rs

+            Some(storage_type) => Some(DeletionVectorDescriptor::unique_id_from_parts(
+                storage_type,
+                getters[1].get(i, "deletionVector.pathOrInlineDv")?,
+                getters[2].get_opt(i, "deletionVector.offset")?,
+            )),
+            None => None,


could just do a .map(...)?

A map call would intercept those ?, which is annoying and bulky to deal with:

let dv_unique_id = getters[0] .get_opt(i, "deletionVector.storageType")? .map(|storage_type| { DeletionVectorDescriptor::unique_id_from_parts( storage_type, getters[1].get(i, "deletionVector.pathOrInlineDv")?, getters[2].get_opt(i, "deletionVector.offset")?, ) }) .transpose()?;

oh I was thinking of a storage_type.map(DeletionVectorDescriptor...) but probably has the same ? issue?

zachschuermann · 2024-11-21T19:22:56Z

kernel/src/scan/log_replay.rs

-    removes: Vec<Remove>,
-    selection_vector: Option<Vec<bool>>,
-    // whether or not we are visiting commit json (=true) or checkpoint (=false)
+struct AddRemoveDedupVisitor<'seen> {


nit: maybe quick docs sorry for asking for docs so much lol and probably useful to derive debug? (i think every field already implements Debug?)

Suggested change

struct AddRemoveDedupVisitor<'seen> {

#[derive(Debug)]

struct AddRemoveDedupVisitor<'seen> {

Doc comment added. Is it normal in rust to derive Debug for non-public classes that don't (yet) need it?
(seems easy enough to add later if/when a need arises?)

we can leave it without - generally i [derive(Debug)] on nearly everything just under the assumption that someone will want to debug print it or log in the future etc.

zachschuermann · 2024-11-21T19:30:03Z

kernel/src/scan/log_replay.rs

-        }
-
-        Ok(visitor.adds)
+        // TODO: Teach expression eval to respect the selection vector we just computed so carefully!


cursory look didn't yield an issue for this - should we create one to make sure it doesn't get dropped?

Yeah we should. I believe kernel-java already has this support.

kernel/src/scan/log_replay.rs

codecov · 2024-11-22T21:42:50Z

Codecov Report

Attention: Patch coverage is 86.91589% with 14 lines in your changes missing coverage. Please review.

Project coverage is 80.78%. Comparing base (a245355) to head (39458f2).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
kernel/src/scan/log_replay.rs	87.50%	3 Missing and 9 partials ⚠️
kernel/src/actions/deletion_vector.rs	90.00%	1 Missing ⚠️
kernel/src/actions/visitors.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #494      +/-   ##
==========================================
- Coverage   81.04%   80.78%   -0.27%     
==========================================
  Files          67       67              
  Lines       14099    14074      -25     
  Branches    14099    14074      -25     
==========================================
- Hits        11426    11369      -57     
- Misses       2091     2130      +39     
+ Partials      582      575       -7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

OussamaSaoudi-db

Just small nits/questions :)

OussamaSaoudi-db · 2024-11-22T22:06:30Z

kernel/src/scan/log_replay.rs

-            is_log_batch,
-            ..Default::default()
+        if self.seen.contains(&key) {
+            debug!(


What's your criteria for putting in debug logging? I want to know when it might be useful to incorporate into my own code.

I don't have any really concrete criteria... but if I added a println! for debugging that turned out to be especially useful, I'm a lot more likely to leave it there as a debug!?

OussamaSaoudi-db · 2024-11-22T22:08:39Z

kernel/src/scan/log_replay.rs

+        // may have a remove with a path at index 4.
+        let (path, getters, is_add) = if let Some(path) = getters[0].get_str(i, "add.path")? {
+            (path, &getters[1..4], true)
+        } else if !self.is_log_batch {


I don't quite understand is_log_batch. This is a visitor for log actions, so presumably everything is a log batch?

Might be worth a doc comment or a rename.

ah yea maybe this could use some more clarity in the code but ill just jump in to say: is_log_batch is supposed to mean is_commit_batch (that is, a batch of actions from commits vs checkpoints)

aha makes sense, ty!

Yeah this is really old terminology -- "log file" in the Delta spec means a nnn.json file -- what we normally call "commit file" today.

kernel/src/scan/log_replay.rs

OussamaSaoudi-db · 2024-11-22T22:31:37Z

kernel/src/scan/log_replay.rs

    fn process_scan_batch(
        &mut self,
-        expression_handler: &dyn ExpressionHandler,
+        expression: &dyn ExpressionEvaluator,


nit: expression implies this is a predicate. Perhaps expression_evaluator, but your call :)

Renamed it to say (conceptually) what it is: add_transform

update: renamed the wrong one (or rather, renamed a different good one)

I renamed this one as predicate

... and renamed back to add_transform because it's not a predicate at all

OussamaSaoudi-db · 2024-11-22T22:39:55Z

One more thing: If it doesn't block you, I'd love to see tests to make sure that:

We are constructing correct selection vector
That we do filter batches that have no actions selected

zachschuermann

restamp

nicklan

lgtm, just two small naming/comment nits

kernel/src/scan/log_replay.rs

scovich · 2024-11-26T01:59:33Z

I'd love to see tests to make sure that:
1. We are constructing correct selection vector

Existing unit tests fail if the selection vector is wrong (that's how I figured out the vector needs to initialize as all-true rather than all-false like it used to).

2. That we do filter batches that have no actions selected

You mean, because data skipping eliminated all adds? The code already has coverage from at least one unit test.

scovich · 2024-11-26T02:18:05Z

FYI: I noticed the most recent semver check had not found anything, so I removed the label and retriggered the job. It completed successfully without re-adding the label, confirming that this is not a breaking change. Most likely the label came from the refactor PR it was originally stacked on.

scovich added 10 commits November 8, 2024 13:40

[VERY WIP] Streamline the log replay code to not materialize file act…

6699f25

…ions

got it working

fa98420

refactor visitors to announce their schema (not used yet)

7408316

use schema and cleanup code

1f5aeba

Merge remote-tracking branch 'oss/main' into better-replay-visitor

3ea3e51

revert log replay changes, clean up code

a703899

add basic column count sanity checking to visitors

891c3b1

doc fix

cf95514

Merge remote-tracking branch 'oss/main' into better-row-visitors

b7f8a00

Simplify log replay visitor and avoid materializing Add/Remove actions

416e7bd

scovich requested review from nicklan and zachschuermann November 14, 2024 21:55

github-actions bot assigned scovich Nov 14, 2024

github-actions bot added the breaking-change Change that will require a version bump label Nov 14, 2024

scovich added 2 commits November 14, 2024 14:10

small fixes

9a354f8

comment fix

d6ba06b

scovich commented Nov 14, 2024

View reviewed changes

even more simplifications

1b76ce5

scovich mentioned this pull request Nov 19, 2024

Improved row visitor API #481

Merged

zachschuermann approved these changes Nov 21, 2024

View reviewed changes

scovich added 3 commits November 21, 2024 13:27

address reviews

079efbd

Merge remote-tracking branch 'oss/main' into better-replay-visitor

97cb98c

rename method

0adbaf0

scovich added 2 commits November 22, 2024 13:43

fmt

6e77e4d

comment

5fba134

OussamaSaoudi-db reviewed Nov 22, 2024

View reviewed changes

zachschuermann approved these changes Nov 23, 2024

View reviewed changes

nicklan approved these changes Nov 26, 2024

View reviewed changes

kernel/src/scan/log_replay.rs Show resolved Hide resolved

kernel/src/scan/log_replay.rs Outdated Show resolved Hide resolved

scovich mentioned this pull request Nov 26, 2024

Implement Log Replay for Change Data Feed #540

Merged

scovich and others added 2 commits November 25, 2024 17:45

address reviews

e12d7c7

Merge branch 'main' into better-replay-visitor

e745475

last nit

39458f2

scovich removed the breaking-change Change that will require a version bump label Nov 26, 2024

scovich merged commit 485525b into delta-io:main Nov 26, 2024
19 of 20 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify log replay visitor and avoid materializing Add/Remove actions #494

Simplify log replay visitor and avoid materializing Add/Remove actions #494

scovich commented Nov 14, 2024 •

edited

Loading

codecov bot commented Nov 14, 2024 •

edited

Loading

scovich Nov 14, 2024

zachschuermann Nov 21, 2024

scovich Nov 14, 2024

zachschuermann left a comment

zachschuermann Nov 21, 2024

zachschuermann Nov 21, 2024

scovich Nov 21, 2024

zachschuermann Nov 23, 2024

scovich Nov 26, 2024

zachschuermann Nov 21, 2024

scovich Nov 21, 2024 •

edited

Loading

zachschuermann Nov 23, 2024

zachschuermann Nov 21, 2024

scovich Nov 21, 2024

zachschuermann Nov 23, 2024

codecov bot commented Nov 22, 2024 •

edited

Loading

OussamaSaoudi-db left a comment

OussamaSaoudi-db Nov 22, 2024

scovich Nov 26, 2024

OussamaSaoudi-db Nov 22, 2024

zachschuermann Nov 23, 2024

OussamaSaoudi-db Nov 23, 2024

scovich Nov 25, 2024

OussamaSaoudi-db Nov 22, 2024

scovich Nov 26, 2024

scovich Nov 26, 2024

scovich Nov 26, 2024

OussamaSaoudi-db commented Nov 22, 2024

zachschuermann left a comment

nicklan left a comment

scovich commented Nov 26, 2024

scovich commented Nov 26, 2024

	struct AddRemoveDedupVisitor<'seen> {
	#[derive(Debug)]
	struct AddRemoveDedupVisitor<'seen> {

Simplify log replay visitor and avoid materializing Add/Remove actions #494

Simplify log replay visitor and avoid materializing Add/Remove actions #494

Conversation

scovich commented Nov 14, 2024 • edited Loading

What changes are proposed in this pull request?

How was this change tested?

codecov bot commented Nov 14, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zachschuermann 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

scovich Nov 21, 2024 • edited Loading

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

codecov bot commented Nov 22, 2024 • edited Loading

Codecov Report

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

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

OussamaSaoudi-db commented Nov 22, 2024

zachschuermann left a comment

Choose a reason for hiding this comment

nicklan left a comment

Choose a reason for hiding this comment

scovich commented Nov 26, 2024

scovich commented Nov 26, 2024

scovich commented Nov 14, 2024 •

edited

Loading

codecov bot commented Nov 14, 2024 •

edited

Loading

scovich Nov 21, 2024 •

edited

Loading

codecov bot commented Nov 22, 2024 •

edited

Loading