Timeseries results are incoherent for case interval is out of range and case false filter. by b-slim · Pull Request #5649 · apache/druid

b-slim · 2018-04-16T15:21:59Z

Issue
Timeseries results are incoherent for case interval is out of range and case filter matching zero rows.
IMO intervals and filters need to behave the same thus if user issue a query with SkipEmptyBuckets=false with count aggregator result must be the similar.
Please look at the unit test to understand the issue (FYI it is not a SQL issue but SQL is a good way to express the issue).
~~Fix~~
TBH Am no sure what is the best way to fix this. I have added some add some nasty checks for the broker side that returns an empty query runner that does the trick but it make the code looks odd.
For the Histroical side i was able to added as part of the Timeseries Internal Query processing logic.
Am still thinking about a better way to fix this, the reason i am submitting this PR is to collect feedback/suggestions please let me know if you see a better way to fix this.
Fix V2 I have taken the route suggested by @gianm and add the the check to mergeResutls.
The new patch checks for empty results for case Granularity ALL and NoSkipping of empty buckets.

This change is

gianm · 2018-04-16T15:37:25Z

What do the test queries return without the patch?

b-slim · 2018-04-16T15:52:08Z

@gianm thanks for taking a look it returns nothing []

gianm · 2018-04-16T16:22:30Z

If the goal is to get timeseries results to be more SQL-compatible then this patch isn't quite right: we only want it to run for the granularity = all case. SQL queries should only return a "zeroed out" row when there is no grouping. And a timeseries query with granularity != all does have a grouping (something like group by floor(__time to X)), so it should return no rows at all rather than a "zeroed out" row.

It probably also makes more sense for this logic to be in the toolChest mergeResults rather than in the mergeRunners? mergeRunners from what I have seen is mainly concerned with taking care of running the runners in parallel, and mergeResults is mainly concerned about post-processing stuff.

gianm · 2018-04-16T16:23:47Z

@b-slim you probably also need to edit the test for testGroupByWithFilterMatchingNothing

gianm · 2018-04-16T16:24:40Z

And if we don't already have one, I'd add a test for a query like:

SELECT COUNT(*) FROM foo WHERE dim1 = 'nonexistent' GROUP BY FLOOR(__time TO DAY)

This one should return no rows.

b-slim · 2018-04-16T16:46:51Z

@gianm yes the main drive is to be more or like SQL standard. But also i though this can be seen as a bugISH case, if you consider interval as a filter then you would expect it to return results like other filter do. Anyway am okay with narrowing this to Granularity ALL only.

Change-Id: I92180498e2e6695212b286d980e349c136c78c86

Change-Id: I20c83095072bbf3b4a3a57dfc1934d528e2c7a1a

Change-Id: I1d88fab500c615bc46db4f4497ce93089976441f

b-slim · 2018-04-20T00:31:33Z

@gianm i have added the tests can you please take a quick look on this.

gianm · 2018-04-20T02:37:49Z

+        QUERY_CONTEXT_DONT_SKIP_EMPTY_BUCKETS,
+        "SELECT exp(count(*)) + 10, sum(m2)  FROM druid.foo WHERE  dim2 = 0",
+        CalciteTests.REGULAR_USER_AUTH_RESULT,
+        null,


Please include the query in these three tests - it really improves the quality of the tests, by making sure the planner is generating the exact query that we want, in addition to the results being correct.

sure will do.

gianm · 2018-04-20T03:17:09Z

+
+      final Sequence<Result<TimeseriesResultValue>> finalSequence;
+
+      if (query.getGranularity().equals(Granularities.ALL) && !query.isSkipEmptyBuckets() && baseResults.toList()


Calling baseResults.toList() here materializes the results, meaning in the case where it is not empty we will actually compute it twice. And also it won't stream. To fix this, you might be able to do a trick like the one for grandTotal, where the initial sequence is lazily mapped and then concatted with a new sequence that has the extra rows you want to add.

@gianm valid point but since the granularity is all, it is one data point anyway thus was assuming it is not too bad to call to.List(), what you think?

Ah yeah, that's a good point, I forgot about short-circuiting :)

IMO, to make the code clearer & more idiot-proof it would be good to move the toList inside the if and comment that it's ok here since there's at most one granularity. Then nobody will make the same mistake in reading it that I did.

sure will do.

Change-Id: I56cdd980e44f0685806efb45e29031fa2e328ec4

Change-Id: I42fdd28da5471f6ae57d3962f671741b106300cd

gianm

Hi @b-slim, just a couple minor comments left about the timestamp & testing.

gianm · 2018-04-20T16:03:48Z

+      final Sequence<Result<TimeseriesResultValue>> finalSequence;
+
+      if (query.getGranularity().equals(Granularities.ALL) && !query.isSkipEmptyBuckets()) {
+        //Usally it is NOT Okay to materialize results via toList(), but Granularity is ALL thus we have only one record


Hmm. I just realized this means that the query runner will now issue the query and block when run is called, rather when the sequence is actually accumulated. I think this is probably fine, at least, I can't think of a reason why it would break anything…

TBH same here and i feel using the merge sequence (streaming fashion) is gonna make code more unreadable for no big gains.

gianm · 2018-04-20T16:09:40Z

+                                      ), null));
+      aggregatorNames[i] = aggregatorSpecs.get(i).getName();
+    }
+    TimeseriesResultBuilder bob = new TimeseriesResultBuilder(DateTimes.EPOCH);


Usually timeseries with "all" granularity returns the start of the data interval as the timestamp. Here, there is no data interval (since there is no data), but it would probably be more consistent to return at least the start of the time interval. How about returning the start timestamp of the first interval in query.getIntervals()? And if there are no intervals (maybe the user specified []) then returning DateTimes.EPOCH.

I am not too picky about which way we go. But I do think that either way, the specific timestamp that gets returned can't be tested through the SQL layer (it would ignore the timestamp), so there should be a unit test added to TimeseriesQueryRunnerTest which verifies the timestamp.

@gianm good point, wondering if we should just set it to null since there is no data for that interval?

I think it should be non-null, since people have come to expect that for granularity: all. It would probably break stuff for it to be null.

Change-Id: I0bd414d2278e3eddc2810e4f5080e6cf6a117f12

Change-Id: I99a2380934c9ab350ca934c56041dc343c08b99f

b-slim · 2018-04-23T17:10:50Z

@gianm thanks I have added tests and the start intervals

gianm

Last couple of comments I think. Other than the parts I mentioned the rest looks good.

gianm · 2018-04-23T18:23:24Z

+
+      if (query.getGranularity().equals(Granularities.ALL) && !query.isSkipEmptyBuckets()) {
+        //Usally it is NOT Okay to materialize results via toList(), but Granularity is ALL thus we have only one record
+        finalSequence = baseResults.toList().isEmpty() ? Sequences.simple(Collections.singletonList(


I believe this will end up running the query on the cluster twice (once for the toList and then another time, later, when the finalSequence is accumulated). We should avoid this by caching baseResults.toList() and doing a Sequences.simple out of it once it's computed.

sure make sense

gianm · 2018-04-23T18:25:38Z

+        )
+    );
+
+    // Must create a toolChest so we can run mergeResults (which applies grand totals).


This comment doesn't make sense, since we aren't doing grand totals. Maybe it should say,

// Must create a toolChest so we can run mergeResults (which creates the zeroed-out row).

yeah copy and past form other test will fix it

Change-Id: I726a3b905a9520d8b1db70e4ba17853c65c414a4

b-slim · 2018-04-23T19:04:35Z

@gianm thanks

gianm

LGTM, thanks @b-slim

nishantmonu51

👍 LGTM post travis

…nd case false filter. (apache#5649) * adding some tests Change-Id: I92180498e2e6695212b286d980e349c136c78c86 * added empty sequence runner Change-Id: I20c83095072bbf3b4a3a57dfc1934d528e2c7a1a * treat only granularity ALL Change-Id: I1d88fab500c615bc46db4f4497ce93089976441f * moving toList within If and add expected queries Change-Id: I56cdd980e44f0685806efb45e29031fa2e328ec4 * typo Change-Id: I42fdd28da5471f6ae57d3962f671741b106300cd * adding tests and fix logic of intervals Change-Id: I0bd414d2278e3eddc2810e4f5080e6cf6a117f12 * fix style Change-Id: I99a2380934c9ab350ca934c56041dc343c08b99f * comments review Change-Id: I726a3b905a9520d8b1db70e4ba17853c65c414a4

b-slim assigned gianm Apr 16, 2018

b-slim added 3 commits April 16, 2018 20:44

adding some tests

cd02112

Change-Id: I92180498e2e6695212b286d980e349c136c78c86

added empty sequence runner

743e3e6

Change-Id: I20c83095072bbf3b4a3a57dfc1934d528e2c7a1a

treat only granularity ALL

05e65f2

Change-Id: I1d88fab500c615bc46db4f4497ce93089976441f

b-slim force-pushed the fix_timeseries branch from b8e9718 to 05e65f2 Compare April 17, 2018 21:58

b-slim assigned nishantmonu51 Apr 20, 2018

gianm reviewed Apr 20, 2018

View reviewed changes

b-slim added 2 commits April 20, 2018 08:36

moving toList within If and add expected queries

c4831cf

Change-Id: I56cdd980e44f0685806efb45e29031fa2e328ec4

typo

75920c0

Change-Id: I42fdd28da5471f6ae57d3962f671741b106300cd

b-slim added the Area - Querying label Apr 20, 2018

b-slim added this to the 0.13.0 milestone Apr 20, 2018

gianm reviewed Apr 20, 2018

View reviewed changes

b-slim added 2 commits April 23, 2018 09:55

adding tests and fix logic of intervals

96bde7e

Change-Id: I0bd414d2278e3eddc2810e4f5080e6cf6a117f12

fix style

69aea17

Change-Id: I99a2380934c9ab350ca934c56041dc343c08b99f

gianm reviewed Apr 23, 2018

View reviewed changes

comments review

5895699

Change-Id: I726a3b905a9520d8b1db70e4ba17853c65c414a4

gianm approved these changes Apr 23, 2018

View reviewed changes

nishantmonu51 approved these changes Apr 23, 2018

View reviewed changes

b-slim merged commit 73da742 into apache:master Apr 23, 2018

b-slim deleted the fix_timeseries branch April 26, 2018 01:27

KenjiTakahashi mentioned this pull request Jul 2, 2018

Inconsistent timeseries results for non-existing segments? #5923

Closed

clambertus unassigned gianm and nishantmonu51 Jul 6, 2018


		final Sequence<Result<TimeseriesResultValue>> finalSequence;

		if (query.getGranularity().equals(Granularities.ALL) && !query.isSkipEmptyBuckets() && baseResults.toList()

Uh oh!

Conversation

b-slim commented Apr 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gianm commented Apr 16, 2018

Uh oh!

b-slim commented Apr 16, 2018

Uh oh!

gianm commented Apr 16, 2018

Uh oh!

gianm commented Apr 16, 2018

Uh oh!

gianm commented Apr 16, 2018

Uh oh!

b-slim commented Apr 16, 2018

Uh oh!

b-slim commented Apr 20, 2018

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!

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!

b-slim commented Apr 23, 2018

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

b-slim commented Apr 23, 2018

Uh oh!

gianm left a comment

Choose a reason for hiding this comment

Uh oh!

nishantmonu51 left a comment • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

b-slim commented Apr 16, 2018 •

edited

Loading

nishantmonu51 left a comment •

edited

Loading