Skip to content
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.

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

move transaction module into actions/ and rename to set_transaction #386

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

zachschuermann
Copy link
Collaborator

@zachschuermann zachschuermann commented Oct 9, 2024

Prefactor for write-path work. this does three things

  1. move transaction into actions module
  2. rename module to set_transaction and Transaction to SetTransaction
  3. add commitInfo to actions schema and define a commit info object which (for now) just includes kernel version

@zachschuermann zachschuermann self-assigned this Oct 9, 2024
Copy link

codecov bot commented Oct 9, 2024

Codecov Report

Attention: Patch coverage is 96.96970% with 1 line in your changes missing coverage. Please review.

Project coverage is 77.69%. Comparing base (6b0df5d) to head (1fdb098).

Files with missing lines Patch % Lines
kernel/src/actions/visitors.rs 91.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #386   +/-   ##
=======================================
  Coverage   77.68%   77.69%           
=======================================
  Files          49       49           
  Lines       10084    10087    +3     
  Branches    10084    10087    +3     
=======================================
+ Hits         7834     7837    +3     
  Misses       1805     1805           
  Partials      445      445           

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

Copy link
Collaborator

@nicklan nicklan left a comment

Choose a reason for hiding this comment

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

mostly lgtm, just a couple small nits

kernel/src/actions/mod.rs Show resolved Hide resolved
kernel/src/actions/mod.rs Outdated Show resolved Hide resolved
kernel/src/actions/mod.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@scottsand-db scottsand-db left a comment

Choose a reason for hiding this comment

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

LGTM. This brings the name of the "transaction" ("txn") Delta protocol action to be en-par with how Delta-Kernel-API and Delta-Spark refer to it as in the code, a SetTransaction. That name is a signification improvement over "Transaction", and the unification between the kernels is also great.

zachschuermann and others added 2 commits October 9, 2024 15:53
Co-authored-by: Nick Lanham <nicklan@users.noreply.github.com>
Co-authored-by: Nick Lanham <nicklan@users.noreply.github.com>
@github-actions github-actions bot added the breaking-change Change that will require a version bump label Oct 9, 2024
Copy link
Collaborator

@nicklan nicklan left a comment

Choose a reason for hiding this comment

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

One last thing I noticed that needs to be renamed. You'll also need to fix the name in inspect-table/src/main.rs due to #190

kernel/src/actions/mod.rs Show resolved Hide resolved
kernel/src/actions/visitors.rs Show resolved Hide resolved
Copy link
Collaborator

@nicklan nicklan left a comment

Choose a reason for hiding this comment

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

lgtm!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Change that will require a version bump
Development

Successfully merging this pull request may close these issues.

3 participants