feat: add visitor `SidecarVisitor` and `Sidecar` action struct #673

sebastiantia · 2025-02-01T00:52:21Z

What changes are proposed in this pull request?

This PR introduces the Sidecar action and its associated SidecarVisitor.

This action and visitor is used for the V2 checkpoints Reader/Writer table feature.

Edge cases:

If a batch of actions includes two sidecar actions referencing the same file, issue a warning. We will not throw an error; instead, we will ignore the duplicate file path to prevent unnecessary re-reading of the file.
Error if the sidecar path field is a full file-path instead of just the filename. Ref to issue: Support full paths instead of only file-names in sidecar actions's path field #675

resolves #668

Note: PR also simplifies some conditional flags detailed in issue: #672

How was this change tested?

Ensure schema projection to Sidecar works
Ensure that the visitor correctly extracts Sidecar actions
Ensure duplicate paths in sidecar actions are ignored
Ensure VisitorError is returned when full file path is passed in path field

sebastiantia · 2025-02-01T00:55:22Z

kernel/src/actions/mod.rs

+#[cfg_attr(test, derive(Serialize, Default), serde(rename_all = "camelCase"))]
+struct Sidecar {
+    /// A path to the sidecar file. Because sidecar files must always reside in the table's own
+    ///  _delta_log/_sidecars directory, implementations are encouraged to store only the file's name.


This comment is quoted from the protocol linked here.

I am assuming this means we will need to handle both cases where the path is:

just the file name of the sidecar files

the absolute path (including parent directories)

Or can we assume that implementations are not only encouraged, but DO simply store the file name of the sidecar files as the path?

Delta spark assumes that path is just the file name:
https://github.com/delta-io/delta/blob/990c8e8aedb69a1660e7780ab1d1a19f5c95615f/kernel/kernel-api/src/main/java/io/delta/kernel/internal/replay/ActionsIterator.java#L274

Can I get a 👍 to do the same?

In think we have to stick to the protocol here and assume it may also be a URL. That said, not sure how many writers out there in the wild already write v2 checkpoints. Maybe we can still update the protocol?

Thanks for the input @roeap. instead of blocking on updating the protocol, I'm going to go ahead with implementing support for only filenames in this PR along with error handling when a file path is passed. Supporting full-path parsing should be supported like you said so I've opened up an issue for future work #675

If this is the route we decide to go down, make sure that's clear in the docs.

codecov · 2025-02-01T00:57:04Z

Codecov Report

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

Project coverage is 84.17%. Comparing base (06d8dbb) to head (91e550c).

Files with missing lines	Patch %	Lines
kernel/src/actions/visitors.rs	88.07%	4 Missing and 9 partials ⚠️
ffi/src/error.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #673      +/-   ##
==========================================
+ Coverage   84.14%   84.17%   +0.03%     
==========================================
  Files          77       77              
  Lines       17710    17834     +124     
  Branches    17710    17834     +124     
==========================================
+ Hits        14902    15012     +110     
- Misses       2096     2101       +5     
- Partials      712      721       +9

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

roeap

looking good. just some minor comments.

kernel/src/actions/mod.rs

roeap · 2025-02-03T13:03:43Z

kernel/src/actions/mod.rs

+#[cfg_attr(test, derive(Serialize, Default), serde(rename_all = "camelCase"))]
+struct Sidecar {
+    /// A path to the sidecar file. Because sidecar files must always reside in the table's own
+    ///  _delta_log/_sidecars directory, implementations are encouraged to store only the file's name.


In think we have to stick to the protocol here and assume it may also be a URL. That said, not sure how many writers out there in the wild already write v2 checkpoints. Maybe we can still update the protocol?

kernel/src/actions/mod.rs

kernel/src/actions/visitors.rs

OussamaSaoudi · 2025-02-03T17:54:13Z

kernel/src/actions/visitors.rs

+            r#"{"sidecar":{"path":"016ae953-37a9-438e-8683-9a9a4a79a395.parquet","sizeInBytes":9268,"modificationTime":1714496113961,"tags": null}}"#,
+            r#"{"sidecar":{"path":"3a0d65cd-4056-49b8-937b-95f9e3ee90e5.parquet","sizeInBytes":9268,"modificationTime":1714496113962,"tags": null}}"#,


I wonder if we should just merge this test with test_parse_action_batch_without_sidecar_actions by moving the sidecar actions here into the action_batch. That we we test both that:

We correctly parse sidecar actions

We correctly ignore irrelevant actions.

^ similar to protocol and cdc

Also make one of the sidecar files have a real tags field with a string-string map.

Finally, I'm not certain "tags": null is something that occurs in tables in the wild. Would tags just be omitted? In any case, we should probably test that we can take a sidecar action with no tags.

Synced with Oussama on tags field, which can be:
not defined (omitted) / null (which does not get omitted from the action & shows up as null) / string: string map

Will be merging the tests and I'll add a sidecar action into the action_batch to make sure it is ignored during parsing for other actions.

kernel/src/actions/visitors.rs

OussamaSaoudi · 2025-02-03T18:01:20Z

kernel/src/actions/visitors.rs

@@ -544,6 +584,36 @@ mod tests {
        Ok(())
    }

+    #[test]


Aside: None of these tests check the error case where a required field is missing 🤔 Maybe we should make an issue to add those. Personally I'd also like to see a VisitorError type so that we can make stronger assertions on the failure cases.

good idea! #674

sebastiantia · 2025-02-03T22:55:51Z

kernel/src/actions/visitors.rs

+            // Since path column is required, use it to detect presence of a sidecar action
+            if let Some(path) = getters[0].get_opt(i, "sidecar.path")? {
+                if self.seen_paths.contains(path) {
+                    warn!("Duplicate sidecar path {} found during visiting", path);


If a batch of actions includes two sidecar actions referencing the same file, we issue a warning. We will ignore the duplicate file path to prevent unnecessary re-reading of the file and not throw an error as we can still construct table state.

This is done as a best-effort. Ideally we could ensure no sidecar files are read more than once by tracking sidecar file paths across all batches that makeup a checkpoint file, but this introduces an unbounded memory dependency on the # of sidecar-paths encountered (as we would need to store them all somehow). I do not think the added performance in cases where sidecar files are referenced more than once (unlikely) warrants more than this change.

I'm not sure if it's worth doing this extra work just to issue a warning. The vast majority of cases, we won't encounter this. If we do encounter it, AddRemoveDedupVisitor will handle it. I think of it like this: Expensive checks should only be done when it's a matter of correctness. All other checks/warnings should be cheap to do.

I would consider this an expensive check because we use double the memory to maintain both the Vec and the HashSet.

OussamaSaoudi · 2025-02-03T23:15:06Z

kernel/src/actions/mod.rs

+#[cfg_attr(test, derive(Serialize, Default), serde(rename_all = "camelCase"))]
+struct Sidecar {
+    /// A path to the sidecar file. Because sidecar files must always reside in the table's own
+    ///  _delta_log/_sidecars directory, implementations are encouraged to store only the file's name.


If this is the route we decide to go down, make sure that's clear in the docs.

OussamaSaoudi · 2025-02-03T23:22:49Z

kernel/src/actions/visitors.rs

+            // Since path column is required, use it to detect presence of a sidecar action
+            if let Some(path) = getters[0].get_opt(i, "sidecar.path")? {
+                if self.seen_paths.contains(path) {
+                    warn!("Duplicate sidecar path {} found during visiting", path);


I'm not sure if it's worth doing this extra work just to issue a warning. The vast majority of cases, we won't encounter this. If we do encounter it, AddRemoveDedupVisitor will handle it. I think of it like this: Expensive checks should only be done when it's a matter of correctness. All other checks/warnings should be cheap to do.

I would consider this an expensive check because we use double the memory to maintain both the Vec and the HashSet.

OussamaSaoudi · 2025-02-03T23:26:29Z

kernel/src/actions/visitors.rs

+
+        // The visitor should error out when it encounters a full file path
+        let res = visitor.visit_rows_of(batch.as_ref()).unwrap_err();
+        assert!(matches!(res, Error::VisitorError(_)));


Oh sry I should've made myself clear. VisitorError should definitely be in a separate PR/issue. Rust errors are best represented as enums where every error case is its own enum variant. That's a non-trivial amount of code, so it's best left for later.

Oh all right, I can go ahead and revert this

OussamaSaoudi · 2025-02-03T23:27:23Z

kernel/src/actions/visitors.rs

+        // The second sidecar is a duplicate of the first, so one is ignored
+        assert_eq!(visitor.sidecars.len(), 1);
+        assert_eq!(visitor.sidecars[0], sidecar1);


Like I mentioned above, I don't think this is a case we want to optimize/test for.

OussamaSaoudi · 2025-02-03T23:30:01Z

kernel/src/actions/visitors.rs

+                    continue;
+                }
+
+                if path.contains('/') {


If we have consensus that path should only be the file name, add a comment explaining that we only allow filename and not path.

sebastiantia added 3 commits January 31, 2025 16:07

first pass

5b85cdf

simplify

ab93b8b

adjust visibility

5e31858

sebastiantia requested review from OussamaSaoudi and OussamaSaoudi-db February 1, 2025 00:52

github-actions bot assigned sebastiantia Feb 1, 2025

sebastiantia commented Feb 1, 2025

View reviewed changes

sebastiantia marked this pull request as ready for review February 1, 2025 01:04

sebastiantia requested a review from zachschuermann February 1, 2025 01:05

roeap reviewed Feb 3, 2025

View reviewed changes

OussamaSaoudi reviewed Feb 3, 2025

View reviewed changes

review

91e550c

sebastiantia commented Feb 3, 2025

View reviewed changes

github-actions bot added the breaking-change Change that will require a version bump label Feb 3, 2025

sebastiantia requested review from OussamaSaoudi and roeap February 3, 2025 23:08

OussamaSaoudi reviewed Feb 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add visitor `SidecarVisitor` and `Sidecar` action struct #673

feat: add visitor `SidecarVisitor` and `Sidecar` action struct #673

sebastiantia commented Feb 1, 2025 •

edited

Loading

sebastiantia Feb 1, 2025 •

edited

Loading

roeap Feb 3, 2025

sebastiantia Feb 3, 2025

OussamaSaoudi Feb 3, 2025

codecov bot commented Feb 1, 2025 •

edited

Loading

roeap left a comment

roeap Feb 3, 2025

OussamaSaoudi Feb 3, 2025

OussamaSaoudi Feb 3, 2025

OussamaSaoudi Feb 3, 2025

OussamaSaoudi Feb 3, 2025

sebastiantia Feb 3, 2025 •

edited

Loading

OussamaSaoudi Feb 3, 2025

sebastiantia Feb 3, 2025

sebastiantia Feb 3, 2025 •

edited

Loading

OussamaSaoudi Feb 3, 2025

OussamaSaoudi Feb 3, 2025

OussamaSaoudi Feb 3, 2025

OussamaSaoudi Feb 3, 2025

sebastiantia Feb 3, 2025

OussamaSaoudi Feb 3, 2025

OussamaSaoudi Feb 3, 2025

		r#"{"sidecar":{"path":"016ae953-37a9-438e-8683-9a9a4a79a395.parquet","sizeInBytes":9268,"modificationTime":1714496113961,"tags": null}}"#,
		r#"{"sidecar":{"path":"3a0d65cd-4056-49b8-937b-95f9e3ee90e5.parquet","sizeInBytes":9268,"modificationTime":1714496113962,"tags": null}}"#,

@@ @@ -544,6 +584,36 @@ mod tests { @@
                       Ok(())
                   }
+                  #[test]

feat: add visitor SidecarVisitor and Sidecar action struct #673

Are you sure you want to change the base?

feat: add visitor SidecarVisitor and Sidecar action struct #673

Conversation

sebastiantia commented Feb 1, 2025 • edited Loading

What changes are proposed in this pull request?

How was this change tested?

sebastiantia Feb 1, 2025 • 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

codecov bot commented Feb 1, 2025 • edited Loading

Codecov Report

roeap 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

sebastiantia Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebastiantia Feb 3, 2025 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat: add visitor `SidecarVisitor` and `Sidecar` action struct #673

feat: add visitor `SidecarVisitor` and `Sidecar` action struct #673

sebastiantia commented Feb 1, 2025 •

edited

Loading

sebastiantia Feb 1, 2025 •

edited

Loading

codecov bot commented Feb 1, 2025 •

edited

Loading

sebastiantia Feb 3, 2025 •

edited

Loading

sebastiantia Feb 3, 2025 •

edited

Loading