Skip to content

fix: set namespace commit handler for LanceDataset.commit#6002

Merged
jackye1995 merged 2 commits intolance-format:mainfrom
jackye1995:managed-versioning-fix
Feb 24, 2026
Merged

fix: set namespace commit handler for LanceDataset.commit#6002
jackye1995 merged 2 commits intolance-format:mainfrom
jackye1995:managed-versioning-fix

Conversation

@jackye1995
Copy link
Copy Markdown
Contributor

@jackye1995 jackye1995 commented Feb 24, 2026

Also added helper function extract_namespace_arc for shared logic

@github-actions github-actions Bot added bug Something isn't working python labels Feb 24, 2026
@github-actions
Copy link
Copy Markdown
Contributor

Code Review

P1: Missing validation for namespace/table_id pair

The commit method changes add namespace and table_id parameters, but there's no validation that both must be provided together. If a user provides only one parameter, managed versioning is silently skipped.

In python/python/lance/dataset.py around line 3535-3561: Add validation before passing to Rust:

if (namespace is None) != (table_id is None):
    raise ValueError("Both 'namespace' and 'table_id' must be provided together.")

This follows the existing pattern in the codebase (see __init__.py:204, __init__.py:231, dataset.py:5758, dataset.py:5844).

Minor note

The namespace extraction logic in dataset.rs:2260-2293 duplicates existing code from the Dataset::new method (lines 611-648). Consider extracting this into a helper function to avoid divergence, though this is not blocking.

@jackye1995 jackye1995 force-pushed the managed-versioning-fix branch from ac85e09 to 5394044 Compare February 24, 2026 21:50
Comment thread python/.cargo/config.toml
"-Wclippy::string_add_assign",
"-Wclippy::string_add",
"-Wclippy::string_lit_as_bytes",
"-Wclippy::string_to_string",
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.

depreacted clippy lint

@jackye1995 jackye1995 force-pushed the managed-versioning-fix branch from 5394044 to 9cd646a Compare February 24, 2026 22:03
The clippy::string_to_string lint was removed in recent Rust versions.
Removing it from the config fixes the CI lint failure.

Also refactored extract_namespace_arc into a shared utility function
in namespace.rs to reduce code duplication.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@jackye1995 jackye1995 force-pushed the managed-versioning-fix branch from 9cd646a to 29d2605 Compare February 24, 2026 22:32
@jackye1995 jackye1995 merged commit 8e86943 into lance-format:main Feb 24, 2026
14 checks passed
wjones127 pushed a commit to wjones127/lance that referenced this pull request Feb 25, 2026
…at#6002)

Also added helper function `extract_namespace_arc` for shared logic

---------

Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
wjones127 pushed a commit to wjones127/lance that referenced this pull request Feb 25, 2026
…at#6002)

Also added helper function `extract_namespace_arc` for shared logic

---------

Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
wjones127 pushed a commit that referenced this pull request Feb 26, 2026
Also added helper function `extract_namespace_arc` for shared logic

---------

Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working python

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants