-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Progress Bar #1432
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
Progress Bar #1432
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! Very exciting to have a progress bar soon. Some comments:
|
|
||
| class ProgressBar { | ||
| public: | ||
| explicit ProgressBar(Executor *executor, idx_t show_progress_after, idx_t time_update_bar, bool test = false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than defining a separate class that is used by external libraries, I would prefer if this was integrated directly in the ClientContext (in ExecutePreparedStatement, where the materialized query result is constructed). That seems less brittle and more generally usable (e.g. what happens now if the connection is destroyed while the progress bar is still alive).
I would add a PRAGMA enable_progress_bar and PRAGMA disable_progress_bar that enable or disable the bar respectively (default to off), the timer can also be configured in this way. The shell can then simply run the enable pragma by default on startup.
There are also some #ifdefs that need to be added here. We have DUCKDB_NO_THREADS (see task_scheduler.cpp) that needs to be added here, and DUCKDB_DISABLE_PRINT (see printer.cpp). I would prefer that the print code is moved to printer.cpp entirely so we can keep our print statements centralized.
Note that if DUCKDB_DISABLE_PRINT is set, the progress bar should still work, just not print anything. This way clients that require printing to be disabled (i.e. the R client) can still use the progress bar stuff internally.
src/include/duckdb/execution/operator/persistent/physical_insert.hpp
Outdated
Show resolved
Hide resolved
src/include/duckdb/execution/operator/persistent/physical_update.hpp
Outdated
Show resolved
Hide resolved
|
Thanks, looks great now! |
This PR has a progress bar for base table scans.
If a query takes longer than 2s it automatically starts printing a progress bar.
(Unfortunately, windows was not very fond of 🦆 so I had to use a rather boring | to depict progress :-( )
Of course, other table scans are still missing, I can think of the CSV and Parquet Reader, but maybe others as well, we can list them on task #983 and I'm more than happy to implement it.
Also, this PR only implement the call in the shell, other APIs still have to be implemented (I guess python, R, ...), should add those on the task as well