fix(datafusion): keep hot runs in one session#833
fix(datafusion): keep hot runs in one session#833kumarUjjawal wants to merge 1 commit intoClickHouse:mainfrom
Conversation
|
@qoega If I could get your eyes on this? |
|
Hi @kumarUjjawal -- thank you. How did you test this change? Maybe it would be a good exercise to try and get DataFusion 53 numbers with this script? That way we both double check the script works and update the results for DF 53 |
|
I can try and help do this (run for DF 53) if needed |
I asked Codex to build a small bash script for local test. It used a fake
If you could help run the DF 53 bench that would be great. 🙏 |
|
Will try later this week -- basically we need to run it on aws. |
|
Thank you! |
|
I am somewhat overwhelmed this week with other things. I'll see what I can do later. Maybe @pmcgleenon has some time to review / rerun the numbers (he used to be our star number runner 🦾 ) |
|
I'm running the benchmarks now, I'll test this change out. |
|
@kumarUjjawal this seems to also be true for the non-partitioned version. |
|
One weird thing that this change has caused consistently (for both partitioned and single) - the last iteration of q23 (which has a dynamic filter) becomes a lot slower! Something like |
|
Is there some deeper issue here as q23 shouldn't behave like this right? |
|
yeah definitely, might be already fixed upstream. For now I'm still running everything with the existing scripts, but once I get through that I'll go back and check if it reproduces with DataFusion's latest |
I think they are very dependent on timing and when the file is opened vs when the top value is seen If the last value is seen quickly enough then it can skip several files entirely, but if the top value isnt' seen fast enough then the files get processed |
rschu1ze
left a comment
There was a problem hiding this comment.
I think that for symmetry, it would make sense to apply the same changes to datafusion/run.sh.
|
|
||
| echo $1 | ||
| cat queries.sql | while read -r query; do | ||
| while IFS= read -r query || [ -n "$query" ]; do |
There was a problem hiding this comment.
It is not clear what IFS means.
| echo 3 | sudo tee /proc/sys/vm/drop_caches >/dev/null | ||
|
|
||
| echo "$query" > /tmp/query.sql | ||
| QUERY_FILE="${TMP_DIR}/query-${QUERY_NUM}.sql" |
There was a problem hiding this comment.
There is too much copying going on here, at least for my taste, and there are no code comments that explain the rationale. Please try to simplify.
Summary
This fixes the partitioned DataFusion runner so hot runs are really hot. See apache/datafusion#21696
Before this change, the script started a new
datafusion-cliprocess for every try. That made each try use a cold process, so tries 2 and 3 did not keep the warmed metadata cache.What changed
create.sqland the same query 3 timesdatafusion-clisessionElapsedlines fromcreate.sqlnullinstead of mixing in setup timingsThis follows the same main idea used by the DuckDB partitioned runner: one process per query, with repeated runs inside that process.
Thank you for review 🙏
Thank You for Your Contribution!
We appreciate your effort and contribution to the project. To ensure that your Pull Request (PR) adheres to our guidelines, please ensure to review the rules mentioned in our contribution guidelines:
ClickHouse/ClickBench Contribution Rules
Thank you for your attention to these details and for helping us maintain the quality and integrity of the project.