Adding filters for TimeBoundary on backend #3168

rajk-tetration · 2016-06-20T19:05:22Z

This adds support for adding filters in TimeBoundaryQuerys.

Questions:

In TimeBoundaryQueryRunnerFactory.java, did I add the functionality in a reasonable way?
In TimeBoundaryQueryRunnerTest.java, it isn't apparent that the filtering is working, since the current filter matches for all rows. I can't figure out how the dataSource() function works or how to find a list of dataSources and their associated logfiles, help on that would be appreciated as well.
What other changes do I need to make to finalize this? (I'm guessing there's some stuff to do with the JSON queries)

Thanks.

Signed-off-by: Balachandar Kesavan <[email protected]>

fjy · 2016-06-21T17:06:31Z

@rajk-tetration Thanks for this contribution. We'll try to get to reviewing it soon.

drcrallen · 2016-06-24T17:38:50Z

Tagging for 0.9.2 to make sure it doesn't get lost in the noise

gianm · 2016-06-24T22:11:28Z

@rajk-tetration, I haven't looked in detail yet, but I bet you'll also have to adjust filterSegments in TimeBoundaryQueryQueryToolChest (and add a test for the case where there is a filter).

For the test I see you had a comment like "no rows matched the filter--what is expected behavior in that case?". Probably the same as whatever an unfiltered timeBoundary would do if there were rows at all (I think that's returning an empty array, but double-check).

jon-wei · 2016-06-24T22:58:01Z

processing/src/main/java/io/druid/query/timeboundary/TimeBoundaryQueryRunnerFactory.java

+              };
+
+              if (legacyQuery.getDimensionsFilter() != null) {
+                /* Should make a function? The only difference between these two parts is the descending boolean. */


Sure, I think making this a function would be good

jon-wei · 2016-06-24T23:12:50Z

@rajk-tetration

1.) I think the TimeBoundaryQueryRunnerFactory implementation is reasonable

2.) For the QueryRunnerTestHelper, the datasource isn't actually used if you call

public static <T, QueryType extends Query<T>> List<QueryRunner<T>> makeQueryRunners(
      QueryRunnerFactory<T, QueryType> factory
  )

that function uses a hard-coded set of segments.

For testing, MultiSegmentSelectQueryTest may be a useful reference.

The default druid.sample.tsv is problematic for testing this filtering behavior as you pointed out; MultiSegmentSelectQueryTest creates its own segments so you'll have control over the timestamps.

The multi segment structure may also be useful for testing the filterSegments behavior that @gianm mentioned.

I think a change needed to finalize is to implement/verify that the filtering works on multiple segments.

rajk-tetration · 2016-06-28T20:13:42Z

Thanks for the comments.

@gianm I just deleted the filterSegments override and let it inherit; my reasoning was that there's no way to tell which segments are needed without the actual filter (I think similar to the TimeSeries case) - is this correct?

@jon-wei Thanks, that was really helpful, I was able to use it to do testing on multiple segments.

gianm · 2016-06-28T20:39:03Z

We really do want to filter the segments down when there is no filter - that's the common use case for timeBoundary and the optimization can make a huge difference.

rajk-tetration · 2016-06-28T21:44:54Z

@gianm Right, that makes sense. Updated.

fjy · 2016-07-01T20:05:55Z

processing/src/main/java/io/druid/query/timeboundary/TimeBoundaryQueryRunnerFactory.java

+      @Override
+      public Result<DateTime> apply(Cursor cursor)
+      {
+        if (cursor.isDone()) { return null; }


formatting issue

To clarify, should return null be on a separate line?

fjy · 2016-07-07T00:26:53Z

processing/src/main/java/io/druid/query/timeboundary/TimeBoundaryQuery.java

  private final String bound;

  @JsonCreator
  public TimeBoundaryQuery(
      @JsonProperty("dataSource") DataSource dataSource,
      @JsonProperty("intervals") QuerySegmentSpec querySegmentSpec,
      @JsonProperty("bound") String bound,
+      @JsonProperty("filter") DimFilter dimFilter,


This requires updating the timeboundary query documentation in docs/content/querying

fjy · 2016-07-07T00:28:23Z

👍 after docs updated.

gianm · 2016-07-07T18:24:09Z

@rajk-tetration TimeBoundaryQuery.getCacheKey needs to be updated too.

gianm · 2016-07-07T18:25:18Z

docs/content/querying/timeboundaryquery.md

@@ -9,6 +9,7 @@ Time boundary queries return the earliest and latest data points of a data set.
    "queryType" : "timeBoundary",
    "dataSource": "sample_datasource",
    "bound"     : < "maxTime" | "minTime" > # optional, defaults to returning both timestamps if not set 
+    "filter"    : { "type": "and", "fields": [<filter>, <filter>, ...] } # optional


Is there a reason this is an "and" filter rather than just <filter>?

I couldn't find an example of just using "filter": <filter> anywhere else in the docs, for example in TimeSeries an actual filter is given. I can change it, just wanted to be consistent.

hmm, fair enough, I guess this style predated your edit so feel free to leave it that way.

rajk-tetration · 2016-07-07T19:00:54Z

@gianm I'm not really sure what getCacheKey is, is it just a unique identifier for this query? Should I serialize the filter and add it to the buffer?

ghost · 2016-07-29T21:15:58Z

@gianm - do you have any advice/references on implementing getCacheKey?

gianm · 2016-07-29T21:27:52Z

@raj-kesavan sorry for the delay, I missed your first message…

getCacheKey is used for the query cache, which is per-query, per-segment. It should include anything that might affect the query result on a per segment basis. Filters definitely should be included. You could check out TimeseriesQueryQueryToolChests -> getCacheStrategy -> computeCacheKey for an example of how to do that.

fjy · 2016-08-03T02:01:49Z

👍

drcrallen · 2016-08-05T00:00:55Z

I'm really confused here, is this a PR from @raj-kesavan or @rajk-tetration ?

((edit: had double the same name))

drcrallen · 2016-08-05T00:01:39Z

((clarification is important for authorship and CLA guarantees)

gianm · 2016-08-05T14:27:20Z

processing/src/main/java/io/druid/query/timeboundary/TimeBoundaryQuery.java

-  {
-    return false;
-  }
+  public boolean hasFilters() { return dimFilter != null; }


Formatting is off here. You can use the IntelliJ/Eclipse style bundles in https://github.com/druid-io/druid/blob/master/CONTRIBUTING.md to format the code automatically.

gianm · 2016-08-05T14:54:47Z

@rajk-tetration looking good, just a few final comments from me!

drcrallen · 2016-08-05T16:12:48Z

@rajk-tetration Thanks for the contribution! Would you mind filling out the CLA at http://druid.io/community/cla.html please?

rajk-tetration · 2016-08-05T20:43:58Z

@gianm Does this look okay? I was able to import the xml file into IntelliJ but pressing "reformat code" never does anything.

@drcrallen Will do, thanks

gianm · 2016-08-05T21:04:23Z

@rajk-tetration, interesting, I wonder if you need to choose "Druid code style" in your IntelliJ code style settings? Anyway the formatting does look better now.

👍 from me after travis + CLA are sorted.

fjy · 2016-08-09T22:06:35Z

@rajk-tetration did you get a chance to sign the CLA?

fjy · 2016-08-11T14:24:40Z

@rajk-tetration

ghost · 2016-08-13T19:58:55Z

@fjy did you receive the CLA by e-mail?

fjy · 2016-08-15T17:25:21Z

Got CLA. We are good 👍

Balachandar Kesavan added 3 commits June 20, 2016 12:00

Adding filters for TimeBoundary on backend

1c6fb63

Signed-off-by: Balachandar Kesavan <[email protected]>

updating TimeBoundaryQuery constructor in QueryHostFinderTest

c2c3292

add filter helpers

3fc2ea0

nishantmonu51 added Feature Discuss labels Jun 22, 2016

drcrallen added this to the 0.9.2 milestone Jun 24, 2016

jon-wei reviewed Jun 24, 2016
View reviewed changes

update filterSegments + test

7ea155f

Conditional filterSegment depending on whether a filter exists

8d014e5

fjy reviewed Jul 1, 2016
View reviewed changes

Anonymous added 2 commits July 5, 2016 11:17

Style changes

2889bb5

Trigger rebuild

ae0a7b8

fjy reviewed Jul 7, 2016
View reviewed changes

Adding documentation for timeboundaryquery filtering

2591097

gianm reviewed Jul 7, 2016
View reviewed changes

added filter serialization to timeboundaryquery cache

aca3e2c

fjy changed the title ~~[Discuss] Adding filters for TimeBoundary on backend~~ Adding filters for TimeBoundary on backend Aug 3, 2016

fjy removed the Discuss label Aug 3, 2016

gianm reviewed Aug 5, 2016
View reviewed changes

code style changes

6e42b41

fjy merged commit 362b926 into apache:master Aug 15, 2016

fjy mentioned this pull request Aug 24, 2016

Support Min/Max for Timestamp #3299

Merged

seoeun25 pushed a commit to seoeun25/incubator-druid that referenced this pull request Feb 25, 2022

apache#3168 Improve timeout handling of timeseries query

a0034e3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding filters for TimeBoundary on backend #3168

Adding filters for TimeBoundary on backend #3168

rajk-tetration commented Jun 20, 2016 •

edited

Loading

fjy commented Jun 21, 2016

drcrallen commented Jun 24, 2016

gianm commented Jun 24, 2016

jon-wei Jun 24, 2016

jon-wei commented Jun 24, 2016 •

edited

Loading

rajk-tetration commented Jun 28, 2016 •

edited

Loading

gianm commented Jun 28, 2016

rajk-tetration commented Jun 28, 2016

fjy Jul 1, 2016

ghost Jul 4, 2016

fjy Jul 7, 2016

fjy commented Jul 7, 2016

gianm commented Jul 7, 2016

gianm Jul 7, 2016

rajk-tetration Jul 7, 2016 •

edited

Loading

gianm Jul 29, 2016

rajk-tetration commented Jul 7, 2016 •

edited

Loading

ghost commented Jul 29, 2016

gianm commented Jul 29, 2016

fjy commented Aug 3, 2016

drcrallen commented Aug 5, 2016 •

edited

Loading

drcrallen commented Aug 5, 2016

gianm Aug 5, 2016

gianm commented Aug 5, 2016

drcrallen commented Aug 5, 2016

rajk-tetration commented Aug 5, 2016 •

edited

Loading

gianm commented Aug 5, 2016

fjy commented Aug 9, 2016

fjy commented Aug 11, 2016

ghost commented Aug 13, 2016

fjy commented Aug 15, 2016

Adding filters for TimeBoundary on backend #3168

Adding filters for TimeBoundary on backend #3168

Conversation

rajk-tetration commented Jun 20, 2016 • edited Loading

fjy commented Jun 21, 2016

drcrallen commented Jun 24, 2016

gianm commented Jun 24, 2016

jon-wei Jun 24, 2016

Choose a reason for hiding this comment

jon-wei commented Jun 24, 2016 • edited Loading

rajk-tetration commented Jun 28, 2016 • edited Loading

gianm commented Jun 28, 2016

rajk-tetration commented Jun 28, 2016

fjy Jul 1, 2016

Choose a reason for hiding this comment

ghost Jul 4, 2016

Choose a reason for hiding this comment

fjy Jul 7, 2016

Choose a reason for hiding this comment

fjy commented Jul 7, 2016

gianm commented Jul 7, 2016

gianm Jul 7, 2016

Choose a reason for hiding this comment

rajk-tetration Jul 7, 2016 • edited Loading

Choose a reason for hiding this comment

gianm Jul 29, 2016

Choose a reason for hiding this comment

rajk-tetration commented Jul 7, 2016 • edited Loading

ghost commented Jul 29, 2016

gianm commented Jul 29, 2016

fjy commented Aug 3, 2016

drcrallen commented Aug 5, 2016 • edited Loading

drcrallen commented Aug 5, 2016

gianm Aug 5, 2016

Choose a reason for hiding this comment

gianm commented Aug 5, 2016

drcrallen commented Aug 5, 2016

rajk-tetration commented Aug 5, 2016 • edited Loading

gianm commented Aug 5, 2016

fjy commented Aug 9, 2016

fjy commented Aug 11, 2016

ghost commented Aug 13, 2016

fjy commented Aug 15, 2016

rajk-tetration commented Jun 20, 2016 •

edited

Loading

jon-wei commented Jun 24, 2016 •

edited

Loading

rajk-tetration commented Jun 28, 2016 •

edited

Loading

rajk-tetration Jul 7, 2016 •

edited

Loading

rajk-tetration commented Jul 7, 2016 •

edited

Loading

drcrallen commented Aug 5, 2016 •

edited

Loading

rajk-tetration commented Aug 5, 2016 •

edited

Loading