Skip to content

Logical expression optimizer in new analyzer#46644

Merged
antonio2368 merged 8 commits intomasterfrom
or-equality-chain-to-in
Mar 8, 2023
Merged

Logical expression optimizer in new analyzer#46644
antonio2368 merged 8 commits intomasterfrom
or-equality-chain-to-in

Conversation

@antonio2368
Copy link
Copy Markdown
Member

@antonio2368 antonio2368 commented Feb 21, 2023

Changelog category (leave one):

  • Not For Changelog

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

Add logical expression optimizer to new analyzer.

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

Information about CI checks: https://clickhouse.com/docs/en/development/continuous-integration/

@robot-ch-test-poll2 robot-ch-test-poll2 added the pr-improvement Pull request with some product improvements label Feb 21, 2023
@hanfei1991 hanfei1991 self-assigned this Feb 21, 2023
@novikd novikd self-assigned this Feb 21, 2023
@antonio2368
Copy link
Copy Markdown
Member Author

@novikd I changed/added a bunch of things, please tell me if it's okay/going in the right direction.

@hanfei1991 hanfei1991 removed their assignment Feb 27, 2023
Copy link
Copy Markdown
Member

@novikd novikd left a comment

Choose a reason for hiding this comment

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

LGTM

const auto & settings = getSettings();
if (equals_functions.size() < settings.optimize_min_equality_disjunction_chain_length && !expression.node->getResultType()->lowCardinality())
{
std::move(equals_functions.begin(), equals_functions.end(), std::back_inserter(or_operands));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we introduce problems if operands order has changed?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I can't think of anything logical with OR because we take only the equality functions that have constants + we move the IN operator to the back.
Maybe short-circuiting can perform better in some cases but that should be performance only and it's case dependant.

@antonio2368 antonio2368 requested a review from novikd March 7, 2023 12:08
@antonio2368 antonio2368 merged commit a24c9ee into master Mar 8, 2023
@antonio2368 antonio2368 deleted the or-equality-chain-to-in branch March 8, 2023 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-improvement Pull request with some product improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants