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

Basic Numeric and Boolean Filter #110

Merged
merged 2 commits into from
Sep 24, 2019
Merged

Basic Numeric and Boolean Filter #110

merged 2 commits into from
Sep 24, 2019

Conversation

CyanRook
Copy link
Contributor

@CyanRook CyanRook commented Sep 6, 2019 •
edited
Loading

A Basic Filter for VTQuery.

  • Enables a feature to be filter if they fail to match the submitted filters
    • A filter saying 'metric' >= 10 will only accept features with a 'metric' greater than or equal to 10.
  • Can use a collection of filters
  • Can filter numeric or boolean types
    • All numeric types get cast to double to be able to compare between the numeric types

Related Issues

Current Issues

  • Ugly variadic handling
  • No string handling due to complexity of UTF-8, string massaging, and undefined expectations for such a feature.

CyanRook
CyanRook commented Sep 6, 2019
View reviewed changes
src/vtquery.cpp
@@ -219,6 +231,108 @@ std::vector<vtzero::property> get_properties_vector(vtzero::feature& f) {
return v;
}

double convert_to_double(value_type value) {
Copy link
Contributor Author

@CyanRook CyanRook Sep 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Ran into type conversion errors when the filter was one type (float) and the parameter value was a different type (int64) so I just cast everything to double here. There is probably a better way to achieve this but I'm not super familiar with boost::variadic.

CyanRook
CyanRook commented Sep 9, 2019
View reviewed changes
src/vtquery.cpp Outdated
}

/// apply filters to a feature
bool filter_feature(vtzero::feature& feature, std::vector<filter_struct> filters) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Return true if a feature should be rejected. Maybe it should return false as it didn't pass the filter.

CyanRook
CyanRook commented Sep 9, 2019
View reviewed changes
src/vtquery.cpp Outdated
@@ -308,6 +388,10 @@ struct Worker : Nan::AsyncWorker {
while (auto feature = layer.next_feature()) {
auto original_geometry_type = get_geometry_type(feature);

if (filter_enabled && filter_feature(feature, filters)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we have filters and the filters say we should filter out this feature, skip to next feature

Copy link
Member

Choose a reason for hiding this comment

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

Helpful! 👍 Mind adding this as an inline comment?

@CyanRook CyanRook requested a review from mapsam September 9, 2019 15:35
@CyanRook CyanRook changed the title [Experimental] Basic filter Basic Numeric and Boolean Filter Sep 9, 2019
@CyanRook CyanRook marked this pull request as ready for review September 9, 2019 19:19
@CyanRook CyanRook requested a review from a team as a code owner September 9, 2019 19:19
CyanRook
CyanRook commented Sep 9, 2019
View reviewed changes
src/vtquery.cpp Outdated
} else if (filter.type == eq && feature_bool != filter_bool) {
reject_feature = true;
}
} else if (feature_value.which() != 0 && feature_value.which() != 5 && filter.value.which() != 0 && filter.value.which() != 5) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably should be 1 <= .which() <= 4 to indicate numeric types instead of != 0 and != 5

Copy link
Member

Choose a reason for hiding this comment

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

any reason why we can't apply these operations on uint (5) or sint (6) values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was following this and I think the uint and sint get mapped via the boost::variant defined as value_type to uint64_t and int64_t.
The value_type = boost::variant<std::string, float, double, int64_t, uint64_t, bool>; so the numeric ones are 1-5, unless I'm misunderstanding something.

@mapsam mapsam mentioned this pull request Sep 18, 2019
Closed
mapsam
mapsam requested changes Sep 18, 2019
View reviewed changes
Copy link
Member

@mapsam mapsam left a comment

Choose a reason for hiding this comment

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

@CyanRook this is fantastic! I just left a couple comments, and am only requesting changes on the testing front. I'll follow up with a new fixture in mvt-fixtures you can use to add additional tests.

test/vtquery.test.js Show resolved Hide resolved
src/vtquery.cpp Outdated
} else if (filter.type == eq && feature_bool != filter_bool) {
reject_feature = true;
}
} else if (feature_value.which() != 0 && feature_value.which() != 5 && filter.value.which() != 0 && filter.value.which() != 5) {
Copy link
Member

Choose a reason for hiding this comment

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

any reason why we can't apply these operations on uint (5) or sint (6) values?

src/vtquery.cpp Outdated
@@ -308,6 +388,10 @@ struct Worker : Nan::AsyncWorker {
while (auto feature = layer.next_feature()) {
auto original_geometry_type = get_geometry_type(feature);

if (filter_enabled && filter_feature(feature, filters)) {
Copy link
Member

Choose a reason for hiding this comment

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

Helpful! 👍 Mind adding this as an inline comment?

@mapsam mapsam mentioned this pull request Sep 23, 2019
Merged
@mapsam
Copy link
Member

mapsam commented Sep 23, 2019

@CyanRook @mapbox/mvt-fixtures@3.6.0 has been released.

As for the failing ASAN builds, I'll need to take a little bit of time to ramp up into those failures. Will report back soon!

@CyanRook
Copy link
Contributor Author

CyanRook commented Sep 23, 2019 •
edited
Loading

@mapsam Sounds good. I changed something that seemingly fixed codecov so all thats left is asam but I might have just masked the problem instead of fixing it.

@CyanRook
Feature - Basic Filtering [publish binary]
947971d 
* Implemented a basic optional filtering mechanism into VTQuery
* This allows VTQuery to ignore features that don't match the filters
* Enabled filtering for features that pass 'all' or 'any' of the filters.
* Remove Node v4/v6 targets
* Update to mvt-features to version 3.6.0
@mapsam mapsam mentioned this pull request Sep 24, 2019
Closed
mapsam
mapsam approved these changes Sep 24, 2019
View reviewed changes
Copy link
Member

@mapsam mapsam left a comment

Choose a reason for hiding this comment

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

@CyanRook this is such a great addition to vtquery. I'll plan on running out a 0.4.0 release in a separate branch/PR.

Per chat, we're going to save the ASAN build errors for another PR/ticket. #112

@mapsam mapsam merged commit 1805a37 into master Sep 24, 2019
@mapsam mapsam deleted the basic-filter branch September 24, 2019 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mapsam mapsam mapsam approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@CyanRook @mapsam