kgyrtkirk commented on code in PR #16800:
URL: https://github.com/apache/druid/pull/16800#discussion_r1692432686
##########
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
+ )
+ {
+ RowSignature rowSignature = resultArraySignature(query);
+ RowSignature modifiedRowSignature = useNestedForUnknownTypes
+ ?
FrameWriterUtils.replaceUnknownTypesWithNestedColumns(rowSignature)
+ : rowSignature;
Review Comment:
this method has `result` in its name; but it seems like the
`result*Signature` needed some fixups/checks; isn't this `modifiedRowSignature`
the right `signature` for the `query` ?
##########
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(
Review Comment:
I see that `FrameSignaturePair` is not new :)
I wonder if we could possibly merge it with
[FrameRowsAndColumns](https://github.com/apache/druid/pull/16658/files#diff-7bf8fe733f38a1b47bd8bedc6031cea286c5448d19d672b134958d31378bdb88R40)
later ?
##########
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
+ )
+ {
+ RowSignature rowSignature = resultArraySignature(query);
+ RowSignature modifiedRowSignature = useNestedForUnknownTypes
+ ?
FrameWriterUtils.replaceUnknownTypesWithNestedColumns(rowSignature)
Review Comment:
I don't understand why this is need ; what it does...
I think it would be very usefull to explain in the function's apidoc why
`replaceUnknownTypesWithNestedColumns` could be needed - and why that's
beneficial
##########
sql/src/test/java/org/apache/druid/sql/calcite/CalciteWindowQueryTest.java:
##########
@@ -222,6 +229,30 @@ public void windowQueryTest(String filename) throws
Exception
}
}
+ @MethodSource("parametersForWindowQueryTest")
+ @ParameterizedTest(name = "{0}")
+ @SuppressWarnings("unchecked")
+ public void windowQueryTestsWithSubqueryBytes(String filename) throws
Exception
Review Comment:
its a bit unclear for me why the need to re-add this dupliated run of all
testcases
there should be directed cases which are interesting...why run all? is any
of these failed?
##########
sql/src/test/java/org/apache/druid/sql/calcite/CalciteWindowQueryTest.java:
##########
@@ -73,6 +73,13 @@ public class CalciteWindowQueryTest extends
BaseCalciteQueryTest
QueryContexts.ENABLE_DEBUG, true
);
+ private static final Map<String, Object>
DEFAULT_QUERY_CONTEXT_WITH_SUBQUERY_BYTES =
+ ImmutableMap.<String, Object>builder()
+ .putAll(DEFAULT_QUERY_CONTEXT)
+ .put(QueryContexts.MAX_SUBQUERY_BYTES_KEY, "100000")
+ .put(QueryContexts.MAX_SUBQUERY_ROWS_KEY, "0")
Review Comment:
why the need to set `MAX_SUBQUERY_ROWS_KEY` to `0` ?
we have default as 100k and `@Min(1)` in [ServerConfig
defaults](https://github.com/apache/druid/blob/71725b41b54d13e1d4bd6c8b4f9b2972790d3dea/server/src/main/java/org/apache/druid/server/initialization/ServerConfig.java#L138-L140)
so without setting it like this its not even possible to set it to `0`
interestingly the way this value is read from the context could even read
negative values...
another interesting thing is that only `<0` is considered as unset in
[ClientQuerySegmentWrangler](https://github.com/apache/druid/blob/71725b41b54d13e1d4bd6c8b4f9b2972790d3dea/server/src/main/java/org/apache/druid/server/ClientQuerySegmentWalker.java#L669)
--
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]
