Fix null behavior in in/ni expressions (#214)

* Fix null behavior in in/ni expressions

* Add test for null attributes in "ni" filters

* Remove unidecode from "in" expression evaluation again

CHANGELOG.md

@@ -1,3 +1,8 @@
+# 2.51.0
+
+* Fix null behavior in "in" expressions
+* But they are back to not using unidecode
+
 # 2.50.0
 
 * FSL-style "in" expressions use unidecode again

Makefile

@@ -335,6 +335,16 @@ overzoom-test: tippecanoe-overzoom
 	./tippecanoe-decode tests/muni/out/out.dir/000.pbf 0 0 0 > tests/muni/out/out.dir/overzoomed.json
 	cmp tests/muni/out/out.dir/overzoomed.json tests/muni/out/out.dir/direct.json
 	rm -rf tests/muni/out/out.dir tests/muni/out/out.mbtiles tests/muni/out/out.dir/overzoomed.json tests/muni/out/out.dir/direct.json
+	# Test filter with null attribute
+	./tippecanoe-overzoom -j '{"*":["name","ni",[1,5,6,9]]}' -o tests/pbf/12-2145-1391-filter1.pbf tests/pbf/12-2145-1391.pbf 12/2145/1391 12/2145/1391
+	./tippecanoe-decode tests/pbf/12-2145-1391-filter1.pbf 12 2145 1391 > tests/pbf/12-2145-1391-filter1.pbf.json.check
+	cmp tests/pbf/12-2145-1391-filter1.pbf.json.check tests/pbf/12-2145-1391-filter1.pbf.json
+	rm tests/pbf/12-2145-1391-filter1.pbf.json.check tests/pbf/12-2145-1391-filter1.pbf
+	# Test filter with null attribute in "ni" list
+	./tippecanoe-overzoom -j '{"*":["name","ni",[1,5,6,9,null]]}' -o tests/pbf/12-2145-1391-filter2.pbf tests/pbf/12-2145-1391.pbf 12/2145/1391 12/2145/1391
+	./tippecanoe-decode tests/pbf/12-2145-1391-filter2.pbf 12 2145 1391 > tests/pbf/12-2145-1391-filter2.pbf.json.check
+	cmp tests/pbf/12-2145-1391-filter2.pbf.json.check tests/pbf/12-2145-1391-filter2.pbf.json
+	rm tests/pbf/12-2145-1391-filter2.pbf.json.check tests/pbf/12-2145-1391-filter2.pbf
 
 join-test: tippecanoe tippecanoe-decode tile-join
 	./tippecanoe -q -f -z12 -o tests/join-population/tabblock_06001420.mbtiles -YALAND10:'Land area' -L'{"file": "tests/join-population/tabblock_06001420.json", "description": "population"}'

evaluator.cpp

@@ -343,11 +343,9 @@ static int eval(std::function<mvt_value(std::string const &)> feature, json_obje
 	     strcmp(f->value.array.array[1]->value.string.string, "is") == 0 ||
 	     strcmp(f->value.array.array[1]->value.string.string, "isnt") == 0 ||
 	     false)) {
-		mvt_value lhs;
-		lhs.type = mvt_null;  // attributes that aren't found are nulls
-		mvt_value ff = feature(std::string(f->value.array.array[0]->value.string.string));
-		if (ff.type != mvt_no_such_key) {
-			lhs = ff;
+		mvt_value lhs = feature(std::string(f->value.array.array[0]->value.string.string));
+		if (lhs.type == mvt_no_such_key) {
+			lhs.type = mvt_null;
 		}
 
 		if (f->value.array.array[2]->type == JSON_NULL && strcmp(f->value.array.array[1]->value.string.string, "is") == 0) {
@@ -357,15 +355,15 @@ static int eval(std::function<mvt_value(std::string const &)> feature, json_obje
 			return lhs.type != mvt_null;  // null isnt null => false, anything isnt null => true
 		}
 
-		if (lhs.type == mvt_null) {
-			return -1;  // null op anything => null
-		}
-
 		bool fail = false;
 
 		if (f->value.array.array[2]->type == JSON_STRING &&
 		    (strcmp(f->value.array.array[1]->value.string.string, "cn") == 0 ||
 		     strcmp(f->value.array.array[1]->value.string.string, "nc") == 0)) {
+			if (lhs.type == mvt_null) {
+				return -1;  // null cn/nc anything => null
+			}
+
 			std::string s = mvt_value_to_string(lhs, fail, unidecode_data);
 			if (fail) {
 				return -1;  // null cn anything => false
@@ -387,22 +385,28 @@ static int eval(std::function<mvt_value(std::string const &)> feature, json_obje
 		if (f->value.array.array[2]->type == JSON_ARRAY &&
 		    (strcmp(f->value.array.array[1]->value.string.string, "in") == 0 ||
 		     strcmp(f->value.array.array[1]->value.string.string, "ni") == 0)) {
-			std::string s = mvt_value_to_string(lhs, fail, unidecode_data);
-			if (fail) {
-				return -1;  // null in anything => false
-			}
+			// no null check here, since null can be in or not in
 
 			bool contains = false;
 			for (size_t i = 0; i < f->value.array.array[2]->value.array.length; i++) {
-				fail = false;
-				int cmp = compare_fsl(ff, f->value.array.array[2]->value.array.array[i], fail, unidecode_data);
-				if (fail) {
-					continue;  // null
-				}
-
-				if (cmp == 0) {
-					contains = true;
-					break;
+				if (lhs.type == mvt_null) {
+					if (f->value.array.array[2]->value.array.array[i]->type == JSON_NULL) {
+						contains = true;  // null is null
+						break;
+					} else {
+						contains = false;  // null isnt non-null
+					}
+				} else {
+					fail = false;
+					static std::vector<std::string> no_unidecode_data;
+					int cmp = compare_fsl(lhs, f->value.array.array[2]->value.array.array[i], fail, no_unidecode_data);
+
+					if (fail) {
+						contains = false;  // non-null isnt null
+					} else if (cmp == 0) {
+						contains = true;
+						break;
+					}
 				}
 			}
 
@@ -413,7 +417,11 @@ static int eval(std::function<mvt_value(std::string const &)> feature, json_obje
 			}
 		}
 
-		int cmp = compare_fsl(ff, f->value.array.array[2], fail, unidecode_data);
+		if (lhs.type == mvt_null) {
+			return -1;  // null compared to anything => null
+		}
+
+		int cmp = compare_fsl(lhs, f->value.array.array[2], fail, unidecode_data);
 		if (fail) {
 			return -1;  // null
 		}

mvt.cpp

@@ -601,7 +601,7 @@ std::string mvt_value::toString() const {
 	case mvt_null:
 		return "null";
 	default:
-		return "unknown";
+		return "unknown " + std::to_string(type);
 	}
 }
 

tests/pbf/12-2145-1391-filter1.pbf.json

@@ -0,0 +1,15 @@
+{ "type": "FeatureCollection", "properties": { "zoom": 12, "x": 2145, "y": 1391 }, "features": [
+{ "type": "FeatureCollection", "properties": { "layer": "parsed", "version": 2, "extent": 4096 }, "features": [
+{ "type": "Feature", "id": 1, "properties": { "osm_id": "192162", "felt:cluster_size": 1 }, "geometry": { "type": "Point", "coordinates": [ 8.585773, 49.852166 ] } }
+,
+{ "type": "Feature", "id": 516, "properties": { "osm_id": "317282344", "felt:cluster_size": 1 }, "geometry": { "type": "Point", "coordinates": [ 8.591459, 49.856800 ] } }
+,
+{ "type": "Feature", "id": 545, "properties": { "osm_id": "330843617", "felt:cluster_size": 1 }, "geometry": { "type": "Point", "coordinates": [ 8.591051, 49.857063 ] } }
+,
+{ "type": "Feature", "id": 546, "properties": { "osm_id": "330843827", "felt:cluster_size": 1 }, "geometry": { "type": "Point", "coordinates": [ 8.592553, 49.851875 ] } }
+,
+{ "type": "Feature", "id": 547, "properties": { "osm_id": "330844309", "felt:cluster_size": 1 }, "geometry": { "type": "Point", "coordinates": [ 8.581738, 49.854047 ] } }
+,
+{ "type": "Feature", "id": 41908, "properties": { "name": "August-Euler-Flugplatz", "wikipedia": "de:August-Euler-Flugplatz", "ele": "102", "osm_way_id": "23872501", "felt:cluster_size": 1 }, "geometry": { "type": "Point", "coordinates": [ 8.586588, 49.853687 ] } }
+] }
+] }

tests/pbf/12-2145-1391-filter2.pbf.json

@@ -0,0 +1,5 @@
+{ "type": "FeatureCollection", "properties": { "zoom": 12, "x": 2145, "y": 1391 }, "features": [
+{ "type": "FeatureCollection", "properties": { "layer": "parsed", "version": 2, "extent": 4096 }, "features": [
+{ "type": "Feature", "id": 41908, "properties": { "name": "August-Euler-Flugplatz", "wikipedia": "de:August-Euler-Flugplatz", "ele": "102", "osm_way_id": "23872501", "felt:cluster_size": 1 }, "geometry": { "type": "Point", "coordinates": [ 8.586588, 49.853687 ] } }
+] }
+] }

version.hpp

@@ -1,6 +1,6 @@
 #ifndef VERSION_HPP
 #define VERSION_HPP
 
-#define VERSION "v2.50.0"
+#define VERSION "v2.51.0"
 
 #endif