From dc6a70f2c51969df142f0e520833a1d72267129a Mon Sep 17 00:00:00 2001 From: Brian Sperlongano Date: Sat, 30 Apr 2022 10:12:42 -0400 Subject: [PATCH 01/19] Add failing test cases --- .../expression/MultiExpressionTest.java | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/planetiler-core/src/test/java/com/onthegomap/planetiler/expression/MultiExpressionTest.java b/planetiler-core/src/test/java/com/onthegomap/planetiler/expression/MultiExpressionTest.java index 31845e7695..767cc8aae9 100644 --- a/planetiler-core/src/test/java/com/onthegomap/planetiler/expression/MultiExpressionTest.java +++ b/planetiler-core/src/test/java/com/onthegomap/planetiler/expression/MultiExpressionTest.java @@ -298,6 +298,41 @@ void testNot() { assertSameElements(List.of(), index.getMatches(featureWithTags())); } + @Test + void testNor() { + var index = MultiExpression.of(List.of( + entry("a", not(or( + matchAny("key1", "val1"), + matchAny("key2", "val2") + ))) + )).index(); + assertSameElements(List.of(), index.getMatches(featureWithTags("key1", "val1", "key2", "val2"))); + assertSameElements(List.of(), index.getMatches(featureWithTags("key1", "val1", "key2", "val2", "key3", "val3"))); + assertSameElements(List.of(), index.getMatches(featureWithTags("key1", "no", "key2", "val2"))); + assertSameElements(List.of(), index.getMatches(featureWithTags("key1", "val1", "key2", "no"))); + assertSameElements(List.of(), index.getMatches(featureWithTags("key1", "val1"))); + assertSameElements(List.of(), index.getMatches(featureWithTags("key2", "val2"))); + assertSameElements(List.of("a"), index.getMatches(featureWithTags())); + assertSameElements(List.of("a"), index.getMatches(featureWithTags("key1", "no", "key2", "no"))); + } + + @Test + void testNand() { + var index = MultiExpression.of(List.of( + entry("a", not(and( + matchAny("key1", "val1"), + matchAny("key2", "val2") + ))) + )).index(); + assertSameElements(List.of(), index.getMatches(featureWithTags("key1", "val1", "key2", "val2"))); + assertSameElements(List.of(), index.getMatches(featureWithTags("key1", "val1", "key2", "val2", "key3", "val3"))); + assertSameElements(List.of("a"), index.getMatches(featureWithTags())); + assertSameElements(List.of("a"), index.getMatches(featureWithTags("key1", "val1"))); + assertSameElements(List.of("a"), index.getMatches(featureWithTags("key2", "val2"))); + assertSameElements(List.of("a"), index.getMatches(featureWithTags("key1", "val1", "key2", "no"))); + assertSameElements(List.of("a"), index.getMatches(featureWithTags("key1", "no", "key2", "val2"))); + } + @Test void testXor() { var index = MultiExpression.of(List.of( From 514259b4daa996a308304443411acd2e902444e0 Mon Sep 17 00:00:00 2001 From: Brian Sperlongano Date: Sat, 30 Apr 2022 12:11:02 -0400 Subject: [PATCH 02/19] Add missing NOT key checks --- .../com/onthegomap/planetiler/expression/MultiExpression.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/MultiExpression.java b/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/MultiExpression.java index 87a438e37b..ecfbcc1571 100644 --- a/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/MultiExpression.java +++ b/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/MultiExpression.java @@ -69,6 +69,7 @@ private static void getRelevantKeys(Expression exp, Consumer acceptKey) } else if (exp instanceof Expression.Or or) { or.children().forEach(child -> getRelevantKeys(child, acceptKey)); } else if (exp instanceof Expression.Not not) { + getRelevantKeys(not.child(), acceptKey); getRelevantMissingKeys(not.child(), acceptKey); } else if (exp instanceof Expression.MatchField field) { acceptKey.accept(field.field()); @@ -88,6 +89,7 @@ private static void getRelevantMissingKeys(Expression exp, Consumer acce or.children().forEach(child -> getRelevantMissingKeys(child, acceptKey)); } else if (exp instanceof Expression.Not not) { getRelevantKeys(not.child(), acceptKey); + getRelevantMissingKeys(not.child(), acceptKey); } else if (exp instanceof Expression.MatchAny any && any.matchWhenMissing()) { acceptKey.accept(any.field()); } @@ -100,7 +102,7 @@ public Index index() { } boolean caresAboutGeometryType = expressions.stream().anyMatch(entry -> entry.expression.contains(exp -> exp instanceof Expression.MatchType)); - return caresAboutGeometryType ? new GeometryTypeIndex<>(this) : new KeyIndex<>(simplify()); + return caresAboutGeometryType ? new GeometryTypeIndex<>(simplify()) : new KeyIndex<>(simplify()); } /** Returns a copy of this multi-expression that replaces every expression using {@code mapper}. */ From d7e275c1be54f3aeafda5e1f1f85456a1d3a9533 Mon Sep 17 00:00:00 2001 From: Brian Sperlongano Date: Sat, 30 Apr 2022 12:12:37 -0400 Subject: [PATCH 03/19] Undo unrelated simplify --- .../com/onthegomap/planetiler/expression/MultiExpression.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/MultiExpression.java b/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/MultiExpression.java index ecfbcc1571..86a0432a2c 100644 --- a/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/MultiExpression.java +++ b/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/MultiExpression.java @@ -102,7 +102,7 @@ public Index index() { } boolean caresAboutGeometryType = expressions.stream().anyMatch(entry -> entry.expression.contains(exp -> exp instanceof Expression.MatchType)); - return caresAboutGeometryType ? new GeometryTypeIndex<>(simplify()) : new KeyIndex<>(simplify()); + return caresAboutGeometryType ? new GeometryTypeIndex<>(this) : new KeyIndex<>(simplify()); } /** Returns a copy of this multi-expression that replaces every expression using {@code mapper}. */ From b2ab8a82ca0c4924cb327b213621e113e8ba8854 Mon Sep 17 00:00:00 2001 From: Brian Sperlongano Date: Sat, 30 Apr 2022 14:49:45 -0400 Subject: [PATCH 04/19] Add and/or constant tests --- .../expression/MultiExpressionTest.java | 58 +++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/planetiler-core/src/test/java/com/onthegomap/planetiler/expression/MultiExpressionTest.java b/planetiler-core/src/test/java/com/onthegomap/planetiler/expression/MultiExpressionTest.java index 31845e7695..0878261b5d 100644 --- a/planetiler-core/src/test/java/com/onthegomap/planetiler/expression/MultiExpressionTest.java +++ b/planetiler-core/src/test/java/com/onthegomap/planetiler/expression/MultiExpressionTest.java @@ -263,6 +263,35 @@ void testAnd() { assertSameElements(List.of(), index.getMatches(featureWithTags())); } + @Test + void testAndConstant() { + var index = MultiExpression.of(List.of( + entry("a", and( + Expression.TRUE, + matchAny("key1", "val1") + )) + )).index(); + assertSameElements(List.of("a"), index.getMatches(featureWithTags("key1", "val1", "key2", "val2"))); + assertSameElements(List.of(), index.getMatches(featureWithTags("key1", "no", "key2", "val2"))); + assertSameElements(List.of("a"), index.getMatches(featureWithTags("key1", "val1", "key2", "no"))); + assertSameElements(List.of("a"), index.getMatches(featureWithTags("key1", "val1"))); + assertSameElements(List.of(), index.getMatches(featureWithTags("key2", "val2"))); + assertSameElements(List.of(), index.getMatches(featureWithTags())); + + index = MultiExpression.of(List.of( + entry("a", and( + Expression.FALSE, + matchAny("key1", "val1") + )) + )).index(); + assertSameElements(List.of(), index.getMatches(featureWithTags("key1", "val1", "key2", "val2"))); + assertSameElements(List.of(), index.getMatches(featureWithTags("key1", "no", "key2", "val2"))); + assertSameElements(List.of(), index.getMatches(featureWithTags("key1", "val1", "key2", "no"))); + assertSameElements(List.of(), index.getMatches(featureWithTags("key1", "val1"))); + assertSameElements(List.of(), index.getMatches(featureWithTags("key2", "val2"))); + assertSameElements(List.of(), index.getMatches(featureWithTags())); + } + @Test void testOr() { var index = MultiExpression.of(List.of( @@ -281,6 +310,35 @@ void testOr() { assertSameElements(List.of(), index.getMatches(featureWithTags())); } + @Test + void testOrConstant() { + var index = MultiExpression.of(List.of( + entry("a", or( + Expression.TRUE, + matchAny("key1", "val1") + )) + )).index(); + assertSameElements(List.of("a"), index.getMatches(featureWithTags("key1", "val1", "key2", "val2"))); + assertSameElements(List.of("a"), index.getMatches(featureWithTags("key1", "no", "key2", "val2"))); + assertSameElements(List.of("a"), index.getMatches(featureWithTags("key1", "val1", "key2", "no"))); + assertSameElements(List.of("a"), index.getMatches(featureWithTags("key1", "val1"))); + assertSameElements(List.of("a"), index.getMatches(featureWithTags("key2", "val2"))); + assertSameElements(List.of("a"), index.getMatches(featureWithTags())); + + index = MultiExpression.of(List.of( + entry("a", or( + Expression.FALSE, + matchAny("key1", "val1") + )) + )).index(); + assertSameElements(List.of("a"), index.getMatches(featureWithTags("key1", "val1", "key2", "val2"))); + assertSameElements(List.of(), index.getMatches(featureWithTags("key1", "no", "key2", "val2"))); + assertSameElements(List.of("a"), index.getMatches(featureWithTags("key1", "val1", "key2", "no"))); + assertSameElements(List.of("a"), index.getMatches(featureWithTags("key1", "val1"))); + assertSameElements(List.of(), index.getMatches(featureWithTags("key2", "val2"))); + assertSameElements(List.of(), index.getMatches(featureWithTags())); + } + @Test void testNot() { var index = MultiExpression.of(List.of( From e5315b94b9df97afa9e933b9e0fe89ede2d294d1 Mon Sep 17 00:00:00 2001 From: Mike Barry Date: Sun, 1 May 2022 07:42:24 -0400 Subject: [PATCH 05/19] More robust multiexpression filtering --- .../expression/MultiExpression.java | 80 +++++++++---------- 1 file changed, 37 insertions(+), 43 deletions(-) diff --git a/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/MultiExpression.java b/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/MultiExpression.java index 86a0432a2c..125d7b6c0f 100644 --- a/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/MultiExpression.java +++ b/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/MultiExpression.java @@ -19,6 +19,8 @@ import java.util.function.Predicate; import java.util.function.UnaryOperator; import java.util.stream.Collectors; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** * A list of {@link Expression Expressions} to evaluate on input elements. @@ -33,6 +35,7 @@ */ public record MultiExpression (List> expressions) { + private static final Logger LOGGER = LoggerFactory.getLogger(MultiExpression.class); private static final Comparator BY_ID = Comparator.comparingInt(WithId::id); public static MultiExpression of(List> expressions) { @@ -69,28 +72,12 @@ private static void getRelevantKeys(Expression exp, Consumer acceptKey) } else if (exp instanceof Expression.Or or) { or.children().forEach(child -> getRelevantKeys(child, acceptKey)); } else if (exp instanceof Expression.Not not) { - getRelevantKeys(not.child(), acceptKey); - getRelevantMissingKeys(not.child(), acceptKey); + if (not.child()instanceof Expression.MatchAny any && any.matchWhenMissing()) { + acceptKey.accept(any.field()); + } } else if (exp instanceof Expression.MatchField field) { acceptKey.accept(field.field()); - } else if (exp instanceof Expression.MatchAny any) { - acceptKey.accept(any.field()); - } - } - - /** - * Calls {@code acceptKey} for every tag that, when missing, could possibly cause {@code exp} to match an input - * element. - */ - private static void getRelevantMissingKeys(Expression exp, Consumer acceptKey) { - if (exp instanceof Expression.And and) { - and.children().forEach(child -> getRelevantMissingKeys(child, acceptKey)); - } else if (exp instanceof Expression.Or or) { - or.children().forEach(child -> getRelevantMissingKeys(child, acceptKey)); - } else if (exp instanceof Expression.Not not) { - getRelevantKeys(not.child(), acceptKey); - getRelevantMissingKeys(not.child(), acceptKey); - } else if (exp instanceof Expression.MatchAny any && any.matchWhenMissing()) { + } else if (exp instanceof Expression.MatchAny any && !any.matchWhenMissing()) { acceptKey.accept(any.field()); } } @@ -222,54 +209,61 @@ private static class KeyIndex implements Index { private final Map>> keyToExpressionsMap; // same as keyToExpressionsMap but as a list (optimized for iteration when # source feature keys > # tags we care about) private final List>>> keyToExpressionsList; - // expressions that should match when certain tags are *not* present on an input element - private final List>>> missingKeyToExpressionList; - // expressions that match a constant true input element - private final List> constantTrueExpressionList; + // expressions that must always be evaluated on each input element + private final List> alwaysEvaluateExpressionList; private KeyIndex(MultiExpression expressions) { int id = 1; // build the indexes Map>> keyToExpressions = new HashMap<>(); - Map>> missingKeyToExpressions = new HashMap<>(); - List> constants = new ArrayList<>(); + List> always = new ArrayList<>(); for (var entry : expressions.expressions) { Expression expression = entry.expression; EntryWithId expressionValue = new EntryWithId<>(entry.result, expression, id++); - getRelevantKeys(expression, - key -> keyToExpressions.computeIfAbsent(key, k -> new HashSet<>()).add(expressionValue)); - getRelevantMissingKeys(expression, - key -> missingKeyToExpressions.computeIfAbsent(key, k -> new HashSet<>()).add(expressionValue)); - if (expression.equals(TRUE)) { - constants.add(expressionValue); + if (mustAlwaysEvaluate(expression)) { + always.add(expressionValue); + } else { + getRelevantKeys(expression, + key -> keyToExpressions.computeIfAbsent(key, k -> new HashSet<>()).add(expressionValue)); } } // create immutable copies for fast iteration at matching time - constantTrueExpressionList = List.copyOf(constants); + if (!always.isEmpty()) { + LOGGER.warn("{} expressions will be evaluated for every element:", always.size()); + for (var expression : always) { + LOGGER.warn(" {}", expression.expression.generateJavaCode()); + } + } + alwaysEvaluateExpressionList = List.copyOf(always); keyToExpressionsMap = keyToExpressions.entrySet().stream().collect(Collectors.toUnmodifiableMap( Map.Entry::getKey, entry -> entry.getValue().stream().toList() )); keyToExpressionsList = List.copyOf(keyToExpressionsMap.entrySet()); - missingKeyToExpressionList = missingKeyToExpressions.entrySet().stream() - .map(entry -> Map.entry(entry.getKey(), entry.getValue().stream().toList())).toList(); numExpressions = id; } + private boolean mustAlwaysEvaluate(Expression expression) { + if (expression instanceof Expression.Or or) { + return or.children().stream().anyMatch(this::mustAlwaysEvaluate); + } else if (expression instanceof Expression.And and) { + return and.children().stream().allMatch(this::mustAlwaysEvaluate); + } else if (expression instanceof Expression.Not not) { + return !mustAlwaysEvaluate(not.child()); + } else if (expression instanceof Expression.MatchAny any && any.matchWhenMissing()) { + return true; + } else { + return TRUE.equals(expression); + } + } + /** Lookup matches in this index for expressions that match a certain type. */ @Override public List> getMatchesWithTriggers(SourceFeature input) { List> result = new ArrayList<>(); boolean[] visited = new boolean[numExpressions]; - for (var entry : constantTrueExpressionList) { - result.add(new Match<>(entry.result, List.of(), entry.id)); - } - for (var entry : missingKeyToExpressionList) { - if (!input.hasTag(entry.getKey())) { - visitExpressions(input, result, visited, entry.getValue()); - } - } + visitExpressions(input, result, visited, alwaysEvaluateExpressionList); Map tags = input.tags(); if (tags.size() < keyToExpressionsMap.size()) { for (String inputKey : tags.keySet()) { From 663282caa1ec773e828a4a656de019d323ad77d0 Mon Sep 17 00:00:00 2001 From: Mike Barry Date: Sun, 1 May 2022 07:47:33 -0400 Subject: [PATCH 06/19] move --- .../expression/MultiExpression.java | 50 ++++++++++--------- 1 file changed, 26 insertions(+), 24 deletions(-) diff --git a/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/MultiExpression.java b/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/MultiExpression.java index 125d7b6c0f..c8cdbccd48 100644 --- a/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/MultiExpression.java +++ b/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/MultiExpression.java @@ -65,20 +65,36 @@ private static void visitExpressions(SourceFeature input, List> res } } + private static boolean mustAlwaysEvaluate(Expression expression) { + if (expression instanceof Expression.Or or) { + return or.children().stream().anyMatch(MultiExpression::mustAlwaysEvaluate); + } else if (expression instanceof Expression.And and) { + return and.children().stream().allMatch(MultiExpression::mustAlwaysEvaluate); + } else if (expression instanceof Expression.Not not) { + return !mustAlwaysEvaluate(not.child()); + } else if (expression instanceof Expression.MatchAny any && any.matchWhenMissing()) { + return true; + } else { + return TRUE.equals(expression); + } + } + /** Calls {@code acceptKey} for every tag that could possibly cause {@code exp} to match an input element. */ private static void getRelevantKeys(Expression exp, Consumer acceptKey) { - if (exp instanceof Expression.And and) { - and.children().forEach(child -> getRelevantKeys(child, acceptKey)); - } else if (exp instanceof Expression.Or or) { - or.children().forEach(child -> getRelevantKeys(child, acceptKey)); - } else if (exp instanceof Expression.Not not) { - if (not.child()instanceof Expression.MatchAny any && any.matchWhenMissing()) { + if (!mustAlwaysEvaluate(exp)) { + if (exp instanceof Expression.And and) { + and.children().forEach(child -> getRelevantKeys(child, acceptKey)); + } else if (exp instanceof Expression.Or or) { + or.children().forEach(child -> getRelevantKeys(child, acceptKey)); + } else if (exp instanceof Expression.Not not) { + if (not.child()instanceof Expression.MatchAny any && any.matchWhenMissing()) { + acceptKey.accept(any.field()); + } + } else if (exp instanceof Expression.MatchField field) { + acceptKey.accept(field.field()); + } else if (exp instanceof Expression.MatchAny any && !any.matchWhenMissing()) { acceptKey.accept(any.field()); } - } else if (exp instanceof Expression.MatchField field) { - acceptKey.accept(field.field()); - } else if (exp instanceof Expression.MatchAny any && !any.matchWhenMissing()) { - acceptKey.accept(any.field()); } } @@ -244,20 +260,6 @@ private KeyIndex(MultiExpression expressions) { numExpressions = id; } - private boolean mustAlwaysEvaluate(Expression expression) { - if (expression instanceof Expression.Or or) { - return or.children().stream().anyMatch(this::mustAlwaysEvaluate); - } else if (expression instanceof Expression.And and) { - return and.children().stream().allMatch(this::mustAlwaysEvaluate); - } else if (expression instanceof Expression.Not not) { - return !mustAlwaysEvaluate(not.child()); - } else if (expression instanceof Expression.MatchAny any && any.matchWhenMissing()) { - return true; - } else { - return TRUE.equals(expression); - } - } - /** Lookup matches in this index for expressions that match a certain type. */ @Override public List> getMatchesWithTriggers(SourceFeature input) { From 1ee37bb5a38a109dce206b519de02df75dee61ae Mon Sep 17 00:00:00 2001 From: Brian Sperlongano Date: Sun, 1 May 2022 19:34:23 -0400 Subject: [PATCH 07/19] Add test case for "missing" --- .../expression/MultiExpressionTest.java | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/planetiler-core/src/test/java/com/onthegomap/planetiler/expression/MultiExpressionTest.java b/planetiler-core/src/test/java/com/onthegomap/planetiler/expression/MultiExpressionTest.java index 14ec257234..7c8edd5ecf 100644 --- a/planetiler-core/src/test/java/com/onthegomap/planetiler/expression/MultiExpressionTest.java +++ b/planetiler-core/src/test/java/com/onthegomap/planetiler/expression/MultiExpressionTest.java @@ -527,6 +527,31 @@ void testMatchDifferentTypes() { assertEquals("polygon", index.getOrElse(polygon, null)); } + @Test + void testMatchMissing() { + //Test logic: match if either key1 or key2 is missing + var index1 = MultiExpression.of(List.of( + entry("a", or( + matchAny("key1", ""), + matchAny("key2", "") + )) + )).index(); + + var index2 = MultiExpression.of(List.of( + entry("a", not(and( + matchField("key1"), + matchField("key2") + ))) + )).index(); + + List.of(index1, index2).forEach(index -> { + assertSameElements(List.of("a"), index.getMatches(featureWithTags())); + assertSameElements(List.of("a"), index.getMatches(featureWithTags("key1", "value1"))); + assertSameElements(List.of("a"), index.getMatches(featureWithTags("key2", "value2"))); + assertSameElements(List.of(), index.getMatches(featureWithTags("key1", "value1", "key2", "value2"))); + }); + } + private static void assertSameElements(List a, List b) { assertEquals( a.stream().sorted(Comparator.comparing(Object::toString)).toList(), From e21485c69db7b8d12c68c7c7cf0971d31ee49a0a Mon Sep 17 00:00:00 2001 From: Mike Barry Date: Sun, 1 May 2022 20:28:23 -0400 Subject: [PATCH 08/19] comment --- .../com/onthegomap/planetiler/expression/MultiExpression.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/MultiExpression.java b/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/MultiExpression.java index c8cdbccd48..871fcbb0a4 100644 --- a/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/MultiExpression.java +++ b/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/MultiExpression.java @@ -65,6 +65,10 @@ private static void visitExpressions(SourceFeature input, List> res } } + /** + * Returns true if {@code expression} only contains "not filter" so we can't limit evaluating this expression to only + * when a particular key is present on the input. + */ private static boolean mustAlwaysEvaluate(Expression expression) { if (expression instanceof Expression.Or or) { return or.children().stream().anyMatch(MultiExpression::mustAlwaysEvaluate); From 1ab50480582afe4a48ba4034706be95acc063a79 Mon Sep 17 00:00:00 2001 From: Brian Sperlongano Date: Sun, 1 May 2022 20:38:05 -0400 Subject: [PATCH 09/19] Add match not missing test --- .../expression/MultiExpressionTest.java | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/planetiler-core/src/test/java/com/onthegomap/planetiler/expression/MultiExpressionTest.java b/planetiler-core/src/test/java/com/onthegomap/planetiler/expression/MultiExpressionTest.java index 7c8edd5ecf..20114930f1 100644 --- a/planetiler-core/src/test/java/com/onthegomap/planetiler/expression/MultiExpressionTest.java +++ b/planetiler-core/src/test/java/com/onthegomap/planetiler/expression/MultiExpressionTest.java @@ -552,6 +552,27 @@ void testMatchMissing() { }); } + @Test + void testMatchNotMissing() { + //Test logic: match if key1 is present (not missing) + var index1 = MultiExpression.of(List.of( + entry("a", matchField("key1")) + )).index(); + + var index2 = MultiExpression.of(List.of( + entry("a", not( + matchAny("key1", "") + )) + )).index(); + + List.of(index1, index2).forEach(index -> { + assertSameElements(List.of(), index.getMatches(featureWithTags())); + assertSameElements(List.of("a"), index.getMatches(featureWithTags("key1", "value1"))); + assertSameElements(List.of(), index.getMatches(featureWithTags("key2", "value2"))); + assertSameElements(List.of("a"), index.getMatches(featureWithTags("key1", "value1", "key2", "value2"))); + }); + } + private static void assertSameElements(List a, List b) { assertEquals( a.stream().sorted(Comparator.comparing(Object::toString)).toList(), From 076a0b227cc1392b127b50711719bd8d48d235d4 Mon Sep 17 00:00:00 2001 From: Mike Barry Date: Sun, 1 May 2022 21:09:42 -0400 Subject: [PATCH 10/19] simplify matchField --- .../java/com/onthegomap/planetiler/expression/Expression.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/Expression.java b/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/Expression.java index b75bfd7ef5..26cb5d0d58 100644 --- a/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/Expression.java +++ b/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/Expression.java @@ -151,6 +151,8 @@ private static Expression simplifyOnce(Expression expression) { return FALSE; } else if (not.child == FALSE) { return TRUE; + } else if (not.child instanceof MatchAny any && any.values.equals(List.of(""))) { + return matchField(any.field); } return not; } else if (expression instanceof Or or) { From 6712cf9a3c5116eb99f6a921371219a9fa3774e0 Mon Sep 17 00:00:00 2001 From: Mike Barry Date: Mon, 2 May 2022 06:36:35 -0400 Subject: [PATCH 11/19] fix simplify issue --- .../planetiler/expression/Expression.java | 18 +++++++++++++++--- .../planetiler/expression/ExpressionTest.java | 18 ++++++++++++++++++ 2 files changed, 33 insertions(+), 3 deletions(-) diff --git a/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/Expression.java b/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/Expression.java index 26cb5d0d58..1ee6a749b7 100644 --- a/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/Expression.java +++ b/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/Expression.java @@ -13,6 +13,8 @@ import java.util.regex.Pattern; import java.util.stream.Collectors; import java.util.stream.Stream; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** * A framework for defining and manipulating boolean expressions that match on input element. @@ -27,6 +29,7 @@ * */ public interface Expression { + Logger LOGGER = LoggerFactory.getLogger(Expression.class); String LINESTRING_TYPE = "linestring"; String POINT_TYPE = "point"; @@ -132,7 +135,11 @@ private static Expression simplify(Expression initial) { seen.add(simplified); while (true) { simplified = simplifyOnce(simplified); - if (seen.contains(simplified) || seen.size() > 100) { + if (seen.contains(simplified)) { + return simplified; + } + if (seen.size() > 1000) { + LOGGER.warn("Infinite loop while simplifying expression {}", initial); return simplified; } seen.add(simplified); @@ -168,7 +175,7 @@ private static Expression simplifyOnce(Expression expression) { return or(or.children.stream() // hoist children .flatMap(child -> child instanceof Or childOr ? childOr.children.stream() : Stream.of(child)) - .filter(child -> child != FALSE) + .filter(child -> child != FALSE) // or() == or(FALSE) == or(FALSE, FALSE) == FALSE, so safe to remove all here .map(Expression::simplifyOnce).toList()); } else if (expression instanceof And and) { if (and.children.isEmpty()) { @@ -180,10 +187,15 @@ private static Expression simplifyOnce(Expression expression) { if (and.children.contains(FALSE)) { return FALSE; } + if (and.children.contains(TRUE)) { + // since and() == FALSE but and(TRUE) == and(TRUE, TRUE) == TRUE, don't remove the last TRUE + var filteredChildren = and.children.stream().filter(child -> child != TRUE).toList(); + return filteredChildren.isEmpty() ? TRUE : and(filteredChildren); + } return and(and.children.stream() // hoist children .flatMap(child -> child instanceof And childAnd ? childAnd.children.stream() : Stream.of(child)) - .filter(child -> child != TRUE) + // and() == FALSE but and(TRUE) == TRUE so can't remove TRUEs here .map(Expression::simplifyOnce).toList()); } else { return expression; diff --git a/planetiler-core/src/test/java/com/onthegomap/planetiler/expression/ExpressionTest.java b/planetiler-core/src/test/java/com/onthegomap/planetiler/expression/ExpressionTest.java index a31fb3a94d..28b3b58a75 100644 --- a/planetiler-core/src/test/java/com/onthegomap/planetiler/expression/ExpressionTest.java +++ b/planetiler-core/src/test/java/com/onthegomap/planetiler/expression/ExpressionTest.java @@ -80,6 +80,24 @@ void testSimplifyTrue() { assertEquals(TRUE, or(TRUE, matchAB).simplify()); } + @Test + void testSimplifyAndCases() { + assertEquals(TRUE, and(TRUE).simplify()); + assertEquals(TRUE, and(TRUE, TRUE).simplify()); + assertEquals(TRUE, and(and(TRUE)).simplify()); + assertEquals(TRUE, and(and(TRUE), TRUE).simplify()); + assertEquals(TRUE, and(TRUE, and(TRUE), TRUE).simplify()); + assertEquals(matchAB, and(TRUE, and(TRUE), matchAB).simplify()); + } + + @Test + void testSimplifyOrCases() { + assertEquals(FALSE, or(or(FALSE)).simplify()); + assertEquals(FALSE, or(or(FALSE), FALSE).simplify()); + assertEquals(FALSE, or(FALSE, or(FALSE), FALSE).simplify()); + assertEquals(matchAB, or(FALSE, or(FALSE), matchAB).simplify()); + } + @Test void testReplace() { assertEquals( From 88d508db4ac72f1af7d136dd3947300135f3feb0 Mon Sep 17 00:00:00 2001 From: Mike Barry Date: Mon, 2 May 2022 07:04:30 -0400 Subject: [PATCH 12/19] simplify and() to true --- .../planetiler/basemap/BasemapProfile.java | 2 +- .../planetiler/expression/Expression.java | 9 +--- .../expression/MultiExpression.java | 41 +++++++++++++------ .../planetiler/expression/ExpressionTest.java | 23 +++++++++++ .../expression/MultiExpressionTest.java | 10 +---- 5 files changed, 55 insertions(+), 30 deletions(-) diff --git a/planetiler-basemap/src/main/java/com/onthegomap/planetiler/basemap/BasemapProfile.java b/planetiler-basemap/src/main/java/com/onthegomap/planetiler/basemap/BasemapProfile.java index 7d4c4a4975..2e976a8799 100644 --- a/planetiler-basemap/src/main/java/com/onthegomap/planetiler/basemap/BasemapProfile.java +++ b/planetiler-basemap/src/main/java/com/onthegomap/planetiler/basemap/BasemapProfile.java @@ -116,7 +116,7 @@ public BasemapProfile(Translations translations, PlanetilerConfig config, Stats }) .toList(); return new RowDispatch(constructor.create(), handlers); - }).simplify().index(); + }).simplify().indexAndWarn(); wikidataMappings = Tables.MAPPINGS .mapResults(constructor -> handlerMap.getOrDefault(constructor.rowClass(), List.of()).stream() .anyMatch(handler -> !IgnoreWikidata.class.isAssignableFrom(handler.handlerClass())) diff --git a/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/Expression.java b/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/Expression.java index 1ee6a749b7..107d3beb4e 100644 --- a/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/Expression.java +++ b/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/Expression.java @@ -179,7 +179,7 @@ private static Expression simplifyOnce(Expression expression) { .map(Expression::simplifyOnce).toList()); } else if (expression instanceof And and) { if (and.children.isEmpty()) { - return FALSE; + return TRUE; } if (and.children.size() == 1) { return simplifyOnce(and.children.get(0)); @@ -187,15 +187,10 @@ private static Expression simplifyOnce(Expression expression) { if (and.children.contains(FALSE)) { return FALSE; } - if (and.children.contains(TRUE)) { - // since and() == FALSE but and(TRUE) == and(TRUE, TRUE) == TRUE, don't remove the last TRUE - var filteredChildren = and.children.stream().filter(child -> child != TRUE).toList(); - return filteredChildren.isEmpty() ? TRUE : and(filteredChildren); - } return and(and.children.stream() // hoist children .flatMap(child -> child instanceof And childAnd ? childAnd.children.stream() : Stream.of(child)) - // and() == FALSE but and(TRUE) == TRUE so can't remove TRUEs here + .filter(child -> child != TRUE) // and() == and(TRUE) == and(TRUE, TRUE) == TRUE, so safe to remove all here .map(Expression::simplifyOnce).toList()); } else { return expression; diff --git a/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/MultiExpression.java b/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/MultiExpression.java index 871fcbb0a4..5940544020 100644 --- a/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/MultiExpression.java +++ b/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/MultiExpression.java @@ -104,12 +104,24 @@ private static void getRelevantKeys(Expression exp, Consumer acceptKey) /** Returns an optimized index for matching {@link #expressions()} against each input element. */ public Index index() { + return index(false); + } + + /** + * Same as {@link #index()} but logs a warning when there are degenerate expressions that must be evaluated on every + * input. + */ + public Index indexAndWarn() { + return index(true); + } + + private Index index(boolean warn) { if (expressions.isEmpty()) { return new EmptyIndex<>(); } boolean caresAboutGeometryType = expressions.stream().anyMatch(entry -> entry.expression.contains(exp -> exp instanceof Expression.MatchType)); - return caresAboutGeometryType ? new GeometryTypeIndex<>(this) : new KeyIndex<>(simplify()); + return caresAboutGeometryType ? new GeometryTypeIndex<>(this, warn) : new KeyIndex<>(simplify(), warn); } /** Returns a copy of this multi-expression that replaces every expression using {@code mapper}. */ @@ -232,7 +244,7 @@ private static class KeyIndex implements Index { // expressions that must always be evaluated on each input element private final List> alwaysEvaluateExpressionList; - private KeyIndex(MultiExpression expressions) { + private KeyIndex(MultiExpression expressions, boolean warn) { int id = 1; // build the indexes Map>> keyToExpressions = new HashMap<>(); @@ -249,10 +261,10 @@ private KeyIndex(MultiExpression expressions) { } } // create immutable copies for fast iteration at matching time - if (!always.isEmpty()) { + if (warn && !always.isEmpty()) { LOGGER.warn("{} expressions will be evaluated for every element:", always.size()); for (var expression : always) { - LOGGER.warn(" {}", expression.expression.generateJavaCode()); + LOGGER.warn(" {}: {}", expression.result, expression.expression); } } alwaysEvaluateExpressionList = List.copyOf(always); @@ -293,19 +305,22 @@ private static class GeometryTypeIndex implements Index { private final KeyIndex lineIndex; private final KeyIndex polygonIndex; - private GeometryTypeIndex(MultiExpression expressions) { + private GeometryTypeIndex(MultiExpression expressions, boolean warn) { // build an index per type then search in each of those indexes based on the geometry type of each input element // this narrows the search space substantially, improving matching performance - pointIndex = indexForType(expressions, Expression.POINT_TYPE); - lineIndex = indexForType(expressions, Expression.LINESTRING_TYPE); - polygonIndex = indexForType(expressions, Expression.POLYGON_TYPE); + pointIndex = indexForType(expressions, Expression.POINT_TYPE, warn); + lineIndex = indexForType(expressions, Expression.LINESTRING_TYPE, warn); + polygonIndex = indexForType(expressions, Expression.POLYGON_TYPE, warn); } - private KeyIndex indexForType(MultiExpression expressions, String type) { - return new KeyIndex<>(expressions - .replace(matchType(type), TRUE) - .replace(e -> e instanceof Expression.MatchType, FALSE) - .simplify()); + private KeyIndex indexForType(MultiExpression expressions, String type, boolean warn) { + return new KeyIndex<>( + expressions + .replace(matchType(type), TRUE) + .replace(e -> e instanceof Expression.MatchType, FALSE) + .simplify(), + warn + ); } /** diff --git a/planetiler-core/src/test/java/com/onthegomap/planetiler/expression/ExpressionTest.java b/planetiler-core/src/test/java/com/onthegomap/planetiler/expression/ExpressionTest.java index 28b3b58a75..b4c9dad899 100644 --- a/planetiler-core/src/test/java/com/onthegomap/planetiler/expression/ExpressionTest.java +++ b/planetiler-core/src/test/java/com/onthegomap/planetiler/expression/ExpressionTest.java @@ -1,12 +1,18 @@ package com.onthegomap.planetiler.expression; +import static com.onthegomap.planetiler.TestUtils.newPoint; import static com.onthegomap.planetiler.expression.Expression.*; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; +import com.onthegomap.planetiler.reader.SimpleFeature; +import com.onthegomap.planetiler.reader.SourceFeature; import com.onthegomap.planetiler.reader.WithTags; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.Map; import java.util.Set; import org.junit.jupiter.api.Test; @@ -16,6 +22,14 @@ class ExpressionTest { public static final Expression.MatchAny matchCD = matchAny("c", "d"); public static final Expression.MatchAny matchBC = matchAny("b", "c"); + static SourceFeature featureWithTags(String... tags) { + Map map = new HashMap<>(); + for (int i = 0; i < tags.length; i += 2) { + map.put(tags[i], tags[i + 1]); + } + return SimpleFeature.create(newPoint(0, 0), map); + } + @Test void testSimplify() { assertEquals(matchAB, matchAB.simplify()); @@ -84,6 +98,7 @@ void testSimplifyTrue() { void testSimplifyAndCases() { assertEquals(TRUE, and(TRUE).simplify()); assertEquals(TRUE, and(TRUE, TRUE).simplify()); + assertEquals(TRUE, and(TRUE, and()).simplify()); assertEquals(TRUE, and(and(TRUE)).simplify()); assertEquals(TRUE, and(and(TRUE), TRUE).simplify()); assertEquals(TRUE, and(TRUE, and(TRUE), TRUE).simplify()); @@ -98,6 +113,14 @@ void testSimplifyOrCases() { assertEquals(matchAB, or(FALSE, or(FALSE), matchAB).simplify()); } + @Test + void testEvaluateEmptyAnd() { + assertEquals( + and().evaluate(featureWithTags(), new ArrayList<>()), + and().simplify().evaluate(featureWithTags(), new ArrayList<>()) + ); + } + @Test void testReplace() { assertEquals( diff --git a/planetiler-core/src/test/java/com/onthegomap/planetiler/expression/MultiExpressionTest.java b/planetiler-core/src/test/java/com/onthegomap/planetiler/expression/MultiExpressionTest.java index 20114930f1..5b42042fea 100644 --- a/planetiler-core/src/test/java/com/onthegomap/planetiler/expression/MultiExpressionTest.java +++ b/planetiler-core/src/test/java/com/onthegomap/planetiler/expression/MultiExpressionTest.java @@ -4,6 +4,7 @@ import static com.onthegomap.planetiler.TestUtils.newPoint; import static com.onthegomap.planetiler.TestUtils.rectangle; import static com.onthegomap.planetiler.expression.Expression.*; +import static com.onthegomap.planetiler.expression.ExpressionTest.featureWithTags; import static com.onthegomap.planetiler.expression.MultiExpression.entry; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; @@ -15,21 +16,12 @@ import com.onthegomap.planetiler.reader.WithTags; import java.util.ArrayList; import java.util.Comparator; -import java.util.HashMap; import java.util.List; import java.util.Map; import org.junit.jupiter.api.Test; class MultiExpressionTest { - private static SourceFeature featureWithTags(String... tags) { - Map map = new HashMap<>(); - for (int i = 0; i < tags.length; i += 2) { - map.put(tags[i], tags[i + 1]); - } - return SimpleFeature.create(newPoint(0, 0), map); - } - @Test void testEmpty() { var index = MultiExpression.of(List.of()).index(); From 0a87abc1861a4b31bf17d64d1d23d4a0c5a828a1 Mon Sep 17 00:00:00 2001 From: Brian Sperlongano Date: Mon, 2 May 2022 19:45:43 -0400 Subject: [PATCH 13/19] Add new test case --- .../expression/MultiExpressionTest.java | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/planetiler-core/src/test/java/com/onthegomap/planetiler/expression/MultiExpressionTest.java b/planetiler-core/src/test/java/com/onthegomap/planetiler/expression/MultiExpressionTest.java index 20114930f1..9f4a28fa6b 100644 --- a/planetiler-core/src/test/java/com/onthegomap/planetiler/expression/MultiExpressionTest.java +++ b/planetiler-core/src/test/java/com/onthegomap/planetiler/expression/MultiExpressionTest.java @@ -573,6 +573,40 @@ void testMatchNotMissing() { }); } + void testAndOrMatch() { + var expr = and( + or( + matchAny("key", "val") + ), + TRUE + ); + var index = MultiExpression.of(List.of(entry("a", expr))).index(); + + //Ensure MultiExpression and regular Expression work the same + assertSameElements(List.of("a"), index.getMatches(featureWithTags("key", "val"))); + assertSameElements(List.of("a"), index.getMatches(featureWithTags("key", "val", "otherkey", "otherval"))); + assertSameElements(List.of(), index.getMatches(featureWithTags("otherkey", "otherval"))); + assertSameElements(List.of(), index.getMatches(featureWithTags("key", "otherval"))); + assertSameElements(List.of(), index.getMatches(featureWithTags())); + + var list = new ArrayList(); + + assertTrue(expr.evaluate(featureWithTags("key", "val"), list)); + assertTrue(expr.evaluate(featureWithTags("key", "val", "otherkey", "otherval"), list)); + assertFalse(expr.evaluate(featureWithTags("otherkey", "otherval"), list)); + assertFalse(expr.evaluate(featureWithTags("key", "otherval"), list)); + assertFalse(expr.evaluate(featureWithTags(), list)); + + expr.simplify(); + + //Ensure Expression works the same after a simplify + assertTrue(expr.evaluate(featureWithTags("key", "val"), list)); + assertTrue(expr.evaluate(featureWithTags("key", "val", "otherkey", "otherval"), list)); + assertFalse(expr.evaluate(featureWithTags("otherkey", "otherval"), list)); + assertFalse(expr.evaluate(featureWithTags("key", "otherval"), list)); + assertFalse(expr.evaluate(featureWithTags(), list)); + } + private static void assertSameElements(List a, List b) { assertEquals( a.stream().sorted(Comparator.comparing(Object::toString)).toList(), From ce41a247d9fe99e000bed88b619af92ed8c94fb5 Mon Sep 17 00:00:00 2001 From: Mike Barry Date: Tue, 3 May 2022 20:48:33 -0400 Subject: [PATCH 14/19] tweaks --- .../planetiler/benchmarks/BasemapMapping.java | 2 +- .../onthegomap/planetiler/expression/Expression.java | 10 ++-------- .../planetiler/expression/MultiExpression.java | 3 +++ 3 files changed, 6 insertions(+), 9 deletions(-) diff --git a/planetiler-benchmarks/src/main/java/com/onthegomap/planetiler/benchmarks/BasemapMapping.java b/planetiler-benchmarks/src/main/java/com/onthegomap/planetiler/benchmarks/BasemapMapping.java index 02e3b8cf92..b41ea367c5 100644 --- a/planetiler-benchmarks/src/main/java/com/onthegomap/planetiler/benchmarks/BasemapMapping.java +++ b/planetiler-benchmarks/src/main/java/com/onthegomap/planetiler/benchmarks/BasemapMapping.java @@ -33,7 +33,7 @@ public static void main(String[] args) { try (var reader = OsmInputFile.readFrom(Path.of("data", "sources", "massachusetts.osm.pbf"))) { reader.forEachBlock(block -> { for (var element : block.decodeElements()) { - if (random.nextDouble() < 0.2) { + if (random.nextDouble() < 1) { if (inputs.size() % 1_000_000 == 0) { logger.log(); } diff --git a/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/Expression.java b/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/Expression.java index 107d3beb4e..082afb670f 100644 --- a/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/Expression.java +++ b/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/Expression.java @@ -139,8 +139,7 @@ private static Expression simplify(Expression initial) { return simplified; } if (seen.size() > 1000) { - LOGGER.warn("Infinite loop while simplifying expression {}", initial); - return simplified; + throw new IllegalStateException("Infinite loop while simplifying expression " + initial); } seen.add(simplified); } @@ -292,12 +291,7 @@ public String generateJavaCode() { @Override public boolean evaluate(WithTags input, List matchKeys) { - int size = children.size(); - // Optimization: this method consumes the most time when matching against input elements, and - // iterating through this list by index is slightly faster than an enhanced for loop - // noinspection ForLoopReplaceableByForEach - for intellij - for (int i = 0; i < size; i++) { - Expression child = children.get(i); + for (Expression child : children) { if (child.evaluate(input, matchKeys)) { return true; } diff --git a/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/MultiExpression.java b/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/MultiExpression.java index 5940544020..c2044c93c4 100644 --- a/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/MultiExpression.java +++ b/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/MultiExpression.java @@ -85,6 +85,9 @@ private static boolean mustAlwaysEvaluate(Expression expression) { /** Calls {@code acceptKey} for every tag that could possibly cause {@code exp} to match an input element. */ private static void getRelevantKeys(Expression exp, Consumer acceptKey) { + // if a sub-expression must always be evaluated, then either the whole expression must always be evaluated + // or there is another part of the expression that limits the elements on which it must be evaluated, so we can + // ignore keys from this sub-expression. if (!mustAlwaysEvaluate(exp)) { if (exp instanceof Expression.And and) { and.children().forEach(child -> getRelevantKeys(child, acceptKey)); From efdd8ecdab03ac3aeb7f49483579ad85cbb557d4 Mon Sep 17 00:00:00 2001 From: Mike Barry Date: Tue, 3 May 2022 20:48:40 -0400 Subject: [PATCH 15/19] 0.9 --- .../com/onthegomap/planetiler/benchmarks/BasemapMapping.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/planetiler-benchmarks/src/main/java/com/onthegomap/planetiler/benchmarks/BasemapMapping.java b/planetiler-benchmarks/src/main/java/com/onthegomap/planetiler/benchmarks/BasemapMapping.java index b41ea367c5..e969dbf276 100644 --- a/planetiler-benchmarks/src/main/java/com/onthegomap/planetiler/benchmarks/BasemapMapping.java +++ b/planetiler-benchmarks/src/main/java/com/onthegomap/planetiler/benchmarks/BasemapMapping.java @@ -33,7 +33,7 @@ public static void main(String[] args) { try (var reader = OsmInputFile.readFrom(Path.of("data", "sources", "massachusetts.osm.pbf"))) { reader.forEachBlock(block -> { for (var element : block.decodeElements()) { - if (random.nextDouble() < 1) { + if (random.nextDouble() < 0.9) { if (inputs.size() % 1_000_000 == 0) { logger.log(); } From f25bd567a3b0904d63853170e87ecc15569152a1 Mon Sep 17 00:00:00 2001 From: Mike Barry Date: Tue, 3 May 2022 21:21:21 -0400 Subject: [PATCH 16/19] remove unreachable code --- .../onthegomap/planetiler/expression/MultiExpression.java | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/MultiExpression.java b/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/MultiExpression.java index c2044c93c4..15dbdc91e2 100644 --- a/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/MultiExpression.java +++ b/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/MultiExpression.java @@ -93,10 +93,9 @@ private static void getRelevantKeys(Expression exp, Consumer acceptKey) and.children().forEach(child -> getRelevantKeys(child, acceptKey)); } else if (exp instanceof Expression.Or or) { or.children().forEach(child -> getRelevantKeys(child, acceptKey)); - } else if (exp instanceof Expression.Not not) { - if (not.child()instanceof Expression.MatchAny any && any.matchWhenMissing()) { - acceptKey.accept(any.field()); - } + } else if (exp instanceof Expression.Not) { + // not(matchAny("field", "")) should track "field" as a relevant key, but that gets simplified to matchField("field") + // so don't need to handle that here } else if (exp instanceof Expression.MatchField field) { acceptKey.accept(field.field()); } else if (exp instanceof Expression.MatchAny any && !any.matchWhenMissing()) { From 04fb8dcdcc7061b4534330dbbe68af4a5984b474 Mon Sep 17 00:00:00 2001 From: Brian Sperlongano Date: Wed, 4 May 2022 05:15:28 -0400 Subject: [PATCH 17/19] Add test cases --- .../onthegomap/planetiler/expression/ExpressionTest.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/planetiler-core/src/test/java/com/onthegomap/planetiler/expression/ExpressionTest.java b/planetiler-core/src/test/java/com/onthegomap/planetiler/expression/ExpressionTest.java index b4c9dad899..01366c7f31 100644 --- a/planetiler-core/src/test/java/com/onthegomap/planetiler/expression/ExpressionTest.java +++ b/planetiler-core/src/test/java/com/onthegomap/planetiler/expression/ExpressionTest.java @@ -113,6 +113,12 @@ void testSimplifyOrCases() { assertEquals(matchAB, or(FALSE, or(FALSE), matchAB).simplify()); } + @Test + void testSimplifyNotCases() { + assertEquals(FALSE, not(TRUE).simplify()); + assertEquals(TRUE, not(FALSE).simplify()); + } + @Test void testEvaluateEmptyAnd() { assertEquals( From 3cc462a6119c23d9f6c75876ea9cb97e8ed67f91 Mon Sep 17 00:00:00 2001 From: Mike Barry Date: Wed, 4 May 2022 08:37:40 -0400 Subject: [PATCH 18/19] remove empty block --- .../onthegomap/planetiler/expression/MultiExpression.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/MultiExpression.java b/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/MultiExpression.java index 15dbdc91e2..e39715dae1 100644 --- a/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/MultiExpression.java +++ b/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/MultiExpression.java @@ -93,14 +93,13 @@ private static void getRelevantKeys(Expression exp, Consumer acceptKey) and.children().forEach(child -> getRelevantKeys(child, acceptKey)); } else if (exp instanceof Expression.Or or) { or.children().forEach(child -> getRelevantKeys(child, acceptKey)); - } else if (exp instanceof Expression.Not) { - // not(matchAny("field", "")) should track "field" as a relevant key, but that gets simplified to matchField("field") - // so don't need to handle that here } else if (exp instanceof Expression.MatchField field) { acceptKey.accept(field.field()); } else if (exp instanceof Expression.MatchAny any && !any.matchWhenMissing()) { acceptKey.accept(any.field()); } + // ignore not case since not(matchAny("field", "")) should track "field" as a relevant key, but that gets + // simplified to matchField("field") so don't need to handle that here } } From 12a360b087f45aa2ba872333a48aeea17be2f18b Mon Sep 17 00:00:00 2001 From: Mike Barry Date: Wed, 4 May 2022 20:14:49 -0400 Subject: [PATCH 19/19] test some more cases --- .../expression/MultiExpressionTest.java | 22 +++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/planetiler-core/src/test/java/com/onthegomap/planetiler/expression/MultiExpressionTest.java b/planetiler-core/src/test/java/com/onthegomap/planetiler/expression/MultiExpressionTest.java index 220e8f28bb..54c14a22d8 100644 --- a/planetiler-core/src/test/java/com/onthegomap/planetiler/expression/MultiExpressionTest.java +++ b/planetiler-core/src/test/java/com/onthegomap/planetiler/expression/MultiExpressionTest.java @@ -17,6 +17,7 @@ import java.util.ArrayList; import java.util.Comparator; import java.util.List; +import java.util.Locale; import java.util.Map; import org.junit.jupiter.api.Test; @@ -302,6 +303,23 @@ void testOr() { assertSameElements(List.of(), index.getMatches(featureWithTags())); } + @Test + void testMapResult() { + var index = MultiExpression.of(List.of( + entry("a", matchField("key")) + )).mapResults(result -> result.toUpperCase(Locale.ROOT)).index(); + assertSameElements(List.of("A"), index.getMatches(featureWithTags("key", "value"))); + } + + @Test + void testFilterResults() { + var index = MultiExpression.of(List.of( + entry("a", matchField("key")), + entry("b", matchField("key")) + )).filterResults(result -> !result.equals("b")).index(); + assertSameElements(List.of("a"), index.getMatches(featureWithTags("key", "value"))); + } + @Test void testOrConstant() { var index = MultiExpression.of(List.of( @@ -309,7 +327,7 @@ void testOrConstant() { Expression.TRUE, matchAny("key1", "val1") )) - )).index(); + )).indexAndWarn(); assertSameElements(List.of("a"), index.getMatches(featureWithTags("key1", "val1", "key2", "val2"))); assertSameElements(List.of("a"), index.getMatches(featureWithTags("key1", "no", "key2", "val2"))); assertSameElements(List.of("a"), index.getMatches(featureWithTags("key1", "val1", "key2", "no"))); @@ -521,7 +539,7 @@ void testMatchDifferentTypes() { @Test void testMatchMissing() { - //Test logic: match if either key1 or key2 is missing + // Test logic: match if either key1 or key2 is missing var index1 = MultiExpression.of(List.of( entry("a", or( matchAny("key1", ""),