Some minor tweaks and cleanups #27151
Open
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 systematically removes unnecessary stream allocations and boxing, replacing Stream-based APIs with Iterable or for-loops, introduces primitive-specialized types (OptionalInt, Int2ObjectMap, LongAdder) and presized Immutable builders, and restructures Lookup and related pattern matching interfaces for minimal overhead. Class diagram for updated Lookup interface and implementationsclassDiagram
class Lookup {
<<sealed interface>>
+resolve(PlanNode): PlanNode
+resolveGroup(PlanNode): Iterable<PlanNode>
+static noLookup(): Lookup
+static from(Function<GroupReference, Iterable<PlanNode>>): Lookup
}
class ThrowingLookup {
+resolveGroup(PlanNode): Iterable<PlanNode>
}
class LookupFromFunction {
+resolver: Function<GroupReference, Iterable<PlanNode>>
+resolveGroup(PlanNode): Iterable<PlanNode>
}
Lookup <|.. ThrowingLookup
Lookup <|.. LookupFromFunction
Lookup o-- "1" PlanNode
LookupFromFunction o-- "1" Function
ThrowingLookup o-- "1" PlanNode
Class diagram for Assignments and Builder changesclassDiagram
class Assignments {
+assignments: Map<Symbol, Expression>
+static builder(): Builder
+static builderWithExpectedSize(int): Builder
+static identity(Symbol...): Assignments
+static identity(Iterable<Symbol>): Assignments
+static copyOf(Map<Symbol, Expression>): Assignments
+static of(): Assignments
+static of(Symbol, Expression): Assignments
+static of(Symbol, Expression, Symbol, Expression): Assignments
+static of(Collection<Expression>, SymbolAllocator): Assignments
}
class Builder {
-assignments: ImmutableMap.Builder<Symbol, Expression>
+putAll(Assignments): Builder
+putAll(Map<Symbol, Expression>): Builder
+put(Symbol, Expression): Builder
+putIdentity(Symbol): Builder
+putIdentities(Iterable<Symbol>): Builder
+build(): Assignments
}
Assignments o-- "1" Builder
Class diagram for PartitioningScheme changesclassDiagram
class PartitioningScheme {
-partitioning: Partitioning
-outputLayout: List<Symbol>
-outputTypes: Supplier<List<Type>>
-replicateNullsAndAny: boolean
-bucketToPartition: Optional<int[]>
-bucketCount: OptionalInt
-partitionCount: OptionalInt
+getOutputLayout(): List<Symbol>
+getOutputTypes(): List<Type>
+isReplicateNullsAndAny(): boolean
+getBucketToPartition(): Optional<int[]>
+getBucketCount(): OptionalInt
+getPartitionCount(): OptionalInt
+withBucketToPartition(Optional<int[]>): PartitioningScheme
+withBucketCount(OptionalInt): PartitioningScheme
+withPartitioningHandle(PartitioningHandle): PartitioningScheme
+withPartitionCount(OptionalInt): PartitioningScheme
}
PartitioningScheme o-- "1" Partitioning
PartitioningScheme o-- "*" Symbol
Class diagram for QueryPlanOptimizerStats changesclassDiagram
class QueryPlanOptimizerStats {
-rule: String
-invocations: LongAdder
-applied: LongAdder
-totalTime: LongAdder
-failures: LongAdder
+record(long, boolean): void
+recordFailure(): void
+getRule(): String
+getInvocations(): long
+getApplied(): long
+getFailures(): long
+getTotalTime(): long
+snapshot(): QueryPlanOptimizerStatistics
+merge(QueryPlanOptimizerStats): QueryPlanOptimizerStats
}
QueryPlanOptimizerStats o-- "1" LongAdder
Class diagram for Pattern and related matching changesclassDiagram
class Pattern {
+previous: Optional<Pattern>
+accept(Object, Captures, C): Iterable<Match>
+matches(Object, C): boolean
+match(Object): Iterable<Match>
+match(Object, C): Iterable<Match>
+match(Object, Captures, C): Iterable<Match>
}
class OrPattern {
+accept(Object, Captures, C): Iterable<Match>
}
class WithPattern {
+accept(Object, Captures, C): Iterable<Match>
}
class EqualsPattern {
+accept(Object, Captures, C): Iterable<Match>
}
class FilterPattern {
+accept(Object, Captures, C): Iterable<Match>
}
class TypeOfPattern {
+accept(Object, Captures, C): Iterable<Match>
}
class CapturePattern {
+accept(Object, Captures, C): Iterable<Match>
}
Pattern <|.. OrPattern
Pattern <|.. WithPattern
Pattern <|.. EqualsPattern
Pattern <|.. FilterPattern
Pattern <|.. TypeOfPattern
Pattern <|.. CapturePattern
Class diagram for Capture changesclassDiagram
class Capture {
-number: int
+static newCapture(): Capture
}
Class diagram for Memo changes (use of Int2ObjectMap)classDiagram
class Memo {
-groups: Int2ObjectMap<Group>
+extract(): PlanNode
+replace(int, PlanNode, String): void
+getAllReferences(PlanNode): Set<Integer>
+deleteGroup(int): void
+insertChildrenAndRewrite(PlanNode): PlanNode
}
Memo o-- "*" Group
Class diagram for PlanFragment changesclassDiagram
class PlanFragment {
-partitionCount: OptionalInt
-outputPartitioningScheme: PartitioningScheme
+getPartitionCount(): OptionalInt
+getTypes(): List<Type>
}
PlanFragment o-- "1" PartitioningScheme
Class diagram for NodeScheduler and related node selection changesclassDiagram
class NodeScheduler {
+static getAllNodes(NodeMap, boolean): Set<InternalNode>
+static filterNodes(NodeMap, boolean, Set<InternalNode>): Set<InternalNode>
+static selectExactNodes(NodeMap, List<HostAddress>, boolean): List<InternalNode>
}
NodeScheduler o-- "*" InternalNode
Class diagram for usage of Int2ObjectMap in PipelinedStageExecutionclassDiagram
class PipelinedStageExecution {
-tasks: Int2ObjectMap<RemoteTask>
}
PipelinedStageExecution o-- "*" RemoteTask
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
github-actions
bot
added
iceberg
![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 - here's some feedback:
- This PR changes many public methods from Optional/Stream to OptionalInt/Iterable which is a breaking API change—please update the public API docs, release notes, and consider adding deprecation stubs or migration guidance.
- Several JSON-annotated classes now use OptionalInt; ensure Jackson (de)serialization is configured to handle OptionalInt properly by adding custom modules or annotations where needed.
- There are many repetitive manual loops and Guava Iterables usage replacing Stream pipelines—consider introducing a small utility method to convert sources into an ImmutableList to keep code concise and consistent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- This PR changes many public methods from Optional<Integer>/Stream to OptionalInt/Iterable which is a breaking API change—please update the public API docs, release notes, and consider adding deprecation stubs or migration guidance.
- Several JSON-annotated classes now use OptionalInt; ensure Jackson (de)serialization is configured to handle OptionalInt properly by adding custom modules or annotations where needed.
- There are many repetitive manual loops and Guava Iterables usage replacing Stream pipelines—consider introducing a small utility method to convert sources into an ImmutableList to keep code concise and consistent.
## Individual Comments
### Comment 1
<location> `core/trino-main/src/main/java/io/trino/execution/scheduler/UniformNodeSelector.java:161` </location>
<code_context>
Set<InternalNode> blockedExactNodes = new HashSet<>();
boolean splitWaitingForAnyNode = false;
- List<InternalNode> filteredNodes = filterNodes(nodeMap, includeCoordinator, ImmutableSet.of());
+ Set<InternalNode> filteredNodes = filterNodes(nodeMap, includeCoordinator, ImmutableSet.of());
ResettableRandomizedIterator<InternalNode> randomCandidates = new ResettableRandomizedIterator<>(filteredNodes);
Set<InternalNode> schedulableNodes = new HashSet<>(filteredNodes);
</code_context>
<issue_to_address>
**question:** Changing filteredNodes to Set may affect assignment logic if order is important.
Please verify that computeAssignments does not depend on the order of filteredNodes, as switching from List to Set removes ordering guarantees.
</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/execution/scheduler/UniformNodeSelector.java
Show resolved
Hide resolved
martint
reviewed
Oct 29, 2025
core/trino-main/src/main/java/io/trino/sql/planner/plan/Assignments.java
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/sql/planner/iterative/Lookup.java
Show resolved
Hide resolved
wendigo
force-pushed
the
serafin/matching-iterable
branch
3 times, most recently
from
063beeb to
9024bc0
Compare
|
|
||
| @Override | ||
| public <C> Stream<Match> accept(Object object, Captures captures, C context) | ||
| public <C> Iterable<Match> accept(Object object, Captures captures, C context) |
There was a problem hiding this comment.
What's the benefit of using Iterable instead of Stream ?
I thought Stream is usually preferred in modern java code
There was a problem hiding this comment.
We convert stream to the iterator in the usage site anyway so I don't think it is a right abstraction here. I'm not fan of streams either :)
Comment on lines
-48
to
+45
| Stream<PlanNode> resolveGroup(PlanNode node); | ||
| Iterable<PlanNode> resolveGroup(PlanNode node); |
core/trino-main/src/main/java/io/trino/sql/planner/iterative/Lookup.java
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/sql/planner/plan/Assignments.java
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/sql/planner/plan/Assignments.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/execution/scheduler/NodeScheduler.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/execution/scheduler/NodeScheduler.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/execution/scheduler/NodeScheduler.java
Outdated
Show resolved
Hide resolved
wendigo
force-pushed
the
serafin/matching-iterable
branch
from
9024bc0 to
218f953
Compare
raunaqmorarka
approved these changes
Oct 30, 2025
Named captures are not used so keeping a String as a description doesn't provide any value if the prefix is always "@"
To avoid frequent int (un)boxing from Optional<Integer> type.
LongAdder accumulates stripped values during read, rather than making it visible to other threads on write.
This returns a singleton ZERO instance instead of creating a new Object.
wendigo
force-pushed
the
serafin/matching-iterable
branch
from
218f953 to
f955854
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Mostly around:
Description
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:
Summary by Sourcery
Refactor codebase to reduce boxing and Stream overhead, improve performance, and standardize on primitive-based APIs
Enhancements: