Skip to content

cli: Fix datafusion-cli hint edge cases#20609

Merged
goldmedal merged 1 commit intoapache:mainfrom
comphead:spark
Mar 2, 2026
Merged

cli: Fix datafusion-cli hint edge cases#20609
goldmedal merged 1 commit intoapache:mainfrom
comphead:spark

Conversation

@comphead
Copy link
Copy Markdown
Contributor

@comphead comphead commented Feb 27, 2026 •
edited
Loading

Which issue does this PR close?

  • Closes #.

Rationale for this change

The hint show condition relied on just checking if the input string is empty which caused visual issues like

image

What changes are included in this PR?

  helper.rs:                                                
  - Added a show_hint: Cell<bool> field to CliHelper. Uses Cell for interior mutability since hint() takes &self.                                                                                          
  - hint() now sets show_hint to false the moment line is non-empty. The hint only displays when show_hint is still true and the line is empty — i.e., only on the fresh prompt before any key is pressed. 
  - validate() calls reset_hint() after validation so the hint reappears on the next prompt after submission.                                                                                              
  - Added pub fn reset_hint(&self) for external reset.                                                                                                                                                     
                                                            
  exec.rs:
  - On Ctrl+C (ReadlineError::Interrupted), calls rl.helper().unwrap().reset_hint() so the next prompt still shows the hint.

Are these changes tested?

Are there any user-facing changes?

comphead
comphead commented Feb 27, 2026
View reviewed changes
Comment thread datafusion-cli/src/helper.rs
return true;
}
false
DFParser::parse_sql(&sql).is_ok_and(|stmts| {
Copy link
Copy Markdown
Contributor Author

@comphead comphead Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its just simplification

@comphead comphead changed the title cli: Fix cli hint edge cases cli: Fix datafusion-cli hint edge cases Feb 28, 2026
goldmedal
goldmedal approved these changes Mar 2, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@goldmedal goldmedal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @comphead. Look great 👍

@goldmedal goldmedal added this pull request to the merge queue Mar 2, 2026
Merged via the queue into apache:main with commit 43584ca Mar 2, 2026
30 checks passed
@comphead
Copy link
Copy Markdown
Contributor Author

comphead commented Mar 2, 2026

Thanks folks

@alamb alamb mentioned this pull request Mar 10, 2026
Closed
26 tasks
alamb pushed a commit to alamb/datafusion that referenced this pull request Mar 11, 2026
@comphead @alamb
cli: Fix datafusion-cli hint edge cases (apache#20609)
96d2356 
## 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 apache#123` indicates that this PR will close issue apache#123.
-->

- Closes #.

## Rationale for this change

The hint show condition relied on just checking if the input string is
empty which caused visual issues like

<img width="428" height="236" alt="image"
src="https://github.com/user-attachments/assets/2b3a5c61-f910-4313-8844-35e1b0475692"
/>



<!--
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?

```
  helper.rs:                                                
  - Added a show_hint: Cell<bool> field to CliHelper. Uses Cell for interior mutability since hint() takes &self.                                                                                          
  - hint() now sets show_hint to false the moment line is non-empty. The hint only displays when show_hint is still true and the line is empty — i.e., only on the fresh prompt before any key is pressed. 
  - validate() calls reset_hint() after validation so the hint reappears on the next prompt after submission.                                                                                              
  - Added pub fn reset_hint(&self) for external reset.                                                                                                                                                     
                                                            
  exec.rs:
  - On Ctrl+C (ReadlineError::Interrupted), calls rl.helper().unwrap().reset_hint() so the next prompt still shows the hint.
```

<!--
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?

<!--
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.
-->
@alamb alamb mentioned this pull request Mar 11, 2026
Merged
alamb added a commit that referenced this pull request Mar 12, 2026
@alamb @comphead
[branch-53] cli: Fix datafusion-cli hint edge cases (#20609) (#20887)
ab7507f 
- Part of #19692

This PR:
- Backports #20609 from
@comphead to the branch-53 line

Co-authored-by: Oleks V <comphead@users.noreply.github.com>
de-bgunter pushed a commit to de-bgunter/datafusion that referenced this pull request Mar 24, 2026
@comphead @de-bgunter
cli: Fix datafusion-cli hint edge cases (apache#20609)
a678843 
## 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 apache#123` indicates that this PR will close issue apache#123.
-->

- Closes #.

## Rationale for this change

The hint show condition relied on just checking if the input string is
empty which caused visual issues like

<img width="428" height="236" alt="image"
src="https://github.com/user-attachments/assets/2b3a5c61-f910-4313-8844-35e1b0475692"
/>



<!--
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?

```
  helper.rs:                                                
  - Added a show_hint: Cell<bool> field to CliHelper. Uses Cell for interior mutability since hint() takes &self.                                                                                          
  - hint() now sets show_hint to false the moment line is non-empty. The hint only displays when show_hint is still true and the line is empty — i.e., only on the fresh prompt before any key is pressed. 
  - validate() calls reset_hint() after validation so the hint reappears on the next prompt after submission.                                                                                              
  - Added pub fn reset_hint(&self) for external reset.                                                                                                                                                     
                                                            
  exec.rs:
  - On Ctrl+C (ReadlineError::Interrupted), calls rl.helper().unwrap().reset_hint() so the next prompt still shows the hint.
```

<!--
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?

<!--
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.
-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@goldmedal goldmedal goldmedal approved these changes

@Jefffrey Jefffrey Jefffrey approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@comphead @goldmedal @Jefffrey