Skip to content

fix: various bugs to namespace access#5996

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

fix: various bugs to namespace access#5996
jackye1995 merged 2 commits intolance-format:mainfrom
jackye1995:fix-ext

Conversation

@jackye1995
Copy link
Copy Markdown
Contributor

  1. fix python binding short-circuit for DirectoryNamespace and RestNamespace binding
  2. fix local file system access to LanceFileSession for namespace-based access
  3. fix propagating storage options to __manifest table in DirectoryNamespace

@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 for PR #5996

Summary

This PR fixes three bugs in namespace access: Python binding short-circuit for wrapper classes, local file system Writer::shutdown, and storage options propagation in DirectoryNamespace.


P1: Missing Tests

Per the project guidelines: "We do not merge code without tests."

The PR fixes three bugs but doesn't include tests for:

  1. The namespace extraction logic for Python wrapper classes (DirectoryNamespace, RestNamespace in namespace.py)
  2. The Writer::shutdown fix for local file systems
  3. The storage options propagation to describe_table / describe_table_version

Please add tests that verify these fixes work correctly.


P1: Code Duplication in dataset.rs

The namespace extraction logic (~40 lines) is duplicated in two places:

  • Dataset::open (lines ~604-645)
  • get_write_params (lines ~3205-3246)

This creates a maintenance burden—if one location is updated but not the other, bugs can occur. Consider extracting this into a helper function:

fn extract_namespace_arc(py: Python<'_>, ns: &Bound<'_, PyAny>) -> PyResult<Arc<dyn LanceNamespace>> {
    // ... extraction logic
}

P1: Fragile Type Name Comparison

Using string comparison for type names is fragile:

if type_name == "DirectoryNamespace" {

A class with the same name in a different module could match incorrectly. Consider also checking the module path:

let qualified_name = ns.get_type().qualname().ok();
// or check the module
let module = ns.get_type().module().ok();

Notes (Non-Blocking)

  • The Writer::shutdown fix in file.rs is correct—using fully-qualified trait syntax ensures the Writer trait's shutdown method is called instead of AsyncWriteExt::shutdown.

  • The DatasetBuilder changes in dir.rs correctly propagate storage options and session for S3 with custom endpoints.

@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 24, 2026

Codecov Report

❌ Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
rust/lance-namespace-impls/src/dir.rs 0.00% 3 Missing ⚠️

📢 Thoughts on this report? Let us know!

@jackye1995 jackye1995 merged commit 50d29f7 into lance-format:main Feb 24, 2026
29 checks passed
wjones127 pushed a commit to wjones127/lance that referenced this pull request Feb 25, 2026
1. fix python binding short-circuit for DirectoryNamespace and
RestNamespace binding
2. fix local file system access to LanceFileSession for namespace-based
access
3. fix propagating storage options to __manifest table in
DirectoryNamespace
wjones127 pushed a commit to wjones127/lance that referenced this pull request Feb 25, 2026
1. fix python binding short-circuit for DirectoryNamespace and
RestNamespace binding
2. fix local file system access to LanceFileSession for namespace-based
access
3. fix propagating storage options to __manifest table in
DirectoryNamespace
wjones127 pushed a commit that referenced this pull request Feb 26, 2026
1. fix python binding short-circuit for DirectoryNamespace and
RestNamespace binding
2. fix local file system access to LanceFileSession for namespace-based
access
3. fix propagating storage options to __manifest table in
DirectoryNamespace
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.

2 participants