scottlan0113 opened a new pull request, #19461:
URL: https://github.com/apache/pinot/pull/19461
## Motivation
`CombinePlanNode` exposes a protected method per combine operator, so an
implementation can substitute the operator for one query type and leave the
rest to the existing dispatch. Nothing outside
`org.apache.pinot.core.plan.maker` can construct a subclass that uses them,
though.
`makeInstancePlan` and `makeStreamingInstancePlan` both construct
`CombinePlanNode` directly, so substituting means overriding one of those. Both
open with `applyQueryOptions`, which is package-private and reads private
server-level fields — `_maxExecutionThreads`, `_defaultExecutionThreads`,
`_numGroupsLimit`, `_numGroupsWarningLimit`, and the trim/capacity settings —
that `init()` populates from the server configuration and no accessor exposes.
Nothing else calls it. So an override outside this package can neither invoke
it nor reproduce it: a copy would compute the wrong execution-thread count and
trim sizes.
## Change
Extract the construction into a protected `createCombinePlanNode()` used by
both entry points.
```java
protected CombinePlanNode createCombinePlanNode(List<PlanNode> planNodes,
QueryContext queryContext,
ExecutorService executorService, @Nullable ResultsBlockStreamer
streamer) {
return new CombinePlanNode(planNodes, queryContext, executorService,
streamer);
}
```
One method rather than one per entry point. The streaming/non-streaming
distinction is already expressed inside `CombinePlanNode`, which picks the
combine operator per query type; expressing it here as well would state the
same choice twice, and a subclass substituting one query type still overrides
only that one method.
The objects constructed, the conditions and their ordering are unchanged.
## Testing
56 existing tests pass across `CombinePlanNodeTest` (6),
`InstancePlanMakerImplV2Test` (5), `SelectionCombineOperatorTest` (7),
`SortedGroupByCombineOperatorsTest` (12), `StreamingGroupByCombineOperatorTest`
(9) and `StreamingDistinctCombineOperatorTest` (17) — the construction moved,
nothing about what gets constructed changed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
