Skip to content

Add Cdc action for Change Data Feed #506

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 7 commits into from
Nov 21, 2024
Merged

Add Cdc action for Change Data Feed #506

merged 7 commits into from
Nov 21, 2024

Conversation

@OussamaSaoudi-db
Copy link
Collaborator

What changes are proposed in this pull request?

This PR introduces the Cdc action and its associated CdcVisitor. This action is used for the Change Data Feed table feature.

How was this change tested?

  • Ensure schema projection to cdc works
  • Ensure that the visitor correctly extracts cdc actions.
  • Ensure that metadata parses enableChangeDataFeed table feature.

@OussamaSaoudi-db OussamaSaoudi-db changed the title Cdc visitor Add Cdc action for Change Data Feed Nov 19, 2024
@OussamaSaoudi-db OussamaSaoudi-db marked this pull request as ready for review November 19, 2024 02:04
@codecov
Copy link

codecov bot commented Nov 19, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 89.39394% with 7 lines in your changes missing coverage. Please review.

Project coverage is 80.25%. Comparing base (38eb938) to head (ae27b46).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
kernel/src/actions/visitors.rs 82.92% 0 Missing and 7 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #506      +/-   ##
==========================================
+ Coverage   80.21%   80.25%   +0.04%     
==========================================
  Files          61       61              
  Lines       13342    13403      +61     
  Branches    13342    13403      +61     
==========================================
+ Hits        10702    10757      +55     
+ Misses       2092     2091       -1     
- Partials      548      555       +7

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

🚨 Try these New Features:

@zachschuermann
Copy link
Member

flagging this will be impacted by #481

scovich
scovich approved these changes Nov 19, 2024
View reviewed changes
Copy link
Collaborator

@scovich scovich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks fine, but what is the eventual user surface?
CdcVisitor is not even pub(crate)?

@OussamaSaoudi-db
Copy link
Collaborator Author

@scovich good point, I'll make it mimic the visibility of other visitors/actions.

@OussamaSaoudi-db
Copy link
Collaborator Author

@scovich CdcVisitor has the following visibility attributes, which are the same as the AddVisitor and RemoveVisitor

#[cfg_attr(feature = "developer-visibility", visibility::make(pub))]
#[cfg_attr(not(feature = "developer-visibility"), visibility::make(pub(crate)))]

@OussamaSaoudi-db OussamaSaoudi-db merged commit 5a133d3 into delta-io:main Nov 21, 2024
20 checks passed
zachschuermann
zachschuermann reviewed Nov 21, 2024
View reviewed changes
Copy link
Member

@zachschuermann zachschuermann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a post-merge review but LGTM i think you might have accidentally merged an empty file though

kernel/src/table_changes/table_changes_scan.rs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

accidentally committed?

OussamaSaoudi-db added a commit to OussamaSaoudi-db/delta-kernel-rs that referenced this pull request Nov 21, 2024
@OussamaSaoudi-db @scovich
Add Cdc action for Change Data Feed (delta-io#506)
b4c4505 
<!--
Thanks for sending a pull request!  Here are some tips for you:
1. If this is your first time, please read our contributor guidelines:
https://github.com/delta-incubator/delta-kernel-rs/blob/main/CONTRIBUTING.md
2. Run `cargo t --all-features --all-targets` to get started testing,
and run `cargo fmt`.
  3. Ensure you have added or run the appropriate tests for your PR.
4. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP]
Your PR title ...'.
  5. Be sure to keep the PR description updated to reflect all changes.
-->

## What changes are proposed in this pull request?
<!--
Please clarify what changes you are proposing and why the changes are
needed.
The purpose of this section is to outline the changes, why they are
needed, and how this PR fixes the issue.
If the reason for the change is already explained clearly in an issue,
then it does not need to be restated here.
1. If you propose a new API or feature, clarify the use case for a new
API or feature.
  2. If you fix a bug, you can clarify why it is a bug.
-->
This PR introduces the `Cdc` action and its associated `CdcVisitor`.
This action is used for the Change Data Feed table feature.
<!--
Uncomment this section if there are any changes affecting public APIs:
### This PR affects the following public APIs

If there are breaking changes, please ensure the `breaking-changes`
label gets added by CI, and describe why the changes are needed.

Note that _new_ public APIs are not considered breaking.
-->


## How was this change tested?
<!--
Please make sure to add test cases that check the changes thoroughly
including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please
clarify how you tested, ideally via a reproducible test documented in
the PR description.
-->
- Ensure schema projection to `cdc` works
- Ensure that the visitor correctly extracts cdc actions.
- Ensure that metadata parses enableChangeDataFeed table feature.

---------

Co-authored-by: Ryan Johnson <[email protected]>
@OussamaSaoudi-db OussamaSaoudi-db mentioned this pull request Nov 22, 2024
Merged
OussamaSaoudi-db added a commit that referenced this pull request Nov 22, 2024
@OussamaSaoudi-db @zachschuermann
Revert kernel/src/table_changes/scan.rs (#529)
4a0fad2 
<!--
Thanks for sending a pull request!  Here are some tips for you:
1. If this is your first time, please read our contributor guidelines:
https://github.com/delta-incubator/delta-kernel-rs/blob/main/CONTRIBUTING.md
2. Run `cargo t --all-features --all-targets` to get started testing,
and run `cargo fmt`.
  3. Ensure you have added or run the appropriate tests for your PR.
4. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP]
Your PR title ...'.
  5. Be sure to keep the PR description updated to reflect all changes.
-->

## What changes are proposed in this pull request?
<!--
Please clarify what changes you are proposing and why the changes are
needed.
The purpose of this section is to outline the changes, why they are
needed, and how this PR fixes the issue.
If the reason for the change is already explained clearly in an issue,
then it does not need to be restated here.
1. If you propose a new API or feature, clarify the use case for a new
API or feature.
  2. If you fix a bug, you can clarify why it is a bug.
-->
This removes files that were accidentally added in prior PRs that were
un-reviewed in #505 and #506.

<!--
Uncomment this section if there are any changes affecting public APIs:
### This PR affects the following public APIs

If there are breaking changes, please ensure the `breaking-changes`
label gets added by CI, and describe why the changes are needed.

Note that _new_ public APIs are not considered breaking.
-->


## How was this change tested?
<!--
Please make sure to add test cases that check the changes thoroughly
including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please
clarify how you tested, ideally via a reproducible test documented in
the PR description.
-->

Co-authored-by: Zach Schuermann <[email protected]>
@OussamaSaoudi-db OussamaSaoudi-db mentioned this pull request Dec 14, 2024
Closed
21 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zachschuermann zachschuermann zachschuermann left review comments

@nicklan nicklan nicklan approved these changes

@scovich scovich scovich approved these changes

Assignees

@OussamaSaoudi-db OussamaSaoudi-db

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@OussamaSaoudi-db @zachschuermann @nicklan @scovich