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.

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

Extend Filters, extract Ids #509

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from
Draft

Extend Filters, extract Ids #509

wants to merge 11 commits into from

Conversation

rtroilo
Copy link
Member

@rtroilo rtroilo commented Jun 16, 2023

Please add the type of change as label. If your PR is not ready for review and merge, please add 🚧 to the PR title.

Description

Please add a clear and concise description of what your PR solves.

Corresponding issue

Closes #

New or changed dependencies

Checklist

Please check all finished tasks. If some tasks do not apply to your PR, please cross their text out (by using ~...~) and remove their checkboxes.

@rtroilo rtroilo requested a review from tyrasd June 16, 2023 15:10
@tyrasd tyrasd added the enhancement New feature or request label Jun 16, 2023
Copy link
Member

@tyrasd tyrasd left a comment

Choose a reason for hiding this comment

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

see a few minor comments inline below

this.ids = ids;
}

public Set<Long> getIds() {
Copy link
Member

Choose a reason for hiding this comment

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

this could be included in ParseTest.testIdFilterEqualsAnyOf to check if it returns the expected values

Comment on lines +43 to +45
if (filter instanceof TypeFilter typeFilter) {
type = EnumSet.of(typeFilter.getType());
} else {
Copy link
Member

Choose a reason for hiding this comment

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

I think this is missing a case: GeometryTypeFilters do implicitly also filter by type (e.g. geometry: polygon is equivalent to something like (type:way or (type:relation and type=multipolygon)) and <actual geometry check>.

Something like this should work:

Suggested change
if (filter instanceof TypeFilter typeFilter) {
type = EnumSet.of(typeFilter.getType());
} else {
if (filter instanceof TypeFilter typeFilter) {
type.retainAll(EnumSet.of(typeFilter.getType()));
} else if (filter instanceof GeometryTypeFilter geometryFilter) {
type.retainAll(geometryFilter.getOSMTypes());
} else {

Another question is if there are cases where we want the extractor to be called with GeometryTypeFilters. 🤔

compact.add(range);
entry.setValue(compact);
}
}
Copy link
Member

@tyrasd tyrasd Jun 28, 2023

Choose a reason for hiding this comment

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

We could move the code from optimizeFilters1 to this class?!

    public static Set<OSMType> extractTypes(FilterExpression expression) {
        return extractTypes(expression.normalize());
    }

    public static Set<OSMType> extractTypes(List<List<Filter>> normalized) {
        var allTypes = EnumSet.noneOf(OSMType.class);
        extract(normalized, ignored -> Stream.of(true)).keySet()
                .forEach(allTypes::addAll);
        return allTypes;
    }

and in MapReducer.optimizeFilters1:

    mapRed = mapRed.osmTypeInternal(FilterUtil.extractTypes(filter));

Copy link
Member

@tyrasd tyrasd left a comment

Choose a reason for hiding this comment

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

not sure if you noticed, but the indentation would need to be changed from four to two spaces in the new files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants