Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

GroupByQuery on PostAggregation #1292

Merged
merged 1 commit into from
Apr 17, 2015
Merged

GroupByQuery on PostAggregation #1292

merged 1 commit into from
Apr 17, 2015

Conversation

guobingkun
Copy link
Contributor

This patch aims to fix an issue that after Broker forwards GroupByQuery to Historical, "havingSpec" is still applied on PostAggregations. Since PostAggregations are removed from the forwarded query, "havingSpec" will likely evaluate to false on Historical in this case.

Here is a groupBy query that could replicate this issue on wikipedia local index data,

{
   "queryType":"groupBy",
   "dataSource":"wikipedia_index_test",
   "granularity":"day",
   "dimensions":[
      "page"
   ],  
   "aggregations":[
      {   
         "type":"count",
         "name":"rows"
      },  
      {   
         "type":"longSum",
         "fieldName":"added",
         "name":"added_count"
      }   
   ],  
   "postAggregations": [
      {   
         "type":"arithmetic",
         "name":"added_count_times_ten",
         "fn":"*",
         "fields":[
            {"type":"fieldAccess", "name":"added_count", "fieldName":"added_count"},
            {"type":"constant", "name":"const", "value":10}
         ]   
      }   
   ],  
   "having":{"type":"greaterThan", "aggregation":"added_count_times_ten", "value":9000.0},
   "intervals":[
       "2013-08-31T00:00/2013-09-02T00:00"
   ]   
}

When this query is sent to Broker, an empty set is returned.
Expected result is

[ {
            "version" : "v1",
            "timestamp" : "2013-08-31T00:00:00.000Z",
            "event" : {
                "added_count_times_ten" : 9050.0,
                "page" : "Crimson Typhoon",
                "added_count" : 905,
                "rows" : 1
            }
} ]

Send the same query directly to Historical will return the expected result.

@guobingkun guobingkun force-pushed the havingSpec_on_postAggregation branch from 763ef8b to 63f4b64 Compare April 17, 2015 03:14
drcrallen
drcrallen reviewed Apr 17, 2015
View reviewed changes
processing/src/test/java/io/druid/query/groupby/GroupByQueryRunnerTest.java
// add an extra layer of merging, simulate broker forwarding query to historical
TestHelper.assertExpectedObjects(
expectedResults,
factory.getToolchest().mergeResults(mergedRunner).run(fullQuery, context),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you decorate the runner with the pre merge and post merge decorators from the toolchest?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

@drcrallen
Copy link
Contributor

Thanks for the bug tracking and PR @guobingkun !

@guobingkun guobingkun force-pushed the havingSpec_on_postAggregation branch from 63f4b64 to 26f9ad6 Compare April 17, 2015 17:40
Fix an issue that after broker forwards GroupByQuery to historical, h…
cf155e4 
…avingSpec is still applied

on postAggregations which are removed in the forwarded query.

Add a unit test to replicate the issue.
Add a query that can replicate this issue into integration test.
@guobingkun guobingkun force-pushed the havingSpec_on_postAggregation branch from 26f9ad6 to cf155e4 Compare April 17, 2015 18:00
@drcrallen
Copy link
Contributor

👍

@drcrallen
Copy link
Contributor

@guobingkun : Could you please fill out the appropriate CLA at http://druid.io/community/cla.html ?

@guobingkun
Copy link
Contributor Author

Hi @drcrallen , I think I filed one already sometime yesterday?

@drcrallen
Copy link
Contributor

Yep, I see it now, thanks!

fjy added a commit that referenced this pull request Apr 17, 2015
@fjy
Merge pull request #1292 from guobingkun/havingSpec_on_postAggregation
eb1df42 
GroupByQuery on PostAggregation
@fjy fjy merged commit eb1df42 into apache:master Apr 17, 2015
@guobingkun guobingkun deleted the havingSpec_on_postAggregation branch April 17, 2015 19:28
@cheddar
Copy link
Contributor

cheddar commented Apr 18, 2015

@drcrallen Fwiw, I'm not sure which CLA you are believing that Bingkun has filled out, but he was added a while back to our corporate CLA.

@cheddar
Copy link
Contributor

cheddar commented Apr 18, 2015

Actually, no, he's apparently not covered yet. Need to correct that.

@fjy
Copy link
Contributor

fjy commented Apr 18, 2015

@cheddar @guobingkun filled out the individual CLA. He wasn't covered on the corporate CLA and we can't always tell which company someone is from :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@guobingkun @drcrallen @cheddar @fjy