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

adding Upper/Lower Bound Filter #1936

Merged
merged 1 commit into from
Dec 29, 2015
Merged

adding Upper/Lower Bound Filter #1936

merged 1 commit into from
Dec 29, 2015

Conversation

b-slim
Copy link
Contributor

@b-slim b-slim commented Nov 8, 2015

This Pr is Fix to #1879

himanshug
himanshug reviewed Nov 9, 2015
View reviewed changes
processing/src/main/java/io/druid/segment/filter/BetweenFilter.java
}
return (input.compareTo(betweenDimFilter.getLower()) <= 0) && (input.compareTo(betweenDimFilter.getUpper())
>= 0);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

this is probably going to have very weird results for the age use case e.g. this will say that age "100" is smaller than age "20" . I believe, you would need to at least support a flag saying whether the comparison should be done by converting string to numbers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@himanshug thanks, done

@drcrallen
Copy link
Contributor

@himanshug / @b-slim how would you feel if the numeric condition just used the comparator from io.druid.query.topn.AlphaNumericTopNMetricSpec ?

@drcrallen
Copy link
Contributor

and possibly make it "alphaNumeric" instead of just numeric?

@himanshug
Copy link
Contributor

@drcrallen @b-slim SGTM

@b-slim
Copy link
Contributor Author

b-slim commented Nov 9, 2015

@drcrallen thanks, please check it out

drcrallen
drcrallen reviewed Nov 10, 2015
View reviewed changes
processing/src/test/java/io/druid/query/filter/BetweenDimFilterTests.java
import java.io.IOException;
import java.util.Arrays;

public class BetweenDimFilterTests
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to test each of the return cases for isAlphaNumeric both true and false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@drcrallen done !

@drcrallen
Copy link
Contributor

Is it possible to test the apply for the 8 cases?

@xvrl
Copy link
Member

xvrl commented Nov 10, 2015

@b-slim would it make sense to add less than and greater than filters as well while we're at it?
Between could be expressed as a combination of those or have it's own implementation, depending on whether it's worth to optimize, but it seems a bit strange to have between but not < <= > >=

@b-slim
Copy link
Contributor Author

b-slim commented Nov 11, 2015

@xvrl i see how this can be useful, but i think we can have between, < <= > and >= all together.
i do prefer to have between since it saves the amount of bytes sent and and deserialized.
so i guess we can start with this, then i will send another PR of the rest of the troops.

@himanshug
Copy link
Contributor

@xvrl did you mean adding more filters or updating the api for same filter?
@b-slim @xvrl can we handle all cases by calling it a "bound" filter and based on whether user specified one or both of "lower" and "upper", the filter should be able to do appropriate thing.
so if user specifies
"lower" only : then it becomes a lower bound filter
"upper" only: then it becomes a upper bound filter
"lower" and "upper" both: then it becomes between filter

@drcrallen drcrallen mentioned this pull request Nov 16, 2015
Closed
drcrallen
drcrallen reviewed Nov 16, 2015
View reviewed changes
processing/src/main/java/io/druid/query/filter/DimFilterCacheHelper.java
@@ -35,6 +35,7 @@
static final byte JAVASCRIPT_CACHE_ID = 0x7;
static final byte SPATIAL_CACHE_ID = 0x8;
static final byte IN_CACHE_ID = 0x9;
public static byte BETWEEN_CACHE_ID = 0x10;
Copy link
Contributor

Choose a reason for hiding this comment

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

0xA?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@b-slim
Copy link
Contributor Author

b-slim commented Nov 21, 2015

@himanshug and @xvrl please checkout this.

@himanshug himanshug changed the title adding Between Filter adding Upper/Lower Bound Filter Nov 25, 2015
himanshug
himanshug reviewed Nov 25, 2015
View reviewed changes
docs/content/querying/filters.md
@@ -151,6 +151,97 @@ The grammar for a IN filter is as follows:
}
```

### Bound filter

Bound filter can be used to filter by a comparing dimension values to an upper value or/and a lower value.
Copy link
Contributor

Choose a reason for hiding this comment

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

"...by comparing dimension..."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

himanshug
himanshug reviewed Nov 25, 2015
View reviewed changes
processing/src/test/java/io/druid/query/filter/BetweenDimFilterTests.java
}

@Test
public void testSerDesBetweenFilter() throws IOException
Copy link
Contributor

Choose a reason for hiding this comment

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

Bound

@b-slim b-slim force-pushed the between_range_with_predicat branch from a6b4d14 to db6366c Compare December 8, 2015 15:36
@b-slim
Copy link
Contributor Author

b-slim commented Dec 8, 2015

@himanshug can u please check this

@fjy
Copy link
Contributor

fjy commented Dec 8, 2015

@b-slim Merge conflicts
@himanshug any more comments?

himanshug
himanshug reviewed Dec 9, 2015
View reviewed changes
processing/src/main/java/io/druid/query/filter/BoundDimFilter.java
.put(dimensionBytes)
.put((byte) 0xFF)
.put(upperBytes)
.put((byte) 0xFF)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think another PR introduced a separate variable for 0xFF in this context, can you use that?

@himanshug
Copy link
Contributor

👍 after #1936 (comment) is resolved, pls squash your commits.

@b-slim
Copy link
Contributor Author

b-slim commented Dec 10, 2015

@himanshug done

@b-slim b-slim closed this Dec 10, 2015
@b-slim b-slim reopened this Dec 10, 2015
@b-slim b-slim closed this Dec 10, 2015
@b-slim b-slim reopened this Dec 10, 2015
@himanshug
Copy link
Contributor

👍 for me

@b-slim
Copy link
Contributor Author

b-slim commented Dec 15, 2015

@fjy this is ready to go can you merge it please

@fjy
Copy link
Contributor

fjy commented Dec 29, 2015

I read this over again, and I think it is cool. 👍

fjy added a commit that referenced this pull request Dec 29, 2015
@fjy
Merge pull request #1936 from b-slim/between_range_with_predicat
e14ad74 
adding Upper/Lower Bound Filter
@fjy fjy merged commit e14ad74 into apache:master Dec 29, 2015
@fjy fjy mentioned this pull request Jan 26, 2016
Closed
@fjy fjy modified the milestone: 0.9.0 Feb 4, 2016
@fjy fjy mentioned this pull request Feb 5, 2016
Closed
seoeun25 pushed a commit to seoeun25/incubator-druid that referenced this pull request Jan 10, 2020
@navis
apache#1936 Exception thrown in processing row in index task should n…
e146f91 
…ot be regarded as parsing fail
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
0.9.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@b-slim @drcrallen @himanshug @xvrl @fjy