Skip to content

Fix crash when rewriting IN to EXISTS in some case#88602

Merged
davenger merged 2 commits intomasterfrom
fix_88569
Oct 16, 2025
Merged

Fix crash when rewriting IN to EXISTS in some case#88602
davenger merged 2 commits intomasterfrom
fix_88569

Conversation

@davenger
Copy link
Copy Markdown
Member

@davenger davenger commented Oct 15, 2025

Fixes #88569
The crash was caused by 1st argument of original IN function being reset to nullptr here:

auto & copy_of_in_first_parameter = function_in_arguments_nodes[0];
auto subquery_projection = std::make_shared<IdentifierNode>(Identifier{unique_column_name});
equals_function_node_ptr->getArguments().getNodes() = {
std::move(copy_of_in_first_parameter), /// x

Even though the function gets replaced in the query tree, in some case resolveFunction might be called again on a copy of the original FunctionNode pointer, e.g. in case when IN is in IF condition:
resolveExpressionNode(if_function_condition, scope, false /*allow_lambda_expression*/, false /*allow_table_expression*/);

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

...

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh Bot commented Oct 15, 2025

Workflow [PR], commit [db280c6]

Summary:

job_name test_name status info comment
Integration tests (amd_binary, 2/5) failure
test_merge_tree_s3/test.py::test_merge_canceled_by_s3_errors[node-broken_s3_always_multi_part] FAIL cidb #87560
Performance Comparison (amd_release, master_head, 3/3) failure
Configure failure

@clickhouse-gh clickhouse-gh Bot added the pr-not-for-changelog This PR should not be mentioned in the changelog label Oct 15, 2025
@scanhex12 scanhex12 self-assigned this Oct 15, 2025
Copy link
Copy Markdown
Member

@scanhex12 scanhex12 left a comment

Choose a reason for hiding this comment

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

LGTM, but please change label to "critical bugfix"

@davenger
Copy link
Copy Markdown
Member Author

davenger commented Oct 15, 2025

but please change label to "critical bugfix"

AFAIK even though this is a crash, as long as this is a bug in new functionality that hasn't been included in any release yet it's not considered a "bugfix" from changelog standpoint.

@davenger davenger added this pull request to the merge queue Oct 16, 2025
Merged via the queue into master with commit 63c16c7 Oct 16, 2025
120 of 123 checks passed
@davenger davenger deleted the fix_88569 branch October 16, 2025 07:24
@robot-ch-test-poll2 robot-ch-test-poll2 added the pr-synced-to-cloud The PR is synced to the cloud repo label Oct 16, 2025
@clickgapai
Copy link
Copy Markdown
Contributor

Hi @davenger @scanhex12 — while reviewing this PR I found the following:

Happy to discuss — close anything that's wrong or already addressed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-not-for-changelog This PR should not be mentioned in the changelog 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.

rewrite_in_to_join SEGV

4 participants