imply-cheddar commented on code in PR #16800:
URL: https://github.com/apache/druid/pull/16800#discussion_r1721799444
##########
processing/src/main/java/org/apache/druid/query/operator/WindowOperatorQueryQueryToolChest.java:
##########
@@ -116,6 +128,36 @@ public Sequence<Object[]> resultsAsArrays(
return (Sequence) resultSequence;
}
+ @Override
+ public Optional<Sequence<FrameSignaturePair>> resultsAsFrames(
+ WindowOperatorQuery query,
+ Sequence<RowsAndColumns> resultSequence,
+ MemoryAllocatorFactory memoryAllocatorFactory,
+ boolean useNestedForUnknownTypes
+ )
+ {
Review Comment:
> I thought about this, however, we can also have a cluster-level config
that determines the limit, so we should be looking at that as well in the
window tool chest, which seems uncool that the window tool chest has to
determine what to do.
If you have a context parameter, you automatically have a cluster-level
config as well. You can set the default context from cluster-level stuff. If
we are creating a context parameter, we shouldn't have yet-another random
config to set things, that just makes it so much harder to figure out what is
set how.
Additionally, there's nothing wrong with the window query tool chest being
asked what to do, that's good. The queries should be more involved in things
because that allows them to optimize stuff based on what they know they need to
do. To wit, the whole reason you are having problems with this PR is because
there was an attempt to take the decision *away* from the query. If the code
had been written in a way that gave the query control from the first place, you
wouldn't have this problem. If you are worried about "but then everyone has to
implement the same thing", if it is truly the exact same implementation for
every, single, call site, then it's incredibly easy to just have a static
helper that all of the call sites can use. Yes, each place has to call the
helper, but the amount of time it takes to make a few places call a helper is
significantly less than the amount of time it takes to work around an
abstraction that isn't in the right place.
--
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]
