You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "ratmice (via GitHub)" <gi...@apache.org> on 2026/07/08 12:16:40 UTC
[PR] Move datafusion-cli main_inner impl into the datafusion-cli lib. [datafusion]
ratmice opened a new pull request, #23387:
URL: https://github.com/apache/datafusion/pull/23387
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #23386.
## Rationale for this change
Customizing datafusion-cli with custom udf or calling `register_*` currently requires copying all of `main.rs`,
then dealing with the workspace dependencies required to build it, and change it to link against the `datafusion-cli` library.
If one just wants an example cli without much hassle and maintenance, it is a bit much when you hope to just add a couple lines.
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
## What changes are included in this PR?
The first patch moves a lot of main_inner to a new struct `ClientSession`, that then makes it a lot simpler.
Included is an example of registering a cli-custom-udf.rs.
The second patch just reformats the code with rustfmt to deal with the addition of indentation by moving
things into an `impl CliSession`.
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
## Are these changes tested?
I tested these by running the `datafusion-cli` and running `select hello(1);` in the new example.
Otherwise this doesn't change any existing library code.
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
## Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Move datafusion-cli main_inner impl into the datafusion-cli lib. [datafusion]
Posted by "ratmice (via GitHub)" <gi...@apache.org>.
ratmice commented on PR #23387:
URL: https://github.com/apache/datafusion/pull/23387#issuecomment-4920515142
Alright I *think* this has ended up in a reasonable state (modulo bike shedding), and I can't imagine anything else (crosses fingers), so I believe it should be ready for review
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Move datafusion-cli main_inner impl into the datafusion-cli lib. [datafusion]
Posted by "ratmice (via GitHub)" <gi...@apache.org>.
ratmice commented on PR #23387:
URL: https://github.com/apache/datafusion/pull/23387#issuecomment-4938818330
So, I feel that the major thing left is to somehow make this reviewable in the github UI.
Somewhere I've read it has been suggested to split the code movement and the changes into separate PRs rather than patches to work around how the github UI displays commits as though they were squashed.
I think it is worth trying, so I'm going to close this and try separating into two PRs and clean up the history a bit.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Move datafusion-cli main_inner impl into the datafusion-cli lib. [datafusion]
Posted by "ratmice (via GitHub)" <gi...@apache.org>.
ratmice commented on PR #23387:
URL: https://github.com/apache/datafusion/pull/23387#issuecomment-4919528945
So the only thing I'm a bit uncertain/hesitant about (I'm really not all that familiar with `clap`), is whether this will work
for anyone who wants to extend the arguments using `#[command(flatten)]`, adding their own command line options, but reuse the standard ones. I kind of assume that use case will need additional changes with something like `CliSession::from_clap_args(clap_args: entry_point::Args)`, but uncertain and hesitate to implement it without any trying to actually do that.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Move datafusion-cli main_inner impl into the datafusion-cli lib. [datafusion]
Posted by "ratmice (via GitHub)" <gi...@apache.org>.
ratmice commented on PR #23387:
URL: https://github.com/apache/datafusion/pull/23387#issuecomment-4918278114
After sleeping on it, there are definitely a few things this patch could do better.
1. Hard coding of "DataFusion CLI {DATAFUSION_CLI_VERSION}"
2. Calling `Args::parse` which will exit instead of `Args::try_parse` or `Args::try_parse_from` which will return a `Result`.
3. Probably should just bite the bullet and make `Args` public, extending the existing example to use the command line arguments for configuring it's `CliSessionContext`, instead of calling `SessionContext::new()`.
4. That'll require taking a `CliSessionContext` parameter, and probably taking ownership of `PrintOptions` from a `CliSession`, perhaps the fields of `CliSession` should just be `pub`?.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Move datafusion-cli main_inner impl into the datafusion-cli lib. [datafusion]
Posted by "ratmice (via GitHub)" <gi...@apache.org>.
ratmice closed pull request #23387: Move datafusion-cli main_inner impl into the datafusion-cli lib.
URL: https://github.com/apache/datafusion/pull/23387
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Move datafusion-cli main_inner impl into the datafusion-cli lib. [datafusion]
Posted by "ratmice (via GitHub)" <gi...@apache.org>.
ratmice commented on PR #23387:
URL: https://github.com/apache/datafusion/pull/23387#issuecomment-4914704885
Hmm, I'm uncertain how to make this patch look nice in the github review UI. It looks okay in my local git history viewer, but afaict github really wants to display them squashed?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Move datafusion-cli main_inner impl into the datafusion-cli lib. [datafusion]
Posted by "ratmice (via GitHub)" <gi...@apache.org>.
ratmice commented on PR #23387:
URL: https://github.com/apache/datafusion/pull/23387#issuecomment-4932562540
Alas, I think there is one more thing to do here:
I just tried to use this to set up a custom `SessionStateBuilder` ... `with_type_planner(...)` call,
and It feels like this doesn't allow you to call `SessionContext::new_with_state` for the context returned by
`from_args`. (from there you can still use the `exec_from_repl` whose `CliContext::session_state` can return a custom state. But it just feels off, because it looks like `SessionContext`, and `CliSessionContext` have two different `SessionState` values.
It feels to me that this calls for a builder API...
```
CliSession::builder().session_context(SessionContext::new_with_state(...)).args(args).build()
```
Then if you don't suppy the builder with one via `session_context` It can just build a default one.
anyhow, I'm going to try that.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Move datafusion-cli main_inner impl into the datafusion-cli lib. [datafusion]
Posted by "ratmice (via GitHub)" <gi...@apache.org>.
ratmice commented on PR #23387:
URL: https://github.com/apache/datafusion/pull/23387#issuecomment-4933148927
Well, I pushed what I think is right, but as a noob I've struggled to come up with a good example that actually utilizes the ability to customize session_state, .
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org