-
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 DimFilterHavingSpec. #3727
Add DimFilterHavingSpec. #3727
Conversation
ccc2ca3
to
6030148
Compare
@gianm i am wondering how is this different from having a filter (out side of the having) ? |
👍 |
@b-slim it's a HAVING filter, so it applies to the grouped results, not to the rows in the segment. You can use it to do things like only include groups that have sum(count) > x. Like the SQL |
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.
LGTM with minor comments
|
||
public class DimFilterHavingSpec implements HavingSpec | ||
{ | ||
private static final byte CACHE_KEY = (byte) 0x9; |
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 0x9? If this is a counting byte (0..8 mean something different), maybe declare them all at the same place?
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.
yeah, it's a counting byte. Currently they aren't centralized. I could do that in a follow on PR, I don't really want to mix the two 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.
@gianm agree
|
||
DimFilterHavingSpec that = (DimFilterHavingSpec) o; | ||
|
||
return dimFilter != null ? dimFilter.equals(that.dimFilter) : that.dimFilter == null; |
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.
dimFilter is non-null, so could be simplified unless you want to stick to "as IntelliJ generated" form.
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.
good point, I think I can just tell IntelliJ this is non-null and it will generate better code.
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.
Re-generated with assumption that dimFilter is non-null.
@Override | ||
public int hashCode() | ||
{ | ||
return dimFilter != null ? dimFilter.hashCode() : 0; |
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.
dimFilter is non-null, so could be simplified unless you want to stick to "as IntelliJ generated" form.
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.
Re-generated with assumption that dimFilter is non-null.
@gianm please bear with me the current example you posted is it possible to just use |
@b-slim it is possible to use GreaterThanHavingSpec for that example, but this PR would allow you to also use BoundDimFilter. Some advantages are:
|
@gianm Good point about re-using the syntax. |
6030148
to
9e6b3bb
Compare
let's add a test that uses extraction fns with the having spec filters, e.g.:
👍 otherwise |
@jon-wei added your test, thanks for writing it |
* Add DimFilterHavingSpec. * Add test for DimFilterHavingSpec with extractionFns.
Allows any DimFilters to be used as HAVING clauses.