Workflow [PR], commit [79cad63]

Summary: ❌

AI Review

Summary

This PR fixes a real LOGICAL_ERROR path in HashJoin::getNonJoinedBlocks by replacing a raw column-count invariant with a unique-name-set comparison, which matches actual deduplication behavior after join-side swap. The core logic change looks correct and targeted, and the PR includes a dedicated regression test. I found one major issue in test robustness (already noted by an existing inline comment) and one additional major issue in exception-message quality.

Findings

⚠️ Majors

• [src/Interpreters/HashJoin/HashJoin.cpp:1743] The invariant now validates set equality, but the thrown error still says Unexpected number of columns and reports an additive decomposition ([left] + [right] + [to_add]) that is not semantically correct after deduplication. This makes debugging future mismatches unnecessarily confusing.
  • Suggested fix: reword the message to describe column-set mismatch and include expected/actual names (or missing/extra names) instead of count arithmetic.

Tests

• ⚠️ tests/queries/0_stateless/04057_global_not_in_join_swap_column_overlap.sh currently suppresses all failures for both repro queries via >/dev/null 2>&1 ||:. This means the test can pass even if the original LOGICAL_ERROR regresses. Add an assertion that the specific error text is absent (or fail on non-zero exit except for explicitly allowed errors).

ClickHouse Rules

Final Verdict

• Status: ⚠️ Request changes
• Minimum required actions:
  • Make the new regression test assertive (do not ignore all failures with ||:).
  • Update the HashJoin::getNonJoinedBlocks exception text to match set-based validation semantics.

Hi @alexey-milovidov, the fix direction is correct but the NameSet approach over-deduplicates. Here's why the 8 tests fail and a suggested correction.

Why the 8 Tests Fail

The 8 failing tests (00370_duplicate_columns_in_subqueries, 02497_storage_join_right_assert, etc.) contain queries with within-side duplicate column names — e.g. the same subquery column appearing twice on the left side:

SELECT 'y', * FROM (SELECT count('y'), count('y'), 2 AS x) AS t1 
RIGHT JOIN (SELECT count('x'), count('y'), 3 AS x) AS t2 ON t1.x = t2.x;

Here count('y') appears twice in t1 (left side). The result_sample_block legitimately has both columns (count: ≥5). But NameSet deduplicates count('y') to a single entry → expected_names.size() < result_sample_block.columns() → assertion fires incorrectly.

The Root Difference

• STID 2980-385f case: expected > actual — same name in left AND right → result deduplicates cross-side → expected overcounts
• Failing tests: expected < actual — same name twice within left → NameSet undercounts

Suggested Fix: Cross-Side Overlap Only

Instead of using NameSet for everything, subtract only the cross-side overlap (names that appear in both the left side and the right side / keys):

size_t left_columns_count = left_sample_block.columns();
if (canRemoveColumnsFromLeftBlock())
    left_columns_count = table_join->getOutputColumns(JoinTableSide::Left).size();

// Build unique name set for left side only
NameSet left_names;
if (canRemoveColumnsFromLeftBlock())
    for (const auto & col : table_join->getOutputColumns(JoinTableSide::Left))
        left_names.insert(col.name);
else
    for (const auto & name : left_sample_block.getNames())
        left_names.insert(name);

// Count names that appear in BOTH left and (right_keys ∪ to_add)
size_t cross_side_overlap = 0;
for (size_t i = 0; i < required_right_keys.columns(); ++i)
    if (left_names.count(required_right_keys.getByPosition(i).name))
        ++cross_side_overlap;
for (size_t i = 0; i < sample_block_with_columns_to_add.columns(); ++i)
    if (left_names.count(sample_block_with_columns_to_add.getByPosition(i).name))
        ++cross_side_overlap;

size_t expected_columns_count = left_columns_count
    + required_right_keys.columns()
    + sample_block_with_columns_to_add.columns()
    - cross_side_overlap;

This:

• Preserves within-left duplicates in the count → no false positive for {count('y'), count('y'), x}
• Correctly subtracts when the same name appears in both left and right → fixes STID 2980-385f
• Minimal change from the original logic — just adds the overlap subtraction