-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Remove more unused code for pre-computed hashes #26017
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This code became unused after removal of HashGenerationOptimizer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR removes unused code related to pre-computed hashes following the deprecation of HashGenerationOptimizer, updating method signatures and eliminating the now‑unused Optional hashChannel parameters.
- Removed unused boolean flag and Optional hashChannel parameters from group-by hash and aggregation-related methods.
- Updated helper methods (e.g. selectGroupByHashMode) to reflect the removal of pre-computed hash support.
- Removed extraneous hash handling code in various operator implementations.
Reviewed Changes
Copilot reviewed 154 out of 154 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| core/trino-main/src/main/java/io/trino/operator/index/UnloadedIndexKeyRecordSet.java | Updated createGroupByHash call by removing an extra boolean parameter. |
| core/trino-main/src/main/java/io/trino/operator/exchange/LocalExchange.java | Removed unused Optional partitionHashChannel and related references. |
| core/trino-main/src/main/java/io/trino/operator/aggregation/partial/SkipAggregationBuilder.java | Eliminated the inputHashChannel parameter and its subsequent handling. |
| core/trino-main/src/main/java/io/trino/operator/GroupByHash.java | Adjusted selectGroupByHashMode signature to remove hasPrecomputedHash. |
| core/trino-main/src/main/java/io/trino/operator/BigintGroupByHash.java | Removed outputRawHash related calculations and hash writing logic. |
Comments suppressed due to low confidence (5)
core/trino-main/src/main/java/io/trino/operator/index/UnloadedIndexKeyRecordSet.java:68
- Ensure that the removal of the second boolean parameter in the createGroupByHash call correctly matches the updated method signature and does not affect the intended behavior.
GroupByHash groupByHash = createGroupByHash(session, distinctChannelTypes, false, false, 10_000, hashStrategyCompiler, NOOP);
core/trino-main/src/main/java/io/trino/operator/GroupByHash.java:52
- Verify that all downstream calls to selectGroupByHashMode have been updated to use the new signature that omits the precomputed hash flag.
static GroupByHashMode selectGroupByHashMode(boolean spillable, List<Type> types)
core/trino-main/src/main/java/io/trino/operator/MarkDistinctOperator.java:80
- Remove the now-unneeded hashChannel parameter from the duplicate method to ensure the factory creation aligns with the updated constructor signature.
return new MarkDistinctOperatorFactory(operatorId, planNodeId, types.subList(0, types.size() - 1), markDistinctChannels, hashChannel, hashStrategyCompiler);
core/trino-main/src/main/java/io/trino/operator/BigintGroupByHash.java:137
- [nitpick] Confirm that the removal of the outputRawHash logic and related hash writing does not affect any consumers that might have depended on raw hash output.
if (outputRawHash) { ... }
core/trino-main/src/main/java/io/trino/operator/FlatHash.java:206
- [nitpick] Check that always computing the hash via flatHashStrategy.hash (after removal of precomputed hash handling) does not introduce performance regressions in high‑throughput scenarios.
long hash = flatHashStrategy.hash(blocks, position);
20a3c29 to
09ee826
Compare
Description
This code became unused after HashGenerationOptimizer was removed
Additional context and related issues
Follow-up to #26002
Release notes
(x) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text: