Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pivot builtin #336

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Pivot builtin #336

wants to merge 8 commits into from

Conversation

richiejp
Copy link
Contributor

@richiejp richiejp commented Dec 10, 2024

Convert pivot extension example to a builtin.

Branch is based on recent index changes, the only two relevant commits so far are:

  • sheet/ext_example: Only free file name on failure
  • sheet: Add pivot stub

TODO:

  • Copy pivot implementation into sheet/pivot.c and use builtin functions instead of extension API
  • Make it so user can only select valid columns (only when selecting the column under the cursor)
  • Fix display/memory bug on drill down where parts of the screen are overwritten with characters from somewhere else
  • set data_filename instead of filename
  • tests
  • check what debugging info is printed during normal operation
  • handle header span and blank leading rows (grouping)
  • fix bug on mixed-line-endings.csv where drill-down only shows the header rows and no value
  • fix assertion failure for v after scrolling down (missing header value)
  • fix bug that causes using v on B and C columns to show a single group with just the header as the value
  • hide row number on group-by screen
  • allow extensions to hide row number
  • add error handling for the V command
  • allow SQL expressions to be entered with the V command

@richiejp richiejp force-pushed the pivot-builtin branch 13 times, most recently from ac3719f to ac6859a Compare December 13, 2024 17:05
@liquidaty
Copy link
Owner

@richiejp EXPECT still fails on my machine, starting with make test-sheet-2. Could not just move all its logic into the Makefile? In any case, the failure seems to have something to do with time -p timeout not being supported. Similar for echo -n. Am close to getting it fixed, at least for my machine, but still puzzled why it isn't failing for the CI auto-build

@liquidaty
Copy link
Owner

Oh now I see, the problem is that the script requires timeout to be installed, which it is in the CI environment but is not guaranteed to exist outside of that (such as in my current environment).

Could you review to see if you can find a way, that isn't too much work, to eliminate the dependency on timeout (or any other external utility not required to build)? If it can't be avoided we can re-evaluate after other approaches have been considered. If it must be present, we should check for it in either the configuration or the testing and output an appropriate error message if not found

@liquidaty
Copy link
Owner

I really like this built-in lowercase 'v' (pivot on current column) feature!

Could you also:

  1. make a minor enhancement which is to hide the "Row #" column from being displayed? We will want to make this an option of some sort that an extension can also utilize, probably the best way to simulate this would be to make it something that the extension example can make use of.
  2. add some error handling and more versatility with the V command. For example, a user should be able to enter an expression such as [my column] + 2 if they'd like to group on that expression. However, doing so will instead interpret [my column] + 2 as a literal string instead of a SQL expression. On the flip side, whatever the user enters should be checked to ensure validity; if "my column" does not actually exist, an error message should be displayed

Other future enhancement (for a separate future PR):

  • option to add more aggregation columns to the output (sum, weighted average, etc)

@richiejp
Copy link
Contributor Author

Re the expect issue, what do you think to the sperate utility, does that work for you #341 (comment)

If not i'll try creating a Make only version, but i suspect it will have the same issues as the script

@liquidaty
Copy link
Owner

Re the expect issue, what do you think to the sperate utility, does that work for you

Since we now know why it was failing in my environment, let's put a pause on addressing the test issue and move on with this and other PRs. I will separately take a closer look at #341 and we can go from there.

@liquidaty
Copy link
Owner

handle header span and blank leading rows (drill down)

Shouldn't this be a no-op? The program is generating the output so it can ensure that the header is the first row and data starts on the second row.

@richiejp
Copy link
Contributor Author

richiejp commented Dec 16, 2024

I really like this built-in lowercase 'v' (pivot on current column) feature!

Thanks! I was originally thinking of having an auto-complete, which would still be nice for files with a lot of columns, but it is far more complicated. Perhaps something to consider for the V command in the future.

Shouldn't this be a no-op? The program is generating the output so it can ensure that the header is the first row and data starts on the second row.

Right, thanks, there is a bug (or multiple bugs) which effect the mixed-line-endings.csv file which I thought were related to the header span, but it appears to be something else. I've changed the task descriptions. (EDIT: actually it did need zsv_opts to be passed through because it recreates the table from the original CSV file on drill down).

@richiejp richiejp force-pushed the pivot-builtin branch 10 times, most recently from 4a4cbed to 495045f Compare December 19, 2024 11:35
@richiejp
Copy link
Contributor Author

Presently there are multiple different names for disabling row number generation and we should probably pick one:

  • no_auto_row_num
  • dont_add_row_num
  • hide_row_nums

@richiejp richiejp marked this pull request as ready for review December 19, 2024 14:33
The user can either pivot on the column under the cursor with 'v' or
enter an SQL group-by expression with V
Allows errors to be displayed for longer without them blocking other
status messages after the user has been given time to view them.

Possibly errors should be displayed in a separate buffer or location,
but while we are overloading the status this gives the user chance to
view them.
@richiejp richiejp force-pushed the pivot-builtin branch 2 times, most recently from 6322e78 to e4720fa Compare December 20, 2024 14:30
@richiejp
Copy link
Contributor Author

macosx 13 failed when trying to scroll to the bottom, it seems it almost scrolled the bottom, but it still needs to go down one line. The stage before that which failed took 5s which is a very long time to filter that file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants