feat: Update `with_commit_info()` API to use `create_one()` API #997

joonyoo181 · 2025-06-02T16:17:26Z

Fixes #762

What changes are proposed in this pull request?

To append Commit Provenance Information to a transaction, the current implementation provides an API called with_commit_info() that takes in the data forengine_commit_info in the form of EngineData. Instead, this PR proposes to change the name into with_engine_commit_info() and take in a HashMap<String, String> as data. This data will be materialized into EngineData during commit() using the new create_one() API.

Additionally, this PR removes old test cases that validate the schema of engine_commit_info.

This PR affects the following public APIs

renaming with_commit_info(EngineData) --> with_engine_commit_info(HashMap<String, String>) in transactions.rs

How was this change tested?

I ran cargo nextest run and passed all tests

…e to use the new APIs

codecov · 2025-06-02T16:20:08Z

Codecov Report

Attention: Patch coverage is 85.71429% with 6 lines in your changes missing coverage. Please review.

Project coverage is 84.56%. Comparing base (717fd48) to head (d2943f0).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
kernel/src/transaction.rs	85.71%	2 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #997      +/-   ##
==========================================
- Coverage   84.68%   84.56%   -0.12%     
==========================================
  Files          92       92              
  Lines       23025    22769     -256     
  Branches    23025    22769     -256     
==========================================
- Hits        19499    19255     -244     
+ Misses       2564     2557       -7     
+ Partials      962      957       -5

☔ 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.

joonyoo181 · 2025-06-02T16:59:57Z

Error::InvalidCommitInfo is no longer being used in the code. Previously, this was thrown if the user passed in an invalid engine_commit_into (e.g. wrong schema) and we had to validate since it was of type EngineData. With the new PR, we don't need to validate since users are constrained by the new APIs to modify CommitInfo. Should this be deleted?

zachschuermann

Awesome start! in an effort to keep changes minimal/targeted, how about we just start with:

(already implemented) with_operation(operation: String)
(new!) with_engine_commit_info(engine_commit_info: HashMap<String, String>)

and create a separate issue/PR for with_operation_parameters(operation_parameters: HashMap<String, String>?

I don't think we will want to make kernel version a public/builder-level API and it isn't clear to me that there's utility in the timestamp case either so let's hold off on those.

kernel/src/transaction.rs

…ion and engine_commit_info

zachschuermann

few comments!

zachschuermann · 2025-06-02T19:35:11Z

kernel/src/transaction.rs

+    // The engine is required to provide commit info before committing the transaction. If the
+    // engine would like to omit engine-specific commit info, it can do so by passing pass an
+    // empty map.


this shouldn't be true anymore right? can we make the API just consume something like an iterable (string, string) and we just (internally) accumulate in a hashmap?

zachschuermann · 2025-06-02T19:36:33Z

kernel/src/transaction.rs

+    // The engine is required to provide commit info before committing the transaction. If the
+    // engine would like to omit engine-specific commit info, it can do so by passing pass an
+    // empty map.
+    pub fn with_commit_info(mut self, engine_commit_info: HashMap<String, String>) -> Self {


let's flag this public API naming and discuss! perhaps we want with_engine_commit_info (since this is the engineCommitInfo field inside the commitInfo action)? cc @nicklan @scovich

I agree, I think with_engine_commit_info would be a better name too, but then I wasn't sure how to determine whether or not CommitInfo has been appended to the transaction. Perhaps we could introduce another call called add_commit_info() that sets a boolean flag?

kernel/src/transaction.rs

zachschuermann · 2025-06-02T19:42:18Z

kernel/src/transaction.rs

+    let hashmap_to_scalar = |hm: Option<&HashMap<String, String>>| -> DeltaResult<Scalar> {
+        match hm {
+            Some(map) => {
+                let pairs = map.iter().map(|(k, v)| (k.clone(), v.clone()));
+                let map_data = MapData::try_new(
+                    MapType::new(DataType::STRING, DataType::STRING, false),
+                    pairs,
+                )?;
+                Ok(Scalar::Map(map_data))
+            }
+            None => {
+                let map_data = MapData::try_new(
+                    MapType::new(DataType::STRING, DataType::STRING, false),
+                    std::iter::empty::<(String, String)>(),
+                )?;
+                Ok(Scalar::Map(map_data))
+            }
+        }
+    };


I wonder if we could/should implement From<HashMap> or rather something like From<Iterator<(K, V)>> for Scalar?

adding @nicklan @scovich

Co-authored-by: Zach Schuermann <[email protected]>

joonyoo181 added 3 commits June 2, 2025 07:26

initial implementation with new test case, modified original test cas…

eec4b1b

…e to use the new APIs

Rebased

a1f3afc

Misunderstood InvalidCommitInfo error, added more comments

e1f80ee

github-actions bot assigned joonyoo181 Jun 2, 2025

github-actions bot added the breaking-change Change that require a major version bump label Jun 2, 2025

joonyoo181 added 4 commits June 2, 2025 09:24

Change to .get_or_insert_with

05007bf

Box up CommitInfo struct to take less memory in Result enum

cdc509e

Replace with Box::default

238a65a

Remove pretty print

80a0ad7

joonyoo181 marked this pull request as ready for review June 2, 2025 16:54

joonyoo181 requested review from zachschuermann, scovich and nicklan and removed request for scovich June 2, 2025 16:54

zachschuermann reviewed Jun 2, 2025

View reviewed changes

kernel/src/transaction.rs Outdated Show resolved Hide resolved

joonyoo181 added 2 commits June 2, 2025 12:06

Remove CommitInfo struct, replace with just string and map for operat…

aee33c1

…ion and engine_commit_info

revise comments

6c855c5

joonyoo181 requested a review from zachschuermann June 2, 2025 19:12

zachschuermann reviewed Jun 2, 2025

View reviewed changes

joonyoo181 and others added 3 commits June 2, 2025 13:00

Update kernel/src/transaction.rs

56537e1

Co-authored-by: Zach Schuermann <[email protected]>

Remove ICT all together

8d98e97

make hm_to_scalar function more idiomatic

d2943f0

joonyoo181 changed the title ~~feat: Replace with_commit_info() API with multiple APIs to provide CommitInfo data and materialize it during commit()~~ feat: Update with_commit_info() API to use create_one() API Jun 5, 2025

zachschuermann mentioned this pull request Jun 24, 2025

feat: add append functions to ffi #962

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: Update `with_commit_info()` API to use `create_one()` API #997

feat: Update `with_commit_info()` API to use `create_one()` API #997

Uh oh!

joonyoo181 commented Jun 2, 2025 •

edited

Loading

Uh oh!

codecov bot commented Jun 2, 2025 •

edited

Loading

Uh oh!

joonyoo181 commented Jun 2, 2025

Uh oh!

zachschuermann left a comment

Uh oh!

Uh oh!

zachschuermann left a comment

Uh oh!

zachschuermann Jun 2, 2025

Uh oh!

zachschuermann Jun 2, 2025

Uh oh!

joonyoo181 Jun 2, 2025

Uh oh!

Uh oh!

Uh oh!

zachschuermann Jun 2, 2025

Uh oh!

joonyoo181 Jun 2, 2025 •

edited

Loading

Uh oh!

Uh oh!

feat: Update with_commit_info() API to use create_one() API #997

Are you sure you want to change the base?

feat: Update with_commit_info() API to use create_one() API #997

Uh oh!

Conversation

joonyoo181 commented Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes are proposed in this pull request?

This PR affects the following public APIs

How was this change tested?

Uh oh!

codecov bot commented Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

joonyoo181 commented Jun 2, 2025

Uh oh!

zachschuermann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zachschuermann left a comment

Choose a reason for hiding this comment

Uh oh!

zachschuermann Jun 2, 2025

Choose a reason for hiding this comment

Uh oh!

zachschuermann Jun 2, 2025

Choose a reason for hiding this comment

Uh oh!

joonyoo181 Jun 2, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zachschuermann Jun 2, 2025

Choose a reason for hiding this comment

Uh oh!

joonyoo181 Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

feat: Update `with_commit_info()` API to use `create_one()` API #997

feat: Update `with_commit_info()` API to use `create_one()` API #997

joonyoo181 commented Jun 2, 2025 •

edited

Loading

codecov bot commented Jun 2, 2025 •

edited

Loading

joonyoo181 Jun 2, 2025 •

edited

Loading