Refactor QueryRunner to accept QueryPlus: Query + QueryMetrics (part of #3798) by leventov · Pull Request #4184 · apache/druid

leventov · 2017-04-19T14:17:59Z

Add QueryPlus. Add QueryRunner.run(QueryPlus, Map) method with default implementation, to replace QueryRunner.run(Query, Map).

This PR is a rethink of #4131, designed for backwards compatibility with extensions.

A follow-up of #3954, a part of #3798.

…t implementation, to replace QueryRunner.run(Query, Map).

gianm · 2017-04-24T20:46:47Z

+  public Sequence<T> run(QuerySegmentWalker walker, Map<String, Object> context)
+  {
+    if (query instanceof BaseQuery) {
+      return ((BaseQuery) query).getQuerySegmentSpec().lookup(query, walker).run(this, context);


Why is this here instead of letting BaseQuery do it?

It means that Query and BaseQuery should learn about QueryPlus, that I didn't want to happen.

Oh, I see. Because "run" needs to get a QueryPlus, not a Query.

gianm · 2017-04-24T20:49:43Z

  String getType();

+  /**
+   * @deprecated use {@link QueryPlus#run(QuerySegmentWalker, Map)} instead. This method could be removed in the next


How would QueryPlus.run be implemented without being able to use this method?

QueryPlus.run() is implemented without using this method for BaseQuery, i. e. for all query types in the Druid repository (all Query impls in the Druid repo extend BaseQuery). QueryPlus.run() falls back to Query.run() only for "unknown" Query implementations in extensions. They should be forced to move to the new API when QueryRunner.run(Query, Map) method is removed in the next major or minor version of Druid.

What new API will they move to if they don't extend BaseQuery? Would you plan to add another method to Query in a later patch?

In backwards incompatible patch, Query.run(walker, context) is removed and Query.getRunner(walker, context) could be added. Then QueryPlus.run(walker, context) is implemented as this.query.getRunner(walker, context).run(this, context).

Could you add a comment describing this plan? Otherwise, anyone reading the code might be confused (like I was) about what the replacement is for this deprecated method that has no obvious replacement.

gianm · 2017-04-24T23:36:12Z

+ * An immutable composite object of {@link Query} + extra stuff needed in {@link QueryRunner}s. This "extra stuff"
+ * is only {@link QueryMetrics} yet.
+ */
+public final class QueryPlus<T>


If you change this to QueryPlus<T, QueryType extends Query<T>> would that reduce the amount of casting required from getQuery()?

Since QueryRunner is also parameterized only with T, the parameter of QueryRunner.run() could only be QueryPlus<T, ? extends Query<T>>, so it won't help much. Adding parameter to QueryRunner or run() method makes it too complex.

I think parametrization in this area pretty much doesn't work (but I don't think this is an issue), so I created QueryPlus to have the same parameter as Query and QueryRunner, to keep changes in this PR to minimum.

gianm · 2017-04-24T23:43:23Z

   */
-  Sequence<T> run(Query<T> query, Map<String, Object> responseContext);
+  @Deprecated
+  default Sequence<T> run(Query<T> query, Map<String, Object> responseContext)


Is anything in Druid still implementing this method, or did you convert them all to QueryPlus?

No implementations. This method is still called in benchmarks and tests at ~270 places.

gianm · 2017-04-25T00:06:58Z


    if (QueryContexts.isBySegment(query) || forceChainedExecution) {
-      return new ChainedExecutionQueryRunner(exec, queryWatcher, queryables).run(query, responseContext);
+      // TODO why query, not queryForRunners?


Ah, this should be queryForRunners. I think it won't have an effect in practice (since in this block we're already at the lowest level of the merge -- see comment above) so it's not a "bug". But nevertheless, queryForRunners would make more sense here.

…t-queryPlus

gianm

LGTM, but would appreciate a comment being added along the lines of #4184 (comment).

leventov · 2017-04-26T21:36:58Z

@gianm thanks for quick review!

himanshug · 2017-04-28T19:56:02Z

+    return doRun(baseRunner, QueryPlus.wrap(query), context);
+  }
+
+  protected Sequence<T> doRun(QueryRunner<T> baseRunner, QueryPlus<T> queryPlus, Map<String, Object> context)


this should be abstract , currently they look like in endless mutual recursion.

Such recursion is supposed to be broken by subclass who overrides one or both of the methods. The same mutual recursion is added in QueryRunner interface, in this PR.

himanshug · 2017-04-28T19:56:48Z

+   * @deprecated override {@link #doRun(QueryRunner, QueryPlus, Map)} instead
+   */
+  @Deprecated
+  protected Sequence<T> doRun(QueryRunner<T> baseRunner, Query<T> query, Map<String, Object> context)


we can remove it , its not part of any public extension

leventov · 2017-05-04T13:47:55Z

@himanshug do you have any other comments?

drcrallen

Overall looks good. Can you add a unit test showing the behavior you are wishing to preserve, with a javadoc comment on when/If the compatibility could be removed?

Also had a surprising change in the by segment query

drcrallen · 2017-05-04T15:28:11Z

                  @SuppressWarnings("unchecked")
                  final Sequence<Result<BySegmentResultValueClass<T>>> resultSequence = clientQueryable.run(
-                      bySegmentQuery.withQuerySegmentSpec(segmentSpec),
+                      queryPlus.withQuerySegmentSpec(segmentSpec),


This changed from the by segment query.

i think its same, bySegmentQuery was just a another name for query, but @leventov can confirm

yes, bySegmentQuery was just (Query<Result<BySegmentResultValueClass<T>>>) ((Query) query), casting the query reference without changing anything.

so behavior should still be the same.

himanshug · 2017-05-04T20:58:46Z

👍

gianm · 2017-05-07T19:36:20Z

@leventov is this still WIP?

leventov · 2017-05-07T21:57:02Z

@gianm I'm going to add test(s) @drcrallen has asked for

…moval plans in Druid 0.11 in Javadocs

…t-queryPlus

leventov · 2017-05-08T23:17:55Z

@drcrallen added test and clarified Javadocs

drcrallen

Thanks!

drcrallen · 2017-05-10T16:08:12Z

I'm holding off on merge for a few hours to give @cheddar a chance to check it out at a high level.

cheddar · 2017-05-10T18:39:21Z

Looked over the general interfaces and stuff and it looks good to me. The one bit of a rabbit hole I went down is why we are lazily instantiating the QueryMetrics instead of doing it at wrap, which was clearly because we need the Toolchest. That then got me thinking that it might be nice to actually attach the Toolchest to QueryPlus at some point as well.

👍

leventov · 2017-05-10T19:20:16Z

@cheddar thanks. Please merge?

Add QueryPlus. Add QueryRunner.run(QueryPlus, Map) method with defaul…

7a6905c

…t implementation, to replace QueryRunner.run(Query, Map).

leventov added this to the 0.10.1 milestone Apr 19, 2017

leventov requested review from cheddar, gianm and himanshug April 19, 2017 14:18

leventov mentioned this pull request Apr 19, 2017

Fix bugs in query builders and in TimeBoundaryQuery.getFilter() #4131

Merged

gianm reviewed Apr 25, 2017

View reviewed changes

gianm added the Improvement label Apr 25, 2017

leventov added 4 commits April 25, 2017 17:43

Fix GroupByMergingQueryRunnerV2

d9b919e

Fix QueryResourceTest

bc47882

Merge remote-tracking branch 'upstream/master' into queryRunner-accep…

05e760b

…t-queryPlus

Merge remote-tracking branch 'upstream/master' into queryRunner-accep…

a012f0a

…t-queryPlus

leventov added the Design Review label Apr 26, 2017

gianm approved these changes Apr 26, 2017

View reviewed changes

Expand the comment to Query.run(walker, context)

73cb936

himanshug reviewed Apr 28, 2017

View reviewed changes

Remove legacy version of BySegmentSkippingQueryRunner.doRun()

979ea64

drcrallen requested changes May 4, 2017

View reviewed changes

leventov added the WIP label May 5, 2017

leventov added 2 commits May 8, 2017 17:27

Add LegacyApiQueryRunnerTest and be more specific about legacy API re…

b389b9c

…moval plans in Druid 0.11 in Javadocs

Merge remote-tracking branch 'upstream/master' into queryRunner-accep…

9b96177

…t-queryPlus

leventov removed the WIP label May 8, 2017

drcrallen approved these changes May 10, 2017

View reviewed changes

drcrallen merged commit e09e892 into apache:master May 10, 2017

drcrallen deleted the queryRunner-accept-queryPlus branch May 10, 2017 19:25

leventov mentioned this pull request May 24, 2017

Remove QueryRunner.run(Query, resposnseContext) and related methods in Query #4324

Closed

leventov mentioned this pull request Jul 27, 2017

granularity method in QueryMetrics. #4570

Merged

jon-wei mentioned this pull request Sep 28, 2017

Druid 0.11.0 release notes #4876

Closed

Conversation

leventov commented Apr 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gianm left a comment

Choose a reason for hiding this comment

Uh oh!

leventov commented Apr 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leventov commented May 4, 2017

Uh oh!

drcrallen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

himanshug commented May 4, 2017

Uh oh!

gianm commented May 7, 2017

Uh oh!

leventov commented May 7, 2017

Uh oh!

leventov commented May 8, 2017

Uh oh!

drcrallen left a comment

Choose a reason for hiding this comment

Uh oh!

drcrallen commented May 10, 2017

Uh oh!

cheddar commented May 10, 2017

Uh oh!

leventov commented May 10, 2017

Uh oh!

Reviewers

Assignees

Labels

leventov commented Apr 19, 2017 •

edited

Loading