feat/making global context accessible for users #1060
Conversation
|
@timsaucer Hopefully I've implemented what you're looking for in #1045. I went ahead and changed the I think the unit tests could use a little work but hoping to receive your feedback first. |
…ps://github.com/jsai28/datafusion-python into feat-making-global-context-accessible-for-users
python/datafusion/context.py
Outdated
| _global_instance = None | ||
|
|
There was a problem hiding this comment.
Why move this from the rust code to the python code?
python/datafusion/context.py
Outdated
| self.ctx = SessionContextInternal(config, runtime) | ||
|
|
||
| @classmethod | ||
| def global_ctx(cls) -> "SessionContextInternal": |
There was a problem hiding this comment.
I think we want to return the wrapper SessionContext since end users may be getting this and would want to use the associated methods from the wrapper classes. It would mean updating your methods in read accordingly.
| #[classmethod] | ||
| #[pyo3(signature = ())] | ||
| fn _global_ctx(_cls: &Bound<'_, PyType>) -> PyResult<Self> { | ||
| fn global_ctx(_cls: &Bound<'_, PyType>) -> PyResult<Self> { |
There was a problem hiding this comment.
As you have it here where you moved the single entry over to the python side, this method goes unused. I would recommend you leave this line as is, but up in the python code you call this method instead of creating _global_instance
There was a problem hiding this comment.
Thanks for your comments, just to summarize whats needed here:
- Expose the global context (
_global_ctx->global_ctx), which I've currently done. - A python wrapper should be created for the global context (in the
SessionContextclass) which calls the above function and wraps it inSessionContextso that users can still use the other associated methods in this class, but with the global context. This should be a class method so that users dont have to instantiateSessionContextfirst. - The
read_*functions (read_parquet, etc) should use the global context from this python wrapper instead of using the one from the internal implementation.
Am I interpreting this correctly? Sorry if I'm overthinking this 😅. I've updated the PR, currently the test_read_csv and test_read_csv_list tests fail so I'm looking into that.
There was a problem hiding this comment.
Yes, this description looks correct.
|
Can you rebase on main to resolve the conflicts? You'll probably have to handle the updated ruff rules in your code. |
|
Ah looks like I needed to run |
2 participants
Which issue does this PR close?
This addresses #1045 - making global context more accessible.
Rationale for this change
This change allows users to more easily access the global context and register functions with it.
What changes are included in this PR?
The
get_global_ctxmethod is made public and added to the python wrapper code incontext.py. A new class method inSessionContextallows users to retrieve the global context. Changes were also made to theread_*functions inio.pyto use the global context from the python wrapper.Are there any user-facing changes?
Yes, a new class method to
SessionContext. Documentation will be added upon review.