Skip to content

Use encoded type in QueryTreeHash#82617

Merged
KochetovNicolai merged 12 commits intomasterfrom
use-encoded-type-in-query-tree-hash
Jul 5, 2025
Merged

Use encoded type in QueryTreeHash#82617
KochetovNicolai merged 12 commits intomasterfrom
use-encoded-type-in-query-tree-hash

Conversation

@KochetovNicolai
Copy link
Copy Markdown
Member

@KochetovNicolai KochetovNicolai commented Jun 25, 2025

Changelog category (leave one):

  • Performance Improvement

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

Try to speedup QueryTreeHash a bit.

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Jun 25, 2025

Workflow [PR], commit [54a3d38]

Summary:

@clickhouse-gh clickhouse-gh bot added the pr-performance Pull request with some performance improvements label Jun 25, 2025
@novikd novikd self-assigned this Jun 26, 2025
@KochetovNicolai
Copy link
Copy Markdown
Member Author

image

@KochetovNicolai KochetovNicolai marked this pull request as ready for review June 30, 2025 17:27
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.

In general LGTM

}

WriteBufferFromOwnString buf;
encodeDataType(type, buf);
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.

Does it work for custom data types?

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.

Yes

return BinaryTypeIndex::Custom;

/// Subclass must convert its internal state and its children to AST
virtual ASTPtr toASTImpl(const ConvertToASTOptions & options) const = 0;

static void updateHashForType(HashState & hash_state, const DataTypePtr & type);
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.

Let's add a comment

Prewhere filter
Prewhere filter column: greaterOrEquals(a, 0)
Filter column: and(less(multiply(2, b), 100), indexHint(less(i, 100))) (removed)
Filter column: and(indexHint(less(i, 100)), less(multiply(2, b), 100)) (removed)
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.

Why did it change?

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 did not investigate, but probably in some places we traverse a hash table with a query tree.
For indexHint it's ok to be in any place.

In #82938 this is reverted back.

@KochetovNicolai KochetovNicolai added this pull request to the merge queue Jul 5, 2025
Merged via the queue into master with commit 745d6e5 Jul 5, 2025
236 of 240 checks passed
@KochetovNicolai KochetovNicolai deleted the use-encoded-type-in-query-tree-hash branch July 5, 2025 13:19
@robot-clickhouse-ci-1 robot-clickhouse-ci-1 added the pr-synced-to-cloud The PR is synced to the cloud repo label Jul 5, 2025
@KochetovNicolai KochetovNicolai added the pr-must-backport Pull request should be backported intentionally. Use this label with great care! label Jul 5, 2025
@robot-clickhouse-ci-1 robot-clickhouse-ci-1 added pr-backports-created-cloud deprecated label, NOOP pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR labels Jul 5, 2025
robot-clickhouse added a commit that referenced this pull request Jul 5, 2025
Cherry pick #82617 to 25.3: Use encoded type in QueryTreeHash
robot-clickhouse added a commit that referenced this pull request Jul 5, 2025
Cherry pick #82617 to 25.4: Use encoded type in QueryTreeHash
robot-clickhouse added a commit that referenced this pull request Jul 5, 2025
Cherry pick #82617 to 25.5: Use encoded type in QueryTreeHash
robot-clickhouse added a commit that referenced this pull request Jul 5, 2025
Cherry pick #82617 to 25.6: Use encoded type in QueryTreeHash
clickhouse-gh bot added a commit that referenced this pull request Jul 5, 2025
Backport #82617 to 25.6: Use encoded type in QueryTreeHash
KochetovNicolai added a commit that referenced this pull request Jul 6, 2025
Backport #82617 to 25.3: Use encoded type in QueryTreeHash
KochetovNicolai added a commit that referenced this pull request Jul 6, 2025
Backport #82617 to 25.4: Use encoded type in QueryTreeHash
KochetovNicolai added a commit that referenced this pull request Jul 6, 2025
Backport #82617 to 25.5: Use encoded type in QueryTreeHash
@robot-ch-test-poll3 robot-ch-test-poll3 added the pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore label Jul 7, 2025
@novikd novikd mentioned this pull request Jan 30, 2026
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore pr-backports-created-cloud deprecated label, NOOP pr-must-backport Pull request should be backported intentionally. Use this label with great care! pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR pr-performance Pull request with some performance improvements 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.

4 participants