feat(python): add get_primary_keys() method to Schema class #436
Conversation
There was a problem hiding this comment.
Pull request overview
Adds Python-side introspection for Schema primary keys to close #384, aligning the Python bindings with existing functionality in the Rust core and the Python TableInfo API.
Changes:
- Add
Schema.get_primary_keys()to the PyO3 Python bindings (bindings/python/src/metadata.rs). - Add an integration assertion verifying
Schema.get_primary_keys()returns the configured keys (bindings/python/test/test_admin.py).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| bindings/python/src/metadata.rs | Exposes primary key column names from the wrapped core Schema via get_primary_keys(). |
| bindings/python/test/test_admin.py | Adds a test assertion that a constructed Schema returns the expected primary key list. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn get_primary_keys(&self) -> Vec<String> { | ||
| self.__schema | ||
| .primary_key() | ||
| .map(|pk| pk.column_names().to_vec()) | ||
| .unwrap_or_default() | ||
| } |
There was a problem hiding this comment.
New Python API method Schema.get_primary_keys() is added in the PyO3 bindings, but the package type stubs (bindings/python/fluss/__init__.pyi) still define Schema without this method. This will leave typing users (mypy/IDE autocomplete) out of sync with runtime behavior; please update the stub to include def get_primary_keys(self) -> List[str]: ... for Schema.
There was a problem hiding this comment.
New Python API method
Schema.get_primary_keys()is added in the PyO3 bindings, but the package type stubs (bindings/python/fluss/__init__.pyi) still defineSchemawithout this method. This will leave typing users (mypy/IDE autocomplete) out of sync with runtime behavior; please update the stub to includedef get_primary_keys(self) -> List[str]: ...forSchema.
Added
| @@ -220,7 +220,12 @@ impl Schema { | |||
| .collect() | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Schema.get_primary_keys() is missing a Rust doc comment (/// ...) while the adjacent Schema methods have doc comments. Adding one keeps the generated Python docstrings/help output consistent across the API surface.
| /// Get primary key column names |
There was a problem hiding this comment.
Schema.get_primary_keys()is missing a Rust doc comment (/// ...) while the adjacent Schema methods have doc comments. Adding one keeps the generated Python docstrings/help output consistent across the API surface.Suggested change
/// Get primary key column names
Added
fresh-borzoni
left a comment
fresh-borzoni
left a comment
There was a problem hiding this comment.
@XiaoHongbo-Hope Ty for the PR, LGTM
charlesdong1991
left a comment
charlesdong1991
left a comment
There was a problem hiding this comment.
LTGM!
Nitpick: not sure if it is worth it to adding a small test to check it returns empty list when no primary key is present
Thanks, added ut |
leekeiabstraction
left a comment
leekeiabstraction
left a comment
There was a problem hiding this comment.
TY for the PR. Left a comment. PTAL!
| def get_column_names(self) -> List[str]: ... | ||
| def get_column_types(self) -> List[str]: ... | ||
| def get_columns(self) -> List[Tuple[str, str]]: ... | ||
| def get_primary_keys(self) -> List[str]: ... |
There was a problem hiding this comment.
Should the website's api reference documentation be updated as well?
|
@XiaoHongbo-Hope Hi, I already push a commit to add the doc. Merging.. |
Thanks! Sorry for missing above comment for flood of CI messages with my github email... |
Purpose
Linked issue: close #384
Brief change log
Tests
API and Format
Documentation