feat: allow configuring datafusion-cli history file location#23797
feat: allow configuring datafusion-cli history file location#23797dariocurr wants to merge 2 commits into
Conversation
datafusion-cli hardcoded the interactive shell's rustyline history as the relative path `.history`, so every launch directory silently got its own history file with no way to point it elsewhere. Add a `--history-file <PATH>` flag and a `DATAFUSION_HISTORY_FILE` environment variable (flag takes precedence), keeping `.history` as the default so existing behavior is unchanged. Closes apache#23796.
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
|
Re the cargo-semver-checks note above: this is an expected, intentional minor breaking change. Per the API health policy, could a committer add the |
| pub async fn exec_from_repl( | ||
| ctx: &dyn CliSessionContext, | ||
| print_options: &mut PrintOptions, | ||
| history_file: Option<&Path>, |
There was a problem hiding this comment.
I think it might be better instead of just adding an Option<&Path>,
to declare some struct like perhaps
#[non_exhaustive]
#[derive(Default)]
struct OtherOptions {
history_path: Option<PathBuf>,
}
Or manually implement default to default to ".history" here in the struct than in the function.
Then if any new options are needed which have default values, we don't have to change the abi again to add them, because they should just get picked up by the non_exhaustive/default combo.
There was a problem hiding this comment.
I should clarify that I'm just a drive-by reviewer, not really a maintainer or anything so take my suggestions for what their worth, perhaps the maintainers will have conflicting ideas.
There was a problem hiding this comment.
Took this suggestion and pushed an update: exec_from_repl now takes a ReplOptions struct (in its own new module, datafusion-cli/src/repl_options.rs, mirroring how PrintOptions gets its own module) instead of the raw Option<&Path>.
I kept it as a plain, exhaustively-constructed struct rather than adding #[non_exhaustive] + partial-construction-via-Default, so it stays consistent with PrintOptions's existing convention in this same crate (plain struct, named-field construction at each call site) rather than introducing a second, different pattern for "options" structs. So this doesn't fully close the door on future signature churn the way your suggestion would, but it does give us a dedicated place to add REPL-specific options later without touching PrintOptions, and it let me centralize both the .history default and the DATAFUSION_HISTORY_FILE env var resolution into ReplOptions::default() instead of duplicating that logic across callers. Thanks for the pointer to the pattern either way!
There was a problem hiding this comment.
Good thinking to keep it consistent, we could always add #[non_exhaustive] to both in follow up patch if that is desired.
This update introduces a new `ReplOptions` struct to manage the history file configuration for the DataFusion CLI. The `exec_from_repl` function has been modified to accept `ReplOptions`, allowing users to specify a custom history file path. The default behavior remains unchanged, defaulting to `.history` if no path is provided. This change enhances the flexibility of the CLI's interactive shell experience.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #23797 +/- ##
==========================================
- Coverage 80.71% 80.63% -0.09%
==========================================
Files 1089 1091 +2
Lines 368760 370720 +1960
Branches 368760 370720 +1960
==========================================
+ Hits 297636 298918 +1282
- Misses 53377 53959 +582
- Partials 17747 17843 +96 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Which issue does this PR close?
Rationale for this change
datafusion-cli's interactive REPL hardcodes its rustyline history file as the relative path.history. Because the path is relative, the history file is created (and looked up) in whatever directory the CLI happens to be launched from, so every working directory silently accumulates its own hidden.historyfile with no way to point it somewhere else.What changes are included in this PR?
--history-file <PATH>CLI flag todatafusion-cli, validated at parse time (rejects a path that is an existing directory, or whose parent directory doesn't exist).DATAFUSION_HISTORY_FILEenvironment variable as an alternative way to set it (e.g. for scripted/containerized setups). The--history-fileflag takes precedence over the environment variable..history, relative to the current directory) is unchanged when neither is set, so this is non-breaking.exec_from_replnow takes aReplOptionsstruct (newdatafusion-cli/src/repl_options.rsmodule, mirroring the existingprint_optionsmodule) instead of hardcoding.historytwice.ReplOptions::default()centralizes both the.historyfallback and theDATAFUSION_HISTORY_FILEenv var resolution in one place.datafusion-cli/examples/cli-session-context.rs(the only other caller ofexec_from_repl) anddocs/source/user-guide/cli/usage.mdaccordingly.Are these changes tested?
This is a small, mostly mechanical CLI argument-plumbing change with no new branching logic worth unit-testing in isolation (the added validator mirrors the existing
parse_valid_file/parse_valid_data_dirhelpers, which aren't separately unit-tested either). I manually verified end-to-end via the built binary:.historyin cwd, unchanged behavior)--history-file <path>overrideDATAFUSION_HISTORY_FILE=<path>override--history-filetaking precedence overDATAFUSION_HISTORY_FILEwhen both are set--history-filepointing at a non-existent parent directory fails fast with a clear error instead of a late rustyline errorExisting
datafusion-cliunit tests pass (cargo test -p datafusion-cli --lib --bins).Are there any user-facing changes?
Yes: a new
--history-fileCLI flag andDATAFUSION_HISTORY_FILEenvironment variable, documented indocs/source/user-guide/cli/usage.md. No breaking changes; default behavior is unchanged.