-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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.
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
Add dimension extraction functionality to SearchQuery #1712
Conversation
035229a
to
46ddda7
Compare
@Override | ||
public int compare(DimensionSpec o1, DimensionSpec o2) | ||
{ | ||
return o1.getOutputName().compareTo(o2.getOutputName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably sorting should be done based on DimensionSpec#dimension instead of outputName.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
multiple queries having different outputName but on the same column maps to same cache key and results in same intermediate results.
CachingClusteredClientTest.testGroupByCachingRenamedAggs does similar testing for groupBy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets also add a UT to CachingClusteredClient for testing caching with renamed dimensions while at it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking into it more I think both implementations are broken. io.druid.query.search.SearchQueryRunner#run
uses the specified ordering to search through the dimensions, and if there is a limit specified, then the results are going to be dependent on the query-specified ordering of the dimensions, and how many results each one of those returned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modified to use same ordering as the query uses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a unit test for the failure would be good to have.
Travis failed in hadoop (stalled?) restarting. |
46ddda7
to
a43817b
Compare
+1 |
Travis failed in unrelated |
@@ -104,9 +120,9 @@ public static AndDimFilterBuilder newAndDimFilterBuilder() | |||
|
|||
/** | |||
* A Builder for OrDimFilter. | |||
* | |||
* <p/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we change the default template to not add those empty tags, Java 8 javadocs does not like them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer that we don't change formatting on the entire file, when doing only localized change, but that's just my personal preference. It makes code reviews harder to try to weed out formatting changes from code changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed in this PR
66dc80f
to
df58995
Compare
LGTM once squashed to remove the extra changes |
* Add IdentityExtractionFn
df58995
to
e569f4b
Compare
Add dimension extraction functionality to SearchQuery
Dimension extraction did not work for search query prior to this.