Workflow [PR], commit [1df7b04]

Summary: ❌

AI Review

Summary

This PR introduces automatic spilling for hash/parallel_hash joins by switching to GraceHashJoin when max_bytes_before_external_join is exceeded. The feature direction is valuable, but there are correctness/concurrency issues in SpillingHashJoin plus PR metadata/docs quality gaps that should be fixed before merge.

Missing context

• ⚠️ No CI logs or benchmark reports were provided in this review context, so performance/regression validation is limited to code inspection.

Findings

❌ Blockers

• [src/Interpreters/SpillingHashJoin.cpp:222] onBuildPhaseFinish updates chosen_join/state without synchronizing with switchToGraceHashJoin, which does transition under switch_mutex. This permits a race where a spill transition is overwritten by in-memory promotion, causing inconsistent join state.
  Suggested fix: guard concurrent branch of onBuildPhaseFinish with switch_mutex and re-check state under the lock before writing chosen_join/state.
• [src/Interpreters/SpillingHashJoin.cpp:123] During COLLECTING, limit checks can be bypassed in underlying hash join insertion path, changing behavior versus regular hash/parallel_hash joins and potentially ignoring max_rows_in_join/max_bytes_in_join semantics before spill happens.
  Suggested fix: preserve equivalent JOIN size-limit checks for the in-memory collecting phase (or explicitly enforce them in SpillingHashJoin).
• [src/Interpreters/SpillingHashJoin.h:22] The implementation still documents unresolved WITH TOTALS correctness via TODO/workarounds in tests, but selection is not hard-disabled for totals scenarios. This can expose known incorrect behavior when spilling is enabled.
  Suggested fix: reject SpillingHashJoin for totals queries during algorithm selection (or throw explicit NOT_IMPLEMENTED when totals are requested with auto-spill).

💡 Nits

• [src/Core/Settings.cpp:7513] PR text/changelog references a non-existent setting name (enable_auto_spilling_hash_join) while implementation uses max_bytes_before_external_join.
  Suggested fix: replace changelog wording with the actual setting name.
• [src/Core/Settings.cpp:7513] New user-facing setting behavior is introduced without corresponding documentation update (Documentation is written remains unchecked).
  Suggested fix: add docs describing semantics, constraints, and migration/rollout guidance for max_bytes_before_external_join.

Tests

• ⚠️ Add/keep dedicated tests for WITH TOTALS + non-zero max_bytes_before_external_join to verify guarded behavior (either explicit rejection or correct semantics) rather than relying on disabling spilling in test setup.
• ⚠️ Add a regression test for concurrent spill transition race (onBuildPhaseFinish vs spill switch) to ensure single, synchronized state transition.
• ⚠️ Add a regression test proving JOIN size limits (max_rows_in_join/max_bytes_in_join with join_overflow_mode) are still enforced on the in-memory path before spill transition.

ClickHouse Rules

Performance & Safety

• Concurrency safety concern: unsynchronized state transition between build-finish promotion and spill switch can lead to inconsistent join implementation selection at runtime.
• Safety/behavior concern: in-memory phase can diverge from established JOIN overflow semantics if limits are not equivalently enforced.

User-Lens

• Users are likely to be confused by changelog/docs mismatch around setting name and by undocumented constraints (especially totals behavior and overflow semantics under auto-spill).

Final Verdict

• Status: ⚠️ Request changes
• Minimum required actions:
  • Fix synchronization/state transition race in SpillingHashJoin build-finish path.
  • Ensure JOIN overflow limits are preserved with auto-spilling enabled.
  • Hard-gate or explicitly reject unsupported totals scenarios for SpillingHashJoin.
  • Correct changelog setting name and add user documentation for max_bytes_before_external_join.

I fixed the issue that you introduced with server side fuzzing. Now please unblock the PR.

optimizeJoinLegacy is depending on HashJoin, check if it is a problem.

My attempt to fix the read buffer related issue #100732

The flaky test check failed because of long running query. I used time ./clickhouse-test 03580_join_runtime_filter --test-runs 1000 -j 30 to measure the test time and it looked fine:

Master:
real    1m27.366s
user    10m39.690s
sys     5m14.201s

this branch:
real    1m27.177s
user    10m37.015s
sys     5m17.897s

So I think the only problem is the huge amount of test runs (1000+) run in the flaky test check.

Finally with Claude I managed to figure out why 00163_shard_join_with_empty_table with empty table fails. In short, the added Resize can mess up the order of the blocks, which means the problem is also present for GraceHash join in master, example (it might need to be run a few times to produce wrong result as it is basically a datarace).

I misread the query, there LIMIT is applied before the ORDER BY, so the output is only consistent because the normal hash join is single threaded.

LLVM Coverage Report

Changed lines: 82.66% (510/617) · Uncovered code

Full report · Diff report

03668_shard_join_in_reverse_order is fixed by #101456
test_user_overcommit is fixed by #101270

The parallel replicas related issues are fixed here #101154