Skip to content

feat: Allow for returning post commit hooks from transactions #996

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

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

Conversation

nicklan
Copy link
Collaborator

@nicklan nicklan commented May 30, 2025

What changes are proposed in this pull request?

Small pre-factor that includes a Vec of PostCommitActions when returning a Committed result from a Transaction.

Each action is just an enum where each variant has enough info for the engine to perform the requested action. Engines are expected to use existing APIs to perform these actions.

This PR affects the following public APIs

Transaction::commit: Now the Committed variant includes a Vec of actions.

How was this change tested?

No actual functionality. That it compiles is enough.

Copy link

codecov bot commented May 30, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.69%. Comparing base (60d0944) to head (2f1a5fe).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #996   +/-   ##
=======================================
  Coverage   84.68%   84.69%           
=======================================
  Files          92       92           
  Lines       23015    23018    +3     
  Branches    23015    23018    +3     
=======================================
+ Hits        19491    19494    +3     
  Misses       2563     2563           
  Partials      961      961           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions github-actions bot added the breaking-change Change that require a major version bump label May 30, 2025
Copy link
Collaborator

@roeap roeap left a comment

Choose a reason for hiding this comment

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

Overall looks good - clean and simple!

I do believe we may have to consider the current checkpointing API which could also show up as a post commit hook? At least I believe kernel should inform the engine that its time to do a checkpoint based on table settings.

There we ask engine to write a data iterator to a given file. The iterator does collect some stats, so we need to hand it back to kernel to finalise the checkpoint.

This could be hidden behind this API if we introduce a method write_parquet similar to write_json and ask engine to give us that iterator back in the result?

@nicklan nicklan requested a review from roeap June 3, 2025 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Change that require a major version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants