Backport #107407 to 26.5: Fix logical error in disjunction filter push-down through JOIN with type-changing USING key by robot-clickhouse · Pull Request #108903 · ClickHouse/ClickHouse · GitHub
Skip to content

Backport #107407 to 26.5: Fix logical error in disjunction filter push-down through JOIN with type-changing USING key#108903

Merged
vdimir merged 4 commits into
26.5from
backport/26.5/107407
Jun 30, 2026
Merged

Backport #107407 to 26.5: Fix logical error in disjunction filter push-down through JOIN with type-changing USING key#108903
vdimir merged 4 commits into
26.5from
backport/26.5/107407

Conversation

@robot-clickhouse

@robot-clickhouse robot-clickhouse commented Jun 30, 2026

Copy link
Copy Markdown
Member

Original pull-request #107407
Cherry-pick pull-request #108756

This pull-request is a last step of an automated backporting.
Treat it as a standard pull-request: look at the checks and resolve conflicts.
Merge it only if you intend to backport changes to the target branch, otherwise just close it.

The PR source

The PR is created in the CI job

Version info

  • Merged into: 26.5.5.8

…h-down through JOIN with type-changing USING key
@robot-clickhouse robot-clickhouse added pr-backport Changes, backported to release branch. Do not use manually - automated use only! pr-bugfix Pull request with bugfix, not backported by default labels Jun 30, 2026
@clickhouse-gh

clickhouse-gh Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

alexey-milovidov and others added 2 commits June 30, 2026 08:58
…tiIf)

The backport's regression test `04330_join_disjunctions_pushdown_using_type_mismatch`
failed in the "distributed plan" stateless configs: the final "analyzer no crash"
query returned an `UNKNOWN_IDENTIFIER` exception instead of the expected `1`.

Root cause is version-specific, not a gap in the backported fix. The query phrases the
statically-dead branch (referencing the unknown identifiers `t04330.x` and `r.s`) with
`multiIf`. On master that dead branch is folded away at analysis time by the `multiIf`
constant-branch special case in `resolveFunction.cpp` (`is_special_function_multi_if`),
so the identifiers are never resolved and the query succeeds. That `multiIf` folding is
not present in 26.5 — it was added later by an unrelated change and is out of scope for
this bug-fix backport — so 26.5 resolves the dead branch and throws.

The `if` constant-branch special case (`is_special_function_if`) is present in 26.5 and
swallows the dead branch identically, exercising the same `scope.join_using_columns`
dangling-pointer crash path that the `IdentifierResolver` `SCOPE_EXIT` fix in this
backport guards. Since `multiIf(c, a, b)` with a single condition is equivalent to
`if(c, a, b)`, the regression is expressed with `if` here. Verified on the 26.5
`amd_asan_ubsan`/`arm_release` build that the test matches the reference (including the
final `1`) with `serialize_query_plan=1`, `serialize_query_plan=0`, and
`optimize_multiif_to_if=1`.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@alexey-milovidov

Copy link
Copy Markdown
Member

The only red was the PR's own test 04330_join_disjunctions_pushdown_using_type_mismatch, failing in both "distributed plan" stateless configs. The final --- analyzer no crash --- query returned an UNKNOWN_IDENTIFIER exception instead of the expected 1.

Root cause is version-specific, not a gap in the backported fix. Both source fixes (filterPushDown.cpp require_stable_types and IdentifierResolver.cpp SCOPE_EXIT) are cherry-picked faithfully. The regression query phrases its statically-dead branch (referencing the unknown identifiers t04330.x / r.s) with multiIf. On master that dead branch is folded away at analysis time by the multiIf constant-branch special case in resolveFunction.cpp (is_special_function_multi_if), so the unknown identifiers are never resolved and the query succeeds. That multiIf folding is not present in 26.5 — it was added by a separate, unrelated change (master d6cf77a8856), out of scope for this bug-fix backport — so 26.5 resolves the dead branch and throws. (The master test also carries SET explain_query_plan_default = 'legacy', but that setting does not exist in 26.5 and is irrelevant: the failure is identifier resolution, reproducible without EXPLAIN.)

Fix: express the dead branch with if instead of multiIf (a single-condition multiIf is equivalent to if). The if constant-branch special case (is_special_function_if) is present in 26.5 and swallows the dead branch identically, exercising the very same scope.join_using_columns dangling-pointer crash path that the IdentifierResolver SCOPE_EXIT fix guards.

Verified on the 26.5 amd_asan_ubsan / arm_release CI build (26.5.5.6) that the test matches the reference (including the final 1) under serialize_query_plan=1 (distributed plan), serialize_query_plan=0, and optimize_multiif_to_if=1; the original multiIf form fails. The same arm master build runs the inner query without error, confirming the version-behaviour difference.

I also merged origin/26.5 (clean, no conflicts) since the branch was a couple of days behind and red.

…sing it

Supersedes the previous commit's `multiIf` -> `if` rewrite. Aligns this 26.5 backport
with the 26.4 sibling (#108902, commit 06d2b2b), where the same situation was
resolved by dropping the `--- analyzer no crash ---` query.

The query reproduces the secondary analyzer fix in `IdentifierResolver.cpp` (a dangling
pointer in `scope.join_using_columns` left behind when an `UNKNOWN_IDENTIFIER` thrown from
a statically-dead branch is swallowed during resolution). That swallow, for a dead
`multiIf` branch, only exists on branches that have the `multiIf` constant-folding path
(`is_special_function_multi_if` in `resolveFunction.cpp`), which was added to master after
26.5 and is unrelated to this fix. 26.5 only swallows dead `if` branches; for `multiIf` the
unknown identifier propagates, so the query errors (`UNKNOWN_IDENTIFIER` for `r.s`) instead
of returning `1`.

An `if`-based variant was considered, but it does not reproduce the crash on an unpatched
26.5 binary either (the dangling-pointer scenario is not reachable without the `multiIf`
swallow), so it would be a query that passes regardless of the fix. The query is therefore
omitted, with an explanatory note, and the `IdentifierResolver.cpp` change is kept for
parity with master. The primary fix and its coverage (the disjunction push-down logical
error `Unexpected return type from equals`) are unchanged and still exercised by the first
part of the test.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@alexey-milovidov

Copy link
Copy Markdown
Member

@vdimir vdimir merged commit 1d04e5e into 26.5 Jun 30, 2026
34 checks passed
@vdimir vdimir deleted the backport/26.5/107407 branch June 30, 2026 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-backport Changes, backported to release branch. Do not use manually - automated use only! pr-bugfix Pull request with bugfix, not backported by default

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants