Skip to content

Conversation

@majin1102
Copy link
Contributor

Close #5781

@github-actions github-actions bot added enhancement New feature or request python labels Jan 22, 2026
@github-actions
Copy link
Contributor

Code Review

P0: Breaking change to default features

The change to python/Cargo.toml modifies the default features:

[features]
-default = []
+default = ["rest", "rest-adapter"]

This is a breaking change that will force all Python builds to include REST dependencies by default. This should either:

  1. Be reverted to keep default = [] (recommended)
  2. Be documented as an intentional breaking change with justification

If the intent is to make REST features available for the PyNamespaceSessionBuilder, consider making this opt-in via a separate feature flag or documenting why these features are required by default.

P1: Incomplete docstrings in Python bindings

The NamespaceSession class and PyNamespaceSessionBuilder methods would benefit from docstrings on the Rust side (via #[pyo3(text_signature = "...")]) or more complete Python documentation, especially for the config parameter supported keys.

Minor observations (not blocking)

  • The test coverage is good with parameterized join scenarios, aggregations, CTEs, and error cases
  • The sql() method returns a combined pyarrow.Table which is a reasonable design choice
  • The to_lance_namespace helper function correctly handles both low-level Rust types and high-level Python wrappers

Overall the implementation looks correct. The main concern is the default features change.

@codecov
Copy link

codecov bot commented Jan 22, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@majin1102 majin1102 marked this pull request as draft January 22, 2026 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request python

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Provide sql API integrated with namespace in Python

1 participant