imply-cheddar commented on code in PR #16800:
URL: https://github.com/apache/druid/pull/16800#discussion_r1721954875
##########
server/src/main/java/org/apache/druid/server/ClientQuerySegmentWalker.java:
##########
@@ -429,22 +432,18 @@ private DataSource inlineIfNecessary(
} else if (canRunQueryUsingLocalWalker(subQuery) ||
canRunQueryUsingClusterWalker(subQuery)) {
// Subquery needs to be inlined. Assign it a subquery id and run it.
- final Sequence<?> queryResults;
+ final Function<Query<T>, QueryRunner<T>> queryRunnerSupplier;
if (dryRun) {
- queryResults = Sequences.empty();
+ queryRunnerSupplier = query -> (queryPlus, responseContext) ->
Sequences.empty();
} else {
- final QueryRunner subqueryRunner = subQuery.getRunner(this);
- queryResults = subqueryRunner.run(
- QueryPlus.wrap(subQuery),
- DirectDruidClient.makeResponseContextForQuery()
- );
+ queryRunnerSupplier = query -> query.getRunner(this);
Review Comment:
I'm not sure you need this to be a `Function<>`. I think it can just be
`QueryRunner<T>`. It should be safe to get the QueryRunner here and then just
pass in the query with the context when you actually run it.
That, or you can make the decision on what serialization format you are
going to ask for ahead of this and add it to the context first. Then you
probably don't even have to change any of the implementation below.
##########
processing/src/main/java/org/apache/druid/query/operator/WindowOperatorQueryQueryToolChest.java:
##########
@@ -50,30 +57,45 @@ public class WindowOperatorQueryQueryToolChest extends
QueryToolChest<RowsAndCol
@SuppressWarnings("unchecked")
public QueryRunner<RowsAndColumns> mergeResults(QueryRunner<RowsAndColumns>
runner)
{
- return new RowsAndColumnsUnravelingQueryRunner(
- (queryPlus, responseContext) -> {
- final WindowOperatorQuery query = (WindowOperatorQuery)
queryPlus.getQuery();
- final List<OperatorFactory> opFactories = query.getOperators();
- if (opFactories.isEmpty()) {
- return runner.run(queryPlus, responseContext);
- }
-
- Supplier<Operator> opSupplier = () -> {
- Operator retVal = new SequenceOperator(
- runner.run(
- queryPlus.withQuery(query.withOperators(new
ArrayList<OperatorFactory>())),
- responseContext
- )
- );
- for (OperatorFactory operatorFactory : opFactories) {
- retVal = operatorFactory.wrap(retVal);
- }
- return retVal;
- };
+ QueryRunner<RowsAndColumns> baseRunner = (queryPlus, responseContext) -> {
+ final WindowOperatorQuery query = (WindowOperatorQuery)
queryPlus.getQuery();
+ final List<OperatorFactory> opFactories = query.getOperators();
+ if (opFactories.isEmpty()) {
+ return runner.run(queryPlus, responseContext);
+ }
- return new OperatorSequence(opSupplier);
+ Supplier<Operator> opSupplier = () -> {
+ Operator retVal = new SequenceOperator(
+ runner.run(
+ queryPlus.withQuery(query.withOperators(new
ArrayList<OperatorFactory>())),
+ responseContext
+ )
+ );
+ for (OperatorFactory operatorFactory : opFactories) {
+ retVal = operatorFactory.wrap(retVal);
}
- );
+ return retVal;
+ };
+
+ return new OperatorSequence(opSupplier);
+ };
+
+ return (queryPlus, responseContext) -> {
+ final Query<RowsAndColumns> query = queryPlus.getQuery();
+ final ResultSerializationMode serializationMode =
query.context().getEnum(
+ ResultSerializationMode.CTX_SERIALIZATION_PARAMETER,
+ ResultSerializationMode.class,
+ ResultSerializationMode.ROWS
+ );
+ switch (serializationMode) {
+ case FRAMES:
+ return baseRunner.run(queryPlus, responseContext);
+ case ROWS:
+ return new
RowsAndColumnsUnravelingQueryRunner(baseRunner).run(queryPlus, responseContext);
Review Comment:
You are building a QueryRunner inside the `run()` method of a QueryRunner,
don't do that. Instead the QueryRunner should be able to do the right thing
based on the query that it got. In this case, you likely need to rename the
`RowsAndColumnsUnravelingQueryRunner` to
`RowsAndColumnsSerializationQueryRunner` and then make that query runner look
at the context and do the right thing.
Additionally, when you look at this code, you will see that it's weird that,
when it is asked for Frames, it returns a Sequence of RowsAndColumns. I
understand that this is because there is some other code that is going to try
to handle this, but as we have learned, that method is problematic in and of
itself already. We should really move to a world where that method isn't
necessary and we can do that by building the frame up here.
So, I would suggest embedding the frame-building code here and keep the
other method as, effectively a noop (the identity function)
--
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]
