Hi Gerd
looks good for me, although you may simplify the code in isIndexable():
a) The expression ((ValueOp) op.getFirst()).isIndexable() appears three times
b) the term (op.getSecond().isType(VALUE) || op.getSecond().isType(FUNCTION)
appears two times and I wonder why it is needed.
Do you have an example? The unit tests don't fail when I remove it.
OK a lot of that is not needed any more.
The attached patch returns isIndexable() to its simplest form and
all the other code is changed to fit.
Steve
Index: src/uk/me/parabola/mkgmap/osmstyle/ExpressionArranger.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- src/uk/me/parabola/mkgmap/osmstyle/ExpressionArranger.java (revision 4141)
+++ src/uk/me/parabola/mkgmap/osmstyle/ExpressionArranger.java (date 1522250497000)
@@ -399,13 +399,16 @@
return 501;
// Everything else is 200+, put regex behind as they are probably a bit slower.
+ case GT:
+ case GTE:
+ case LT:
+ case LTE:
+ return 200;
case REGEX:
- return 201;
+ return 210;
+
default:
- // Non-indexable functions must go to the back.
- if (!isIndexable(op))
- return 1000;
- return 200;
+ return 1000;
}
}
@@ -429,6 +432,8 @@
if (first.isType(EQUALS)) {
keystring = first.getFirst().getKeyValue() + "=" + first.getSecond().getKeyValue();
} else if (first.isType(EXISTS)) {
+ if (!isIndexable(first))
+ throw new SyntaxException(scanner, "Expression cannot be indexed");
keystring = first.getFirst().getKeyValue() + "=*";
} else if (first.isType(NOT_EXISTS)) {
throw new SyntaxException(scanner, "Cannot start rule with tag!=*");
@@ -485,13 +490,12 @@
*
* This is done if the first term is not by itself indexable, but could be made so by pre-pending an EXISTS clause.
*/
- private Op prepareWithExists(Op op) {
+ private static Op prepareWithExists(Op op) {
Op first = op;
if (first.isType(AND))
first = first.getFirst();
- if (NEED_EXISTS.contains(first.getType()) && isIndexable(first)
- || first.isType(EQUALS) && first.getSecond().isType(FUNCTION))
+ if (NEED_EXISTS.contains(first.getType()) || first.isType(EQUALS) && first.getSecond().isType(FUNCTION))
return combineWithExists((BinaryOp) op);
else
return op;
@@ -502,7 +506,7 @@
*
* This is done if the first term is not by itself indexable, but could be made so by pre-pending an EXISTS clause.
*/
- private AndOp combineWithExists(BinaryOp op) {
+ private static AndOp combineWithExists(BinaryOp op) {
Op first = op;
if (first.isType(AND))
first = first.getFirst();
@@ -515,26 +519,28 @@
/**
* True if this expression is 'solved'. This means that the first term is indexable or it is indexable itself.
+ *
+ * This is only used in the tests.
*/
public static boolean isSolved(Op op) {
switch (op.getType()) {
case NOT:
return false;
case AND:
- return isIndexable(op.getFirst());
+ return isAndIndexable(prepareWithExists(op.getFirst()));
case OR:
Op or = op;
boolean valid = true;
do {
- if (!isAndIndexable(or.getFirst()))
+ if (!isAndIndexable(prepareWithExists(or.getFirst())))
valid = false;
or = or.getSecond();
} while (or.isType(OR));
- if (!isAndIndexable(or))
+ if (!isAndIndexable(prepareWithExists(or)))
valid = false;
return valid;
default:
- return isIndexable(op);
+ return isIndexable(prepareWithExists(op));
}
}
@@ -553,11 +559,7 @@
* True if this operation can be indexed. It is a plain equality or Exists operation.
*/
private static boolean isIndexable(Op op) {
- return op.isType(EQUALS)
- && ((ValueOp) op.getFirst()).isIndexable() && op.getSecond().isType(VALUE)
- || NEED_EXISTS.contains(op.getType()) && ((ValueOp) op.getFirst()).isIndexable()
- && (op.getSecond().isType(VALUE) || op.getSecond().isType(FUNCTION))
- || op.isType(EXISTS) && ((ValueOp) op.getFirst()).isIndexable();
+ return (op.isType(EQUALS) || op.isType(EXISTS)) && ((ValueOp) op.getFirst()).isIndexable();
}
/**
Index: test/uk/me/parabola/mkgmap/osmstyle/ExpressionArrangerTest.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- test/uk/me/parabola/mkgmap/osmstyle/ExpressionArrangerTest.java (revision 4141)
+++ test/uk/me/parabola/mkgmap/osmstyle/ExpressionArrangerTest.java (date 1522249961000)
@@ -28,6 +28,7 @@
import static org.junit.Assert.*;
import static uk.me.parabola.mkgmap.osmstyle.ExpressionArranger.fmtExpr;
import static uk.me.parabola.mkgmap.osmstyle.ExpressionArranger.isSolved;
+import static uk.me.parabola.mkgmap.osmstyle.eval.NodeType.AND;
import static uk.me.parabola.mkgmap.osmstyle.eval.NodeType.EQUALS;
@@ -193,7 +194,18 @@
assertTrue(isSolved(op));
assertTrue(op.getFirst().getType() != NodeType.FUNCTION);
}
-
+
+ @Test
+ public void testEqualTagValue() {
+ Op op = createOp("c!=d & a=$b");
+ op = arranger.arrange(op);
+ Iterator<Op> it = arranger.prepareForSave(op);
+ op = it.next();
+ assertEquals(AND, op.getType());
+ assertEquals("$a=* & $a=$b & $c!=d", op.toString());
+ System.out.println(op.toString());
+ }
+
private Op createOp(String s) {
TokenScanner scanner = new TokenScanner("test.file", new StringReader(s));
ExpressionReader er = new ExpressionReader(scanner, FeatureKind.POLYLINE);
_______________________________________________
mkgmap-dev mailing list
mkgmap-dev@lists.mkgmap.org.uk
http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
