Skip to content

feat(namespace-dir): implement alter_transaction for DirectoryNamespace#6974

Merged
yanghua merged 6 commits into
lance-format:mainfrom
XuQianJin-Stars:feature/implement-alter-transaction
Jul 1, 2026
Merged

feat(namespace-dir): implement alter_transaction for DirectoryNamespace#6974
yanghua merged 6 commits into
lance-format:mainfrom
XuQianJin-Stars:feature/implement-alter-transaction

Conversation

@XuQianJin-Stars

@XuQianJin-Stars XuQianJin-Stars commented May 28, 2026

Copy link
Copy Markdown
Contributor

Closes #6973

Implement the alter_transaction method for DirectoryNamespace, which
allows altering a transaction's status and properties. This completes one
of the remaining unimplemented methods in the LanceNamespace trait.

Supported actions

  • SetStatusAction: change transaction status (Queued / Running / Succeeded / Failed / Canceled)
  • SetPropertyAction: set transaction properties with Overwrite / Fail / Skip modes
  • UnsetPropertyAction: remove transaction properties with Skip / Fail modes

All actions in a request are applied atomically — either all succeed, or
the operation fails with an appropriate error.

Persistence via sidecar

Since Lance embeds the Transaction into the manifest and treats it as
immutable, we cannot rewrite the transaction file in place. Previously,
alter_transaction only mutated properties in memory and returned them,
so subsequent describe_transaction / alter_transaction calls could
not observe the modifications.

To fix this, alterations are now persisted in a namespace-owned sidecar
file at:

{table}/_alter_transactions/{uuid}.json

This sidecar records the accumulated alterations (status, added /
overwritten properties, tombstoned properties). describe_transaction
and follow-up alter_transaction calls merge the sidecar on top of the
immutable transaction, so the altered state survives across calls.

Reserved keys

uuid, version, read_version, operation, and tag are treated as
reserved keys that cannot be modified via alter_transaction, since they
are derived from the immutable Transaction metadata. They are still
surfaced in the response for the caller's convenience.

Tests added

  • test_alter_transaction_set_status
  • test_alter_transaction_set_property
  • test_alter_transaction_set_property_fail_mode
  • test_alter_transaction_unset_property
  • test_alter_transaction_invalid_status
  • test_alter_transaction_not_found
  • test_alter_transaction_missing_id

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@github-actions

Copy link
Copy Markdown
Contributor

ACTION NEEDED
Lance follows the Conventional Commits specification for release automation.

The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification.

For details on the error please inspect the "PR Title Check" action.

@XuQianJin-Stars XuQianJin-Stars changed the title feat(namespace-dir): implement alter_transaction for DirectoryNamespace feat(namespace-dir): implement alter_transaction for DirectoryNamespace May 28, 2026
@github-actions github-actions Bot added the enhancement New feature or request label May 28, 2026
@codecov

codecov Bot commented May 28, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 86.89840% with 49 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
rust/lance-namespace-impls/src/dir.rs 86.89% 40 Missing and 9 partials ⚠️

📢 Thoughts on this report? Let us know!

@XuQianJin-Stars
XuQianJin-Stars force-pushed the feature/implement-alter-transaction branch from 6a68586 to c70209c Compare May 29, 2026 07:25
@XuQianJin-Stars
XuQianJin-Stars force-pushed the feature/implement-alter-transaction branch from c70209c to 8ffc563 Compare June 15, 2026 03:27
@github-actions github-actions Bot added the A-namespace Namespace impls label Jun 15, 2026
@XuQianJin-Stars
XuQianJin-Stars force-pushed the feature/implement-alter-transaction branch 2 times, most recently from 96425c7 to 949f86a Compare June 24, 2026 02:02
Implement the alter_transaction method for DirectoryNamespace, which allows
altering a transaction's status and properties. This completes one of the
remaining unimplemented methods in the LanceNamespace trait.

The implementation supports:
- SetStatusAction: change transaction status (Queued/Running/Succeeded/Failed/Canceled)
- SetPropertyAction: set transaction properties with Overwrite/Fail/Skip modes
- UnsetPropertyAction: remove transaction properties with Skip/Fail modes

All actions in a request are applied atomically - either all succeed or
the operation fails with an appropriate error.

Tests added:
- test_alter_transaction_set_status
- test_alter_transaction_set_property
- test_alter_transaction_set_property_fail_mode
- test_alter_transaction_unset_property
- test_alter_transaction_invalid_status
- test_alter_transaction_not_found
- test_alter_transaction_missing_id
@XuQianJin-Stars
XuQianJin-Stars force-pushed the feature/implement-alter-transaction branch from 949f86a to 363dc69 Compare June 30, 2026 09:28
Ok(Self::transaction_response(version, &transaction))
}

async fn alter_transaction(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Did you persist or save the changes?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Did you persist or save the changes?

I updated it.

Previously, alter_transaction only mutated properties in memory and
returned them, so subsequent describe_transaction / alter_transaction
calls could not observe the modifications. Since Lance embeds the
Transaction into the manifest and treats it as immutable, we cannot
rewrite the transaction file in place.

Introduce a namespace-owned sidecar file at
{table}/_alter_transactions/{uuid}.json that records the accumulated
alterations (status, added/overwritten properties, tombstoned
properties). describe_transaction and follow-up alter_transaction
calls now merge the sidecar on top of the immutable transaction so the
altered state survives across calls.

Also treat uuid/version/read_version/operation/tag as reserved keys
that cannot be modified, since they are derived from the immutable
Transaction metadata.
- Remove useless conversion .into() on lance_core::Error
- Use RESERVED_KEYS.contains(&key) instead of iter().any()

@yanghua yanghua left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Left some comments.


[features]
default = ["dir-aws", "dir-azure", "dir-gcp", "dir-oss", "dir-huggingface"]
rest = ["dep:reqwest", "dep:serde"]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Do we need to change this file?

Comment thread rust/lance-namespace-impls/src/dir.rs Outdated
create_scalar_table(&namespace, "users").await;
let transaction_id = create_scalar_index(&namespace, "users", "users_id_idx").await;

if let Some(txn_id) = transaction_id {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This style may cause false positives if it is None. It would be better to use expect(...) or assert!(transaction_id.is_some())

}

#[tokio::test]
async fn test_alter_transaction_set_property_fail_mode() {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Using the reserved field uuid as the key results in zero coverage of the mode='Fail' branch.

- Cargo.toml: revert to original by dropping serde derive; sidecar now
  serializes/deserializes via serde_json::Value manually, so no changes
  to Cargo.toml are needed.
- Replace 'if let Some(txn_id) = transaction_id' with expect(...) in five
  alter_transaction tests to avoid silent false-positive passes.
- test_alter_transaction_set_property_fail_mode: use a non-reserved
  'custom_key' (set once, then set again with mode='Fail') so the test
  actually exercises the mode='Fail' branch instead of tripping the
  reserved-key guard first.
Adopt the same pattern used in dir/manifest.rs, which relies on the
built-in Serialize/Deserialize impls for Option<String>,
HashMap<String, String> and HashSet<String> transitively provided by
serde_json.

- Replace hand-rolled serde_json::Value match with a single
  serde_json::from_slice::<Map<String, Value>> plus per-field
  from_value calls.
- Fold the two-step to_json + serde_json::to_vec into a single
  to_json_bytes helper.
- No Cargo.toml changes; still compiles under
  --no-default-features --features dir-aws where serde is optional.

Net effect: -81 lines in dir.rs, all 8 alter_transaction tests still
pass, cargo fmt/clippy clean.

@yanghua yanghua left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

+1

@yanghua
yanghua merged commit 784a876 into lance-format:main Jul 1, 2026
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-namespace Namespace impls enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(namespace-dir): implement alter_transaction for DirectoryNamespace

2 participants