apache / apache/datafusion

Suggest get_expr_planners() return Vec<> rather than &[]

Open
#11,960 2 comments 0 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
9.3k
Forks
2.4k
Avg merge
3d 7h
Merged PRs (30d)
344

Description

[see here](https://github.com/apache/datafusion/blob/7a1a23d5f50cd113613cf54ee5346f0e83d1edc4/datafusion/core/src/execution/session_state.rs#L1501C1-L1504C6)

Basically this implementation uses a private field access on `SessionState` to return the slice, for us normals we have to call `expr_planners() which will give us a [Vec](https://doc.rust-lang.org/nightly/alloc/vec/struct.Vec.html)<[Arc](https://doc.rust-lang.org/nightly/alloc/sync/struct.Arc.html)>

To not share a temporary value in my ContextProvider implementation of `get_expr_planners` I have to hold onto the Vec in my own state which feels clunky compared to other functions like `udfs()` where I'm just returning `self.state.scalar_functions().keys().cloned().collect()·`

I think changing the API to use Vec rather than a slice would be more symmetrical and easier to just defer up to SessionState in my impl

Contributor guide

Open the contributing guide

Research direction

Start with datafusion/core/src/execution/session_state.rs at the referenced get_expr_planners implementation, then inspect the ContextProvider trait and its callers. Check how expr_planners() and related methods such as udfs() expose state. Done means the API shape is consistent with the requested Vec return and affected code and tests still compile and pass.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
api, backend-api-design
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
38/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.