Conversation
tyrasd
left a comment
There was a problem hiding this comment.
see a few minor comments inline below
oshdb-filter/src/main/java/org/heigit/ohsome/oshdb/filter/FilterUtil.java
Outdated
Show resolved
Hide resolved
| this.ids = ids; | ||
| } | ||
|
|
||
| public Set<Long> getIds() { |
There was a problem hiding this comment.
this could be included in ParseTest.testIdFilterEqualsAnyOf to check if it returns the expected values
Co-authored-by: Martin Raifer <martin.raifer@heigit.org>
| if (filter instanceof TypeFilter typeFilter) { | ||
| type = EnumSet.of(typeFilter.getType()); | ||
| } else { |
There was a problem hiding this comment.
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:
| 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); | ||
| } | ||
| } |
There was a problem hiding this comment.
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));
tyrasd
left a comment
There was a problem hiding this comment.
not sure if you noticed, but the indentation would need to be changed from four to two spaces in the new files.
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.