Skip to content

Conversation

@incfly
Copy link
Contributor

@incfly incfly commented Sep 21, 2025

Signed-off-by: Jianfei Hu hujianfei258@gmail.com

Resolves #63666 Implemented the idea from #63667 (comment)

  • Adds a method mergeToViaIndexFilter to FixedHashMap
  • Create multiple worker during pipeline, 1:1 to the max_threads. Each thread uses their own thread id to invoke mergeToViaIndexFilter to avoid race condition on the fixed hash map.

We can see noticeable performance improvement for the original query, which uses fixed hash map during aggregation.

image

Changelog category (leave one):

  • Performance Improvement

Changelog entry

When a query uses fixed hash map for aggregation state(group by a small integer), ClickHouse would merge the aggregation state in parallel to speed up the query. Resolves #63666

Signed-off-by: Jianfei Hu <hujianfei258@gmail.com>
Signed-off-by: Jianfei Hu <hujianfei258@gmail.com>
some notes https://pastila.nl/?029696d4/e8c50f64d3799b6e0cc1f9da7770dfe7#w6qdcZYrJ67XJhbWGV8TEw==
also pipeline how coordinate, state transition is key.

almost like still sequential for single level.

Signed-off-by: Jianfei Hu <hujianfei258@gmail.com>
Signed-off-by: Jianfei Hu <hujianfei258@gmail.com>
… via prepareBlockAndFillSingleLevel

Signed-off-by: Jianfei Hu <hujianfei258@gmail.com>
Signed-off-by: Jianfei Hu <hujianfei258@gmail.com>
Signed-off-by: Jianfei Hu <hujianfei258@gmail.com>
Signed-off-by: Jianfei Hu <hujianfei258@gmail.com>
Signed-off-by: Jianfei Hu <hujianfei258@gmail.com>
Signed-off-by: Jianfei Hu <hujianfei258@gmail.com>
@clickhouse-gh
Copy link

clickhouse-gh bot commented Sep 21, 2025

Workflow [PR], commit [8b9cac5]

Summary:

job_name test_name status info comment
Stateless tests (amd_ubsan, parallel) failure
00900_long_parquet_load FAIL cidb, flaky
Integration tests (amd_asan, old analyzer, 4/6) failure
test_scheduler_cpu_preemptive/test.py::test_downscaling[cpu-slot-preemption-timeout-1ms] FAIL cidb
test_backup_restore_on_cluster/test_slow_rmt.py::test_replicated_database_async FAIL cidb
OOM in dmesg FAIL cidb
Stress test (arm_asan, s3) failure
Server died FAIL cidb
Hung check failed, possible deadlock found (see hung_check.log) FAIL cidb
Killed by signal (in clickhouse-server.log) FAIL cidb
Fatal message in clickhouse-server.log (see fatal_messages.txt) FAIL cidb
Killed by signal (output files) FAIL cidb

@clickhouse-gh clickhouse-gh bot added pr-performance Pull request with some performance improvements submodule changed At least one submodule changed in this PR. labels Sep 21, 2025
Signed-off-by: Jianfei Hu <hujianfei258@gmail.com>
@incfly incfly force-pushed the fixed-parallel-merge branch from 5eb06ca to 32a2b55 Compare September 21, 2025 04:34
@incfly incfly removed the submodule changed At least one submodule changed in this PR. label Sep 21, 2025
Signed-off-by: Jianfei Hu <hujianfei258@gmail.com>
@incfly incfly force-pushed the fixed-parallel-merge branch from a471df8 to c5a3c05 Compare September 21, 2025 04:48
Signed-off-by: Jianfei Hu <hujianfei258@gmail.com>
Signed-off-by: Jianfei Hu <hujianfei258@gmail.com>
@incfly incfly force-pushed the fixed-parallel-merge branch from 23367a0 to 79b9ad8 Compare September 21, 2025 18:48
@nickitat nickitat self-assigned this Sep 22, 2025
@incfly incfly force-pushed the fixed-parallel-merge branch from a2fb81f to dce079f Compare September 23, 2025 06:32
Signed-off-by: Jianfei Hu <hujianfei258@gmail.com>
@incfly incfly force-pushed the fixed-parallel-merge branch from dce079f to 7cc9b44 Compare September 23, 2025 06:36
@incfly
Copy link
Contributor Author

incfly commented Sep 24, 2025

Yeah. That's because we check fixed hashmap size during the merge, via checkLimits which require iterate all element in the buffer to see if they are zero. Meanwhile other threads are adding new entries to the first map.

I think we might be able to skip checkLimit()/size() at all, though this means more merges are executed potentially. Need to check how two level merge handles this.

@nickitat
Copy link
Member

Yeah. That's because we check fixed hashmap size during the merge, via checkLimits which require iterate all element in the buffer to see if they are zero. Meanwhile other threads are adding new entries to the first map.

I think we might be able to skip checkLimit()/size() at all, though this means more merges are executed potentially. Need to check how two level merge handles this.

It's fine to do the check on a single thread once a merge is done

Signed-off-by: Jianfei Hu <hujianfei258@gmail.com>
Signed-off-by: Jianfei Hu <hujianfei258@gmail.com>
@incfly incfly force-pushed the fixed-parallel-merge branch from 22bcb38 to b00fc8e Compare October 3, 2025 03:18
Signed-off-by: Jianfei Hu <hujianfei258@gmail.com>
@incfly incfly force-pushed the fixed-parallel-merge branch from b00fc8e to 6b7ff25 Compare October 3, 2025 04:51
@incfly incfly force-pushed the fixed-parallel-merge branch from 1450e9d to e677112 Compare October 4, 2025 06:49
Signed-off-by: Jianfei Hu <hujianfei258@gmail.com>
@incfly
Copy link
Contributor Author

incfly commented Oct 6, 2025

03373_named_session_try_recreate_before_timeout failed flaky tracking issue #87869

@incfly
Copy link
Contributor Author

incfly commented Oct 7, 2025

Hi @nickitat this is ready for another look, thanks!

@incfly incfly force-pushed the fixed-parallel-merge branch 4 times, most recently from 4303e6d to 6ef5b27 Compare October 12, 2025 22:16
Signed-off-by: Jianfei Hu <hujianfei258@gmail.com>
@incfly incfly force-pushed the fixed-parallel-merge branch from 6ef5b27 to 6c34c81 Compare October 12, 2025 22:23
@incfly
Copy link
Contributor Author

incfly commented Oct 13, 2025

test_ddl_worker_with_loopback_hosts is related to #88404

@incfly
Copy link
Contributor Author

incfly commented Oct 17, 2025

Hi @nickitat all comments have been addressed, PTAL, thanks!

Signed-off-by: Jianfei Hu <hujianfei258@gmail.com>
Signed-off-by: Jianfei Hu <hujianfei258@gmail.com>
Signed-off-by: Jianfei Hu <hujianfei258@gmail.com>
Signed-off-by: Jianfei Hu <hujianfei258@gmail.com>
Copy link
Member

@nickitat nickitat left a comment

Choose a reason for hiding this comment

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

great work!

@nickitat nickitat enabled auto-merge October 27, 2025 21:20
@nickitat nickitat added this pull request to the merge queue Oct 27, 2025
Merged via the queue into ClickHouse:master with commit f9948b3 Oct 27, 2025
119 of 123 checks passed
@robot-clickhouse-ci-2 robot-clickhouse-ci-2 added the pr-synced-to-cloud The PR is synced to the cloud repo label Oct 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-performance Pull request with some performance improvements pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

key16 aggregation method should be two level

4 participants