Fix bugs in query builders and in TimeBoundaryQuery.getFilter() by leventov · Pull Request #4131 · apache/druid

leventov · 2017-03-29T19:05:33Z

Add Query.getQueryMetrics() and Query.withQueryMetrics() for use in MetricsEmittingQueryRunner and CPUTimeMetricQueryRunner. This is needed to emit some dimensions/metrics during query execution in query engines.

Fix most query builder's copy(query) methods, they were non-static (that doesn't make sense) and "abandoned": didn't copy some of the query fields.
In query.withXxx() methods, use builders instead of calling constructors for brevity.
Fix a bug in TimeBoundaryQuery.getFilter(): was always returning null.

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

…used code in Druids

…ry.getDimensionsFilter()

drcrallen

These change make sense overall. There is a signature change on Query which I'm scratching my brain to see if is really needed.

It feels like something like the query tool chest should be ensuring the query is in the right kind of shape for being run, and I'm not convinced having the runners modify the Query object is the right kind of pattern in the long run.

Thoughts?

drcrallen · 2017-03-29T20:38:16Z

      boolean descending,
-      Map<String, Object> context
+      Map<String, Object> context,
+      QueryMetrics<?> queryMetrics


can we keep a constructor with the same signature for the sake of query extensions?

Added compatibility constructor

drcrallen · 2017-03-29T20:57:02Z

  )
  {
-    final Sequence<T> baseSequence = delegate.run(query, responseContext);
+    QueryMetrics<? super Query<T>> queryMetrics = queryToolChest.makeMetrics(query);


Should this only be done if query.getQueryMetrics() is null?

Makes sense, changed

drcrallen · 2017-03-29T20:58:53Z

-
    applyCustomDimensions.accept(queryMetrics);
-
+    final Query<T> queryWithMetrics = query.withQueryMetrics(queryMetrics);


same question, does this still need to be done if the query already has a QueryMetrics set in it?

leventov · 2017-03-29T21:04:57Z

@drcrallen

These change make sense overall. There is a signature change on Query which I'm scratching my brain to see if is really needed.

It feels like something like the query tool chest should be ensuring the query is in the right kind of shape for being run, and I'm not convinced having the runners modify the Query object is the right kind of pattern in the long run.

Didn't clearly understand, could you please elaborate?

…icQueryRunner and MetricsEmittingQueryRunner

drcrallen · 2017-03-29T21:26:35Z

@leventov what I mean is that if you look at the implementations of QueryRunner, they tend to not modify the query object EXCEPT a few cases where a special function in the QueryToolChest does modification.

As such, I'm questioning if modification of the query in arbitrary QueryRunners AND in the QueryToolChest makes sense. It feels like a bad precedent to set. I suggest looking into making the QueryToolChest the place to create the metrics items and attach them to the query, and the query runners simply consume the set values.

leventov · 2017-03-29T21:45:45Z

@drcrallen query object is already changed in the same way in some QueryRunners, e. g. FinalizeResultsQueryRunner and GroupByMergingQueryRunnerV2, via withOverriddenContext().

Generally when I was preparing this PR, I was thinking that currently the Query abstraction is overloaded, and should be split into:

Query -- set of configs that affect the result data. Cachable.
UniqueQuery/QueryInstance/? -- Query + identity of the query sent to the Druid cluster, i. e. queryId. Maybe don't need to be a separate abstraction, and should be merged with Query, or with QueryWithContext?
QueryWithContext/RuntimeQuery/RichQuery/? - UniqueQuery + something attached to it, that makes sense only during the query execution within the Druid cluster. I. e. queryMetrics and most things, Query.context property is currently used for, except (maybe) queryId.

Then QueryRunner.run() accepts QueryWithContext and is free to modify it, but not the underlying Query.

But anyway, I think it shouldn't be done as part of this PR, for compatibility reasons it might need to be delayed to 0.11.0.

himanshug · 2017-03-31T17:44:11Z

@leventov It appears that QueryMetrics is added to Query so that it could be passed around to various runners... how do you feel about changing QueryRunner.run(..) to accept ResponseContext instead of Map<String, Object> and keep QueryMetrics in ResponseContext . ResponseContext might look like...

ResponseContext {
  QueryMetrics queryMetrics,
  Map<String, Object> others, //current code context map
}

leventov · 2017-03-31T19:14:03Z

@himanshug as we discuss in #4113, responseContext shouldn't be used to pass anything to downstream query runners. Query.withOverriddenContext()/Query.getContext() should be used for that, it's a part of Query object. So I made QueryMetrics a part of Query object too.

On the other hand, the Query abstraction should probably be refactored into Query and QueryWithContext: #4131 (comment). But anyway, not as part of this PR and not in Druid 0.10.1.

leventov · 2017-04-04T02:58:50Z

@himanshug do you have other comments?

himanshug · 2017-04-06T15:34:55Z

@leventov sorry, I was away for a while.
However, i thought #4113 conclusion is to change the type of responseContext into something that is safer to use concurrently. responseContext is indeed there to pass around and gather various things.
Query context is more for various "flags/configuration" that are indication to query processing engine for various things. these may or may not be explicitly specified by the user.

That said, I might be wrong or we might want to change things in direction you're suggesting. So, please remind us to discuss this in next dev sync up and we can conclude it.

leventov · 2017-04-07T00:20:04Z

@himanshug I don't see what we disagree about. #4113 and #4131 (comment) couldn't be done in a minor Druid version update like 0.10.1, because they break custom user query types and query runners. This PR adds QueryMetrics in a compatible way for 0.10.x.

I agree that Query is not a very intuitive/suitable place for QueryMetrics, but responseContext is worse. I used to pass some equivalent of QueryMetrics via responseContext when initially implemented this functionality several months ago, and got concurrency issues: #3803. So I think Query is the most suitable place for QueryMetrics for now.

I won't be able to participate next week's dev sync

gianm · 2017-04-10T03:31:29Z

@leventov, in reply to:

I agree that Query is not a very intuitive/suitable place for QueryMetrics, but responseContext is worse. This PR adds QueryMetrics in a compatible way for 0.10.x.

This patch won't preserve extension compatibility anyway, since Query gained withQueryMetrics but BaseQuery doesn't provide a default implementation.

But also: is there a nice migration path from this change now, to something that would be a "better" design for 0.11.0? From your discussion with @himanshug, it sounds like there isn't, and in 0.11.0 we'd just want to remove these methods we're adding now and replace them with something else. I think if that's the case, it's fine to make the "better" change now and have the next release after 0.10.0 be 0.11.0.

leventov · 2017-04-10T05:57:07Z

@gianm uff, ok.

Could you please comment on #4131 (comment)? Also @drcrallen and @himanshug. I don't want to start implementing the change that people will dislike later. Because there is a lot of Query and QueryRunner code to change. (It couldn't be guaranteed that somebody will disagree when looking at actual PR, but I want to minimize probability of that.)

gianm · 2017-04-10T15:04:44Z

I'll take a closer look at that in a bit. I guess you would propose that query endpoints like QueryResource should accept a QueryWithContext? My first thought is that there's not a serious need to reorganize Query to split out the context. Some considerations:

IMO, we don't want to change the query API /druid/v2/ as part of this change. So queries submitted there should still include a context.
Sometimes "internal" context parameters are useful for users to be able to provide, like finalize (if a user wants to get complex metrics in raw form), queryId (if a user wants to use a specific query id).
We need a way for users to provide parameters that affect execution but not results. For example, priority, timeout, useCache, populateCache, groupByStrategy.
We need a way for brokers to pass down information to compute nodes, which needs to be part of the serialized form of a query. For example, when queries are issued from the broker, the groupBy query populates groupByStrategy if it wasn't set by the user, to ensure the same strategy is used on all nodes.
Maybe not the best, but, sometimes query context parameters that users provide do affect the results and need to be included in the cache key. For example: the timeseries parameter skipEmptyBuckets. That probably should have originally been a top level thing rather than a context parameter, but we should retain it in context for query API compatibility.

These points, taken as a whole, to me suggest it makes sense to keep the current design of Query. It satisfies all these needs well and does that in a relatively simple way.

It still makes sense to me to add a QueryPlus object and change QueryRunner to take that, but the "plus" wouldn't be query context, it would be probably response context and queryMetrics.

himanshug · 2017-04-10T15:12:04Z

@leventov @gianm I would say keep Query the way it is now with query context in it.
Regarding QueryRunner.run(QueryPlus) , I'm still not sure why it can't be QueryRunner.run(Query,ResponseContext) where ResponseContext has {QueryPlus - Query}. You do have to pass something in addition to Query to all the runners.

also @leventov do you agree with above but not in favor because it can't be made backward compatible?

@leventov I do agree that we need to minimize disagreements after large code body is written so before writing any code , let us get to some conclusion first.

gianm · 2017-04-10T15:18:39Z

Regarding QueryRunner.run(QueryPlus) , I'm still not sure why it can't be QueryRunner.run(Query,ResponseContext) where ResponseContext has {QueryPlus - Query}. You do have to pass something in addition to Query to all the runners.

QueryRunner.run(QueryPlus) and QueryRunner(Query, ExtraQueryStuff) are equivalent. The only reason I would suggest not naming ExtraQueryStuff as "ResponseContext" is for future proofing. It might have things in it that aren't response context. Like QueryMetrics, or future unforeseen uses. That way we don't have to change the API again if we want to add something else.

leventov · 2017-04-17T11:42:09Z

@gianm your comment: #4131 (comment) makes sense for me. But talking about QueryPlus, responseContext shouldn't be part of it, response context should be returned from QueryRunner.run(), see #4113 (comment).

…s-property

leventov · 2017-04-19T15:14:54Z

@gianm @himanshug After yesterday's dev sync, I created #4184 and removed query.queryMetrics property, as part of this PR. Only bug fixing / refactoring part of this PR is remaining.

jihoonson

@leventov the patch looks good to me. I left a trivial comment.

jihoonson · 2017-04-21T23:57:13Z

+  }

-    limitFn = postProcFn;
+  private static LimitSpec nullToNoopLimitSpec(LimitSpec limitSpec)


It looks to be useful in other places as well. How about moving to LimitSpec and make it public?

gianm

Left some comments about the groupBy builder. The rest looks good to me.

gianm · 2017-04-24T19:13:02Z


    public Builder setLimit(Integer limit)
    {
      this.limit = limit;


I think this needs to clear limitFn and limitSpec in order to force them to be recomputed, otherwise code like this would ignore the setLimit(10) part,

new GroupByQuery.Builder(query).setLimit(10).build();

Similar comment for any other function that writes to anything else that might modify limitFn, including orderByColumnSpecs, limit, havingSpec, or limitSpec. Some should clear both limitSpec and limitFn, some should only clear limitFn.

i think it would be less error prone to just remove limitSpec == null check and recreate it every time.

@gianm setLimit() was duplicating limit(), which had a saner implementation. Removed setLimit() and renamed limit() to setLimit(). Added postProcessingFn = null to some methods. Also never skip constructor checks now.

@himanshug if you mean recreate limitSpec every time in Builder.build(), it couldn't be done because limitSpec could be set directly.

yeah i meant to always recreate limitSpec . but i see, it can't be done due to explicit limitSpec set.

gianm · 2017-04-24T19:16:33Z

+    return postProcFn;
+  }

-    limitFn = postProcFn;


While you're at it, this should probably be called this.postProcFn since it's doing both LIMIT and HAVING.

Renamed to postProcessingFn, and renamed applyLimit() method to postProcess().

…s in GroupByQuery.Builder

gianm

👍

gianm · 2017-04-25T00:16:36Z

@drcrallen @himanshug able to take another look?

leventov · 2017-05-02T14:08:09Z

  }

-  protected Map<String, Object> computeOverridenContext(Map<String, Object> overrides)
+  protected static Map<String, Object> computeOverriddenContext(


This PR breaks compatibility of BaseQuery by refactoring this protected instance method into a static one. Is it a compatibility bug?

I'm not sure if BaseQuery is one of the "supposed to be stable" interfaces or if it's just Query / QueryRunner / QueryToolChest. Good question. We should probably have an annotation or something to make it clear.

I guess since all built-in queries extend BaseQuery, it's likely that extensions would too, so it would be kinder to them to keep compatibility there.

@drcrallen asked me to keep compatibility of BaseQuery in earlier review of this PR: #4131 (comment)

I found this incompatibility because our extension broke :)

For annotation, there is an issue: #4044

Ok, I'll make a PR that fixes incompatibility

Sounds good.

PR apache#4131 introduced a new copy builder for segmentMetadata that did not retain the value of usingDefaultInterval. This led to it being dropped and the default-interval handling not working as expected. Instead of using the default 1 week history when intervals are not provided, the segmentMetadata query would query _all_ segments, incurring an unexpected performance hit. This patch fixes the bug and adds a test for the copy builder.

* SegmentMetadataQuery: Fix default interval handling. PR #4131 introduced a new copy builder for segmentMetadata that did not retain the value of usingDefaultInterval. This led to it being dropped and the default-interval handling not working as expected. Instead of using the default 1 week history when intervals are not provided, the segmentMetadata query would query _all_ segments, incurring an unexpected performance hit. This patch fixes the bug and adds a test for the copy builder. * Intervals

* SegmentMetadataQuery: Fix default interval handling. PR apache#4131 introduced a new copy builder for segmentMetadata that did not retain the value of usingDefaultInterval. This led to it being dropped and the default-interval handling not working as expected. Instead of using the default 1 week history when intervals are not provided, the segmentMetadata query would query _all_ segments, incurring an unexpected performance hit. This patch fixes the bug and adds a test for the copy builder. * Intervals

* SegmentMetadataQuery: Fix default interval handling. PR #4131 introduced a new copy builder for segmentMetadata that did not retain the value of usingDefaultInterval. This led to it being dropped and the default-interval handling not working as expected. Instead of using the default 1 week history when intervals are not provided, the segmentMetadata query would query _all_ segments, incurring an unexpected performance hit. This patch fixes the bug and adds a test for the copy builder. * Intervals

leventov added 2 commits March 29, 2017 03:36

Add queryMetrics property to Query interface; Fix bugs and removed un…

234a761

…used code in Druids

Fix a bug in TimeBoundaryQuery.getFilter() and remove TimeBoundaryQue…

07703ff

…ry.getDimensionsFilter()

leventov added this to the 0.10.1 milestone Mar 29, 2017

leventov assigned himanshug and drcrallen Mar 29, 2017

drcrallen requested changes Mar 29, 2017

View reviewed changes

leventov added 2 commits March 29, 2017 15:11

Don't reassign query's queryMetrics if already present in CPUTimeMetr…

bac6957

…icQueryRunner and MetricsEmittingQueryRunner

Add compatibility constructor to BaseQuery

7210337

drcrallen approved these changes Mar 30, 2017

View reviewed changes

leventov mentioned this pull request Apr 17, 2017

Refactor QueryRunner.run(), make responseContext immutable #4113

Closed

leventov mentioned this pull request Apr 19, 2017

Make timeout behavior consistent to document #4134

Merged

leventov added 2 commits April 19, 2017 17:26

Merge remote-tracking branch 'upstream/master' into query-queryMetric…

9af2490

…s-property

Remove Query.queryMetrics property

3c15d1d

leventov changed the title ~~Add Query.queryMetrics property (part of #3798)~~ Fix bugs in query builders and in TimeBoundaryQuery.getFilter() Apr 19, 2017

leventov removed this from the 0.10.1 milestone Apr 19, 2017

leventov added the Bug label Apr 19, 2017

leventov closed this Apr 19, 2017

leventov reopened this Apr 19, 2017

leventov mentioned this pull request Apr 21, 2017

Refactor QueryRunner to accept QueryPlus: Query + QueryMetrics (part of #3798) #4184

Merged

jihoonson approved these changes Apr 22, 2017

View reviewed changes

gianm reviewed Apr 24, 2017

View reviewed changes

leventov added 2 commits April 25, 2017 00:13

Move nullToNoopLimitSpec() method to LimitSpec interface

472d08b

Rename GroupByQuery.applyLimit() to postProcess(); Fix inconsistencie…

eb77aab

…s in GroupByQuery.Builder

gianm approved these changes Apr 24, 2017

View reviewed changes

himanshug merged commit ee9b5a6 into apache:master Apr 25, 2017

himanshug added this to the 0.10.1 milestone Apr 25, 2017

leventov commented May 2, 2017

View reviewed changes

leventov mentioned this pull request May 2, 2017

Restore BaseQuery.computeOverridenContext() for compatibility #4241

Merged

gianm mentioned this pull request Jun 21, 2017

Add @ExtensionPoint and @PublicApi annotations. #4433

Merged

leventov deleted the query-queryMetrics-property branch July 14, 2017 13:51

gianm mentioned this pull request Mar 14, 2018

SegmentMetadataQuery: Fix default interval handling. #5489

Merged

clambertus unassigned himanshug and drcrallen Jul 6, 2018


		applyCustomDimensions.accept(queryMetrics);

		final Query<T> queryWithMetrics = query.withQueryMetrics(queryMetrics);

Conversation

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

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leventov commented Mar 29, 2017

Uh oh!

drcrallen commented Mar 29, 2017

Uh oh!

leventov commented Mar 29, 2017

Uh oh!

himanshug commented Mar 31, 2017

Uh oh!

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

Uh oh!

Uh oh!

leventov commented Apr 4, 2017

Uh oh!

himanshug commented Apr 6, 2017

Uh oh!

leventov commented Apr 7, 2017

Uh oh!

gianm commented Apr 10, 2017

Uh oh!

leventov commented Apr 10, 2017

Uh oh!

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

Uh oh!

Uh oh!

himanshug commented Apr 10, 2017

Uh oh!

gianm commented Apr 10, 2017

Uh oh!

leventov commented Apr 17, 2017

Uh oh!

leventov commented Apr 19, 2017

Uh oh!

jihoonson 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!

gianm 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!

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

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 Mar 29, 2017 •

edited

Loading

leventov commented Mar 31, 2017 •

edited

Loading

gianm commented Apr 10, 2017 •

edited

Loading

himanshug Apr 24, 2017 •

edited

Loading