Deprecate io.trino.spi.type.Type#appendTo #26922
Open
+253
−502
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideThis PR deprecates the Type#appendTo SPI, removing all custom appendTo implementations and migrating every appendTo call site to use BlockBuilder#append with underlying blocks and positions. Code generation routines (JoinCompiler, MergeSortedPages) and various core operators have been updated to drop type parameters and stored Type fields where possible, streamlining the API and reducing boilerplate. Class diagram for deprecated Type#appendTo and migration to BlockBuilder#appendclassDiagram
class Type {
+writeObject(BlockBuilder blockBuilder, Object value)
+getObject(Block block, int position)
+getObjectValue(Block block, int position)
+getFlatFixedSize()
+getTypeOperatorDeclaration(TypeOperators typeOperators)
+createBlockBuilder(BlockBuilderStatus blockBuilderStatus, int expectedEntries)
+createBlockBuilder(BlockBuilderStatus blockBuilderStatus, int expectedEntries, int expectedBytesPerEntry)
+@Deprecated appendTo(Block block, int position, BlockBuilder blockBuilder)
}
class BlockBuilder {
+append(ValueBlock valueBlock, int valuePosition)
+appendNull()
+build()
}
Type <|-- AbstractIntType
Type <|-- AbstractLongType
Type <|-- AbstractVariableWidthType
Type <|-- ArrayType
Type <|-- MapType
Type <|-- RowType
Type <|-- DoubleType
Type <|-- LongDecimalType
Type <|-- LongTimeWithTimeZoneType
Type <|-- LongTimestampType
Type <|-- LongTimestampWithTimeZoneType
Type <|-- UuidType
Type <|-- BooleanType
Type <|-- ShortDecimalType
Type <|-- ShortTimeWithTimeZoneType
Type <|-- ShortTimestampType
Type <|-- ShortTimestampWithTimeZoneType
Type <|-- SmallintType
Type <|-- TinyintType
Type <|-- IpAddressType
Type <|-- FunctionType
Type <|-- UnknownType
AbstractIntType --|> Type
AbstractLongType --|> Type
AbstractVariableWidthType --|> Type
ArrayType --|> Type
MapType --|> Type
RowType --|> Type
DoubleType --|> Type
LongDecimalType --|> Type
LongTimeWithTimeZoneType --|> Type
LongTimestampType --|> Type
LongTimestampWithTimeZoneType --|> Type
UuidType --|> Type
BooleanType --|> Type
ShortDecimalType --|> Type
ShortTimeWithTimeZoneType --|> Type
ShortTimestampType --|> Type
ShortTimestampWithTimeZoneType --|> Type
SmallintType --|> Type
TinyintType --|> Type
IpAddressType --|> Type
FunctionType --|> Type
UnknownType --|> Type
BlockBuilder <.. Type : uses
BlockBuilder o-- ValueBlock
Class diagram for BlockSet constructor and usage changesclassDiagram
class BlockSet {
-BlockPositionIsIdentical elementIdenticalOperator
-BlockPositionHashCode elementHashCodeOperator
-int maximumSize
+BlockSet(BlockPositionIsIdentical elementIdenticalOperator, BlockPositionHashCode elementHashCodeOperator, int maximumSize)
+add(ValueBlock valueBlock, int valuePosition)
+contains(ValueBlock valueBlock, int valuePosition)
+getAllWithSizeLimit(BlockBuilder blockBuilder, String functionName, DataSize maxFunctionMemory)
}
BlockSet o-- BlockPositionIsIdentical
BlockSet o-- BlockPositionHashCode
BlockSet o-- BlockBuilder
BlockSet o-- ValueBlock
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `core/trino-main/src/main/java/io/trino/operator/scalar/ArraySortFunction.java:82-83` </location>
<code_context>
return arrayValueBuilder.build(arrayLength, elementBuilder -> {
+ ValueBlock valueBlock = block.getUnderlyingValueBlock();
for (int i = 0; i < arrayLength; i++) {
- type.appendTo(block, positions.getInt(i), elementBuilder);
+ elementBuilder.append(valueBlock, block.getUnderlyingValuePosition(positions.getInt(i)));
}
});
</code_context>
<issue_to_address>
**issue:** Switch from type.appendTo to BlockBuilder.append may affect null handling.
Please verify that BlockBuilder.append correctly handles null values in arrays, as its behavior may differ from type.appendTo.
</issue_to_address>
### Comment 2
<location> `core/trino-main/src/main/java/io/trino/operator/PagesIndex.java:319-321` </location>
<code_context>
// append the row
pageBuilder.declarePosition();
for (int channel = 0; channel < channels.length; channel++) {
- Type type = types.get(channel);
Block block = channels[channel].get(blockIndex);
- type.appendTo(block, blockPosition, pageBuilder.getBlockBuilder(channel));
+ pageBuilder.getBlockBuilder(channel).append(block.getUnderlyingValueBlock(), block.getUnderlyingValuePosition(blockPosition));
}
</code_context>
<issue_to_address>
**issue (bug_risk):** Removal of type usage in appendTo may impact type-specific logic.
Verify that BlockBuilder.append correctly supports all relevant types, including custom and complex ones, since type-specific handling in appendTo is no longer applied.
</issue_to_address>
### Comment 3
<location> `core/trino-main/src/main/java/io/trino/operator/scalar/ZipFunction.java:120` </location>
<code_context>
fieldBuilders.get(fieldIndex).appendNull();
}
else {
- types.get(fieldIndex).appendTo(arrays[fieldIndex], outputPosition, fieldBuilders.get(fieldIndex));
+ Block block = arrays[fieldIndex];
+ fieldBuilders.get(fieldIndex).append(block.getUnderlyingValueBlock(), block.getUnderlyingValuePosition(outputPosition));
}
</code_context>
<issue_to_address>
**issue:** Type-specific appendTo replaced with BlockBuilder.append; check for semantic changes.
Please verify that handling of nulls and type conversions remains consistent, and that the output row structure is correct for all types after this change.
</issue_to_address>
### Comment 4
<location> `core/trino-main/src/main/java/io/trino/operator/scalar/ArrayFlattenFunction.java:79-81` </location>
<code_context>
Block subArray = (Block) arrayType.getObject(array, i);
+ ValueBlock subArrayValueBlock = subArray.getUnderlyingValueBlock();
for (int j = 0; j < subArray.getPositionCount(); j++) {
- type.appendTo(subArray, j, builder);
+ builder.append(subArrayValueBlock, subArray.getUnderlyingValuePosition(j));
}
}
</code_context>
<issue_to_address>
**issue (bug_risk):** Direct append may not handle type-specific conversions.
Verify that builder.append performs necessary conversions and validations for all element types, including nested and custom arrays.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
core/trino-main/src/main/java/io/trino/operator/scalar/ArraySortFunction.java
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/operator/scalar/ArrayFlattenFunction.java
Show resolved
Hide resolved
|
@raunaqmorarka I suppose that it doesn't affect performance |
raunaqmorarka
force-pushed
the
raunaq/remove-type-append
branch
from
95907da to
61e1984
Compare
I don't expect it to make a significant difference |
|
Started benchmark workflow for this PR with test type =
|
|
Started benchmark workflow for this PR with test type =
|
Replaced all usages with io.trino.spi.block.BlockBuilder#append This simplifies block building operations by removing the need to go through the Type interface
Removes all the unused implementations and marks the API for removal
raunaqmorarka
force-pushed
the
raunaq/remove-type-append
branch
from
61e1984 to
6540de1
Compare
wendigo
approved these changes
Oct 13, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Migrates all existing usages to
io.trino.spi.block.BlockBuilder#appendThis simplifies the Type API and the building of blocks from block positions
Additional context and related issues
Release notes
( ) 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.
(x) Release notes are required, with the following suggested text:
Summary by Sourcery
Deprecate the SPI Type.appendTo method and migrate all call sites to BlockBuilder.append for streamlined block construction.
Enhancements: