Broker: Add ability to inline subqueries.#9533
Conversation
The main changes: - ClientQuerySegmentWalker: Add ability to inline queries. - Query: Add "getSubQueryId" and "withSubQueryId" methods. - QueryMetrics: Add "subQueryId" dimension. - ServerConfig: Add new "maxSubqueryRows" parameter, which is used by ClientQuerySegmentWalker to limit how many rows can be inlined per query. - IndexedTableJoinMatcher: Allow creating keys on top of unknown types, by assuming they are strings. This is useful because not all types are known for fields in query results. - InlineDataSource: Store RowSignature rather than component parts. Add more zealous "equals" and "hashCode" methods to ease testing. - Moved QuerySegmentWalker test code from CalciteTests and SpecificSegmentsQueryWalker in druid-sql to QueryStackTests in druid-server. Use this to spin up a new ClientQuerySegmentWalkerTest.
|
This pull request introduces 1 alert when merging 91ea2e9 into 4c620b8 - view on LGTM.com new alerts:
|
|
This pull request introduces 1 alert when merging 6f73c73 into 4c620b8 - view on LGTM.com new alerts:
|
|
This pull request introduces 1 alert when merging e6461d9 into 4c620b8 - view on LGTM.com new alerts:
|
It's complaining that a condition in an |
clintropolis
left a comment
clintropolis
left a comment
There was a problem hiding this comment.
this lgtm 👍
I've realized during review that the laning stuff from #9407 is only going to apply to queries that run on the cluster walker and not the local walker (since ClientQuerySegmentWalker was just a wrapper around the cluster walker previously so I didn't consider this really). This doesn't need to be changed now, and I'll look into doing in a follow-up to make these changes.
|
|
||
| final Pair<QueryRunnerFactoryConglomerate, Closer> conglomerate = CalciteTests.createQueryRunnerFactoryConglomerate(); | ||
| closer.register(conglomerate.rhs); | ||
| final QueryRunnerFactoryConglomerate conglomerate = QueryStackTests.createQueryRunnerFactoryConglomerate(closer); |
jihoonson
left a comment
jihoonson
left a comment
There was a problem hiding this comment.
LGTM. My comments are mostly about Javadoc and annotation. I'm fine with adding them in a follow-up PR.
| } | ||
|
|
||
| @JsonProperty | ||
| @JsonInclude(JsonInclude.Include.NON_NULL) |
There was a problem hiding this comment.
Good point, I'll add this in the next patch.
| @Nullable | ||
| String getId(); | ||
|
|
||
| Query<T> withSubQueryId(String subQueryId); |
There was a problem hiding this comment.
Maybe worth to add a javadoc explaining what is subQueryId and why we need it?
There was a problem hiding this comment.
Good idea, I'll add this in the next patch.
| * Historicals, which use {@link org.apache.druid.server.coordination.ServerManager}). Used by {@link QueryStackTests}. | ||
| * | ||
| * This class's logic is like a mashup of those two classes. With the right abstractions, it may be possible to get rid | ||
| * of this class and replace it with the production classes. |
There was a problem hiding this comment.
Should this class be mentioned in ServerManager and CachingClusteredClient so that these classes are synced up?
There was a problem hiding this comment.
I'll add this in the next patch.
|
|
||
| @JsonProperty | ||
| @Min(1) | ||
| private int maxSubqueryRows = 100000; |
There was a problem hiding this comment.
this should probably be added to the documentation
There was a problem hiding this comment.
I'm hoping to update all the docs at once in a future patch
4 participants
Changes: