feat(ibis): introduce the api to get the details of specfic function#1374
feat(ibis): introduce the api to get the details of specfic function#1374douenergy merged 1 commit intoCanner:mainfrom
Conversation
WalkthroughAdds a v3 API endpoint to fetch metadata for a named function and a corresponding test, plus a new public PySessionContext API that queries information_schema to return function details mapped to PyRemoteFunction. Changes
Sequence DiagramsequenceDiagram
participant Client
participant API as API Endpoint (v3/connector)
participant Session as PySessionContext
participant DB as information_schema
Client->>API: GET /{data_source}/function/{function_name}
API->>API: start tracing span
API->>Session: get_available_function(function_name)
Session->>DB: SQL query on information_schema for function metadata
DB-->>Session: rows (name, param_names, param_types, return_type, description)
Session->>Session: map rows -> PyRemoteFunction list
Session-->>API: Vec<PyRemoteFunction>
API-->>Client: 200 OK + JSON payload
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ibis-server/tests/routers/v3/connector/postgres/test_functions.py (1)
73-86: Good coverage; consider loosening assertion if metadata format may evolveThe test correctly exercises the new
/function/divendpoint and validates the expected payload. If you expect the underlying type formatting or description strings to evolve, you might want to assert on a subset of fields (e.g.,name,function_type, key parts ofparam_types) instead of strict equality on the full object to reduce brittleness; otherwise this is fine as-is.ibis-server/app/routers/v3/connector.py (1)
445-465: Align endpoint description and behavior with/functionssemanticsThe verification confirms all three concerns:
Description mismatch: The description says "get the available function list…" but should reflect that this endpoint returns details for a specific function.
Whitelist inconsistency (confirmed): The
/functionsendpoint (lines 420–443) checksis_white_listand either reads the CSV whitelist or callssession_context.get_available_functions(). However, the/function/{name}endpoint (lines 445–465) always callssession_context.get_available_function()and ignores the whitelist configuration entirely. If the whitelist is meant to gate which functions are exposed, consider:
- Enforcing the same whitelist check here (e.g., returning
404or403for non-whitelisted functions), or- Documenting that
/function/{name}always reflects the underlying information_schema, independent of whitelist configuration.Not-found semantics: An unknown
function_namecurrently returns200with an empty list. Consider returning404if API consumers expect a "not found" signal.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ibis-server/app/routers/v3/connector.py(1 hunks)ibis-server/tests/routers/v3/connector/postgres/test_functions.py(1 hunks)wren-core-py/src/context.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
wren-core-py/src/context.rs (3)
wren-core/core/src/mdl/function/remote_function.rs (11)
from(188-198)from(272-281)from(335-344)name(226-228)name(289-291)name(352-354)param_types(55-58)return_type(234-236)return_type(297-299)from_str(88-95)from_str(128-138)wren-core-py/src/remote_functions.rs (2)
from(65-99)from(103-139)wren-core-py/src/errors.rs (10)
from(25-27)from(31-33)from(37-39)from(43-45)from(49-51)from(55-64)from(68-70)from(74-76)from(80-82)from(86-88)
ibis-server/app/routers/v3/connector.py (7)
ibis-server/app/model/data_source.py (1)
DataSource(60-214)ibis-server/app/dependencies.py (1)
get_wren_headers(26-34)ibis-server/app/util.py (2)
build_context(142-145)set_attribute(148-155)ibis-server/app/config.py (1)
get_remote_function_list_path(61-70)ibis-server/app/mdl/core.py (1)
get_session_context(7-10)wren-core-py/src/remote_functions.rs (1)
to_dict(47-61)wren-core-py/src/context.rs (1)
get_available_function(238-253)
ibis-server/tests/routers/v3/connector/postgres/test_functions.py (1)
ibis-server/tests/conftest.py (1)
client(18-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: ci
- GitHub Check: ci
- GitHub Check: test
d4c0c15 to
5602189
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
wren-core-py/src/context.rs (1)
243-255: SQL injection vulnerability already flagged.The
function_nameparameter flows intoget_registered_functionwhere it is directly interpolated into SQL (line 457). This was already flagged in a previous review.Minor: Consider simplifying
.map(|f| PyRemoteFunction::from(f))to.map(Into::into)for consistency with the existing pattern at line 238.
🧹 Nitpick comments (1)
ibis-server/tests/routers/v3/connector/postgres/test_functions.py (1)
73-87: Test covers the happy path correctly.The test validates the expected response structure for the new endpoint. Consider adding a test for a non-existent function name to verify the endpoint returns an empty list (or appropriate error) in that case.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ibis-server/app/routers/v3/connector.py(1 hunks)ibis-server/tests/routers/v3/connector/postgres/test_functions.py(1 hunks)wren-core-py/src/context.rs(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-05T02:27:29.829Z
Learnt from: goldmedal
Repo: Canner/wren-engine PR: 1161
File: ibis-server/app/routers/v3/connector.py:78-83
Timestamp: 2025-05-05T02:27:29.829Z
Learning: The row-level access control implementation in Wren Engine filters headers with the prefix `X_WREN_VARIABLE_PREFIX` in `EmbeddedEngineRewriter.get_session_properties` and validates session property expressions in `access_control.rs` to ensure they only contain literal values, preventing SQL injection.
Applied to files:
wren-core-py/src/context.rs
🧬 Code graph analysis (3)
ibis-server/tests/routers/v3/connector/postgres/test_functions.py (1)
ibis-server/tests/conftest.py (1)
client(18-23)
wren-core-py/src/context.rs (2)
wren-core-py/src/errors.rs (11)
from(26-28)from(32-34)from(38-40)from(44-46)from(50-52)from(56-65)from(69-71)from(75-77)from(81-83)from(87-89)from(93-95)wren-core-py/src/remote_functions.rs (2)
from(65-99)from(103-139)
ibis-server/app/routers/v3/connector.py (5)
ibis-server/app/dependencies.py (1)
get_wren_headers(26-34)ibis-server/app/util.py (2)
build_context(142-145)set_attribute(148-155)ibis-server/app/config.py (2)
get_config(103-104)get_remote_function_list_path(61-75)wren-core-py/src/remote_functions.rs (1)
to_dict(47-61)wren-core-py/src/context.rs (1)
get_available_function(243-255)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: ci
- GitHub Check: ci
- GitHub Check: test
🔇 Additional comments (3)
wren-core-py/src/context.rs (2)
32-34: LGTM!The new imports for
GenericByteArrayandGenericStringTypeare correctly added to support the array handling into_string_vec.
501-508: LGTM!The
to_string_vechelper correctly converts the Arrow string array toVec<Option<String>>, properly handling null values via the iterator'sOptionreturn.ibis-server/app/routers/v3/connector.py (1)
450-465: Implementation follows existing patterns correctly.The endpoint correctly initializes the session context and retrieves function details. The return type (list) appropriately handles potential function overloads with the same name but different signatures.
Consider whether a 404 response is needed when the function is not found (currently returns empty list
[]). If the current behavior is intentional, a brief inline comment would help clarify.
This PR introduces an API to get the details of the specific function. The information will include the parameter types and their name if defined.
API Endpoint
Response
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.