datafusion icon indicating copy to clipboard operation
datafusion copied to clipboard

Use pager and allow configuration via `\pset`

Open djellemah opened this issue 10 months ago • 8 comments

Which issue does this PR close?

  • Closes #15596

Rationale for this change

Explained in issue. Common in other cli programs implementing querying of data.

What changes are included in this PR?

Add a pager (less by default). Check that it exists. Use it for query output.

Allow some manipulation of which pager program is used, and what its command line arguments are, via \pset (following psql usage of \pset)

Allow use of pager to be switched off.

Do not use a pager if stdout is redirected to a non-tty.

Are these changes tested?

Not in unit tests.

Are there any user-facing changes?

Yes. The purpose of these changes is to improve UX.

Not certain if the change from specifying format using \pset csv to using pset format csv is considered a breaking change.

In any case, \pset appears to come from psql, which uses subcommands to specify several areas of output configuration, one of which is pager. datafusion-cli should therefore have used \pset format whatever from the outset.

djellemah avatar Apr 05 '25 18:04 djellemah

@djellemah thank you for working on this! Can we also add some tests to not break these features in the future? and there are some failures in CI

berkaysynnada avatar Apr 06 '25 12:04 berkaysynnada

This PR has several CI failures so marking as a draft while they are addressed. (I do this to make it easier to see what PRs are waiting on review)

alamb avatar Apr 06 '25 13:04 alamb

I've added commits to address the CI failures.

Of course you're welcome to add some tests to not break these features in the future :-)

djellemah avatar Apr 06 '25 19:04 djellemah

Thank you for this contribution @djellemah -- it is a very nice feature to have

I've added commits to address the CI failures.

Of course you're welcome to add some tests to not break these features in the future :-)

As documented in the contribution guidelines https://datafusion.apache.org/contributor-guide/testing.html#testing

All new features should have test coverage

Thus I agree with @berkaysynnada that this feature should have tests before we merge it.

Since maintainer bandwidth is the most limited resource in this project, I suspect we might be waiting a while if we wait for a maintainer help write tests

alamb avatar Apr 07 '25 13:04 alamb

On f36029c99b14154845d444f0d279147cc72e9ffc, I added a test to verify the CLI execution on the pty. However, testing key presses like ‘w’ or ‘s’ in the test is tricky, and there’s additional complexity with waiting for other tests to finish.

Weijun-H avatar Apr 10 '25 08:04 Weijun-H

Seems to me there's a confluence of several factors here:

  • testing this kind of functionality is not simple.

  • it's user facing, so if it breaks somebody will notice and say something.

  • it's not core functionality so the probability of erroneous behaviors causing severe consequences is fairly low.

  • there is limited maintainer bandwidth.

  • there is limited contributor bandwidth.

In my opinion, the decision matrix here is not quite the same as one would apply to core functionality.

djellemah avatar Apr 13 '25 01:04 djellemah

Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.

github-actions[bot] avatar Jun 13 '25 02:06 github-actions[bot]

Merge conflict introduced by f40e0db161 on 02-may-2025

djellemah avatar Jun 13 '25 13:06 djellemah

Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.

github-actions[bot] avatar Aug 15 '25 02:08 github-actions[bot]