Skip to content

Do not wrap with Slice for comparisons and hashing#28573

Merged
wendigo merged 1 commit into
masterfrom
user/serafin/small-improvements
Mar 6, 2026
Merged

Do not wrap with Slice for comparisons and hashing#28573
wendigo merged 1 commit into
masterfrom
user/serafin/small-improvements

Conversation

@wendigo
Copy link
Copy Markdown
Contributor

@wendigo wendigo commented Mar 6, 2026

Description

Saves on the allocation while comparing

Additional context and related issues

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:

## Section
* Fix some things. ({issue}`issuenumber`)

Copy link
Copy Markdown
Member

@pettyjamesm pettyjamesm left a comment

Choose a reason for hiding this comment

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

LGTM, once the length vs toIndex fix is made

Comment thread core/trino-spi/src/main/java/io/trino/spi/type/AbstractVariableWidthType.java Outdated
@wendigo wendigo force-pushed the user/serafin/small-improvements branch from 69c56ba to 1f83054 Compare March 6, 2026 17:48
@starburstdata-automation
Copy link
Copy Markdown

starburstdata-automation commented Mar 6, 2026

Started benchmark workflow for this PR with test type = iceberg/sf1000_parquet_unpart.

Building Trino finished with status: success
Benchmark finished with status: success
Comparing results to the static baseline values, follow above workflow link for more details/logs.
Status message: NO Regression found.
Benchmark Comparison to the closest run from Master: Report

@pettyjamesm
Copy link
Copy Markdown
Member

Will be interested to see if this yields a perf difference in benchmarks, but I kind of doubt it. The slice allocation and equals method may have been inlined before yielding essentially similar code. It shouldn't hurt though, so no objections to the change as is even without a measurable lift.

@wendigo
Copy link
Copy Markdown
Contributor Author

wendigo commented Mar 6, 2026

@pettyjamesm I doubt that but worth checking anyway :)

@wendigo wendigo merged commit 718b589 into master Mar 6, 2026
212 of 214 checks passed
@wendigo wendigo deleted the user/serafin/small-improvements branch March 6, 2026 20:24
@github-actions github-actions Bot added this to the 480 milestone Mar 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

4 participants