shyjsarah commented on PR #345:
URL: https://github.com/apache/paimon-rust/pull/345#issuecomment-4561776329
> LGTM — clean design, backward-compatible, and solves a real pain point for
non-admin principals.
>
> A few optional suggestions (none blocking):
>
> 1. **Document table-function behavior when `default_db = None`**
(`sql_context.rs:163`)
> `register_table_functions(&self.ctx, &catalog,
default_db.unwrap_or("default"))` still resolves bare table names against
`"default"`. Consider adding a note in the doc comment: "When `default_db =
None`, table functions (`vector_search`, `full_text_search`) require
fully-qualified table names."
> 2. **Guard against `Some("")` at the Rust API level**
> The Python binding handles empty-string → None conversion, but other
Rust callers could pass `Some("")` directly, which would call
`get_database("")` / `create_database("")`. A one-liner validation would make
the API more robust:
> ```rust
> if matches!(default_db, Some("")) {
> return Err(DataFusionError::Plan("default_db must not be
empty".into()));
> }
> ```
> 3. **Optional test**: cover `default_db = None` + bare table function call
(e.g. `vector_search('unqualified', ...)`) to document the expected error path.
>
> All three are nit-level — happy to see them addressed here or in a
follow-up.
Thanks for the review! All three nits addressed in c3971e6:
#1 Doc note added on register_catalog_with_default_db re: the TVF
fallback: bare names in vector_search / full_text_search still resolve against
literal "default"
when default_db = None, so callers must qualify table names in those TVF
calls.
#2 Added the Some("") guard at the top of register_catalog_with_default_db
— returns DataFusionError::Plan("default_db must not be empty; pass None to
skip default-database init"). Doesn't affect the Python binding (already maps
"" → None there); this is purely a footgun guard for raw Rust callers. New unit
test
register_catalog_with_some_empty_string_is_rejected covers it.
#3 Added
register_catalog_with_none_table_function_resolves_bare_name_to_literal_default
— registers with None, runs SELECT * FROM vector_search('bare', ...), asserts
the error names both default and bare (proving the fallback namespace fired
against the MockCatalog's TableNotExist).
Test count: 3 → 5, all green (cargo test -p paimon-datafusion --lib
register_catalog). cargo check -p paimon-datafusion -p pypaimon_rust clean.
cargo fmt applied.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
