Avoid using 'coalesce' for filter comparison operators

Changes treatment of 'undefined' property values: we now treat them as equivalent to `null` -- i.e. `[==, 'foo', null]` now returns true for `{foo: undefined}`

bench/benchmarks/filter_evaluate.js

@@ -40,7 +40,7 @@ module.exports = class FilterEvaluate extends Benchmark {
         for (const layer of this.layers) {
             for (const filter of layer.filters) {
                 for (const feature of layer.features) {
-                    if (typeof filter(feature) !== 'boolean') {
+                    if (typeof filter({zoom: 0}, feature) !== 'boolean') {
                         assert(false, 'Expected boolean result from filter');
                     }
                 }

src/style-spec/feature_filter/index.js

@@ -2,8 +2,8 @@
 
 const compileExpression = require('../function/compile');
 const {BooleanType} = require('../function/types');
+const {typeOf} = require('../function/values');
 
-import type {Feature} from '../function';
 export type FeatureFilter = (globalProperties: {+zoom?: number}, feature: VectorTileFeature) => boolean;
 
 module.exports = createFilter;
@@ -19,23 +19,19 @@ module.exports = createFilter;
  */
 function createFilter(filter: any): FeatureFilter {
     if (!filter) {
-        return (globalProperties, _: VectorTileFeature) => true;
+        return () => true;
     }
 
-    let expression = Array.isArray(filter) ? convertFilter(filter) : filter.expression;
-    if (Array.isArray(expression) && expression[0] !== 'coalesce') {
-        expression = ['coalesce', expression, false];
-    }
+    const expression = Array.isArray(filter) ? convertFilter(filter) : filter.expression;
     const compiled = compileExpression(expression, BooleanType);
 
     if (compiled.result === 'success') {
-        return (globalProperties, feature: VectorTileFeature) => {
-            const expressionFeature: Feature = {
-                properties: feature.properties || {},
-                type: feature.type,
-                id: typeof feature.id !== 'undefined' ? feature.id : null
-            };
-            return compiled.function(globalProperties, expressionFeature);
+        return (g, f) => {
+            try {
+                return compiled.function(g, f);
+            } catch (e) {
+                return false;
+            }
         };
     } else {
         throw new Error(compiled.errors.map(err => `${err.key}: ${err.message}`).join(', '));
@@ -53,9 +49,9 @@ function convertFilter(filter: ?Array<any>): mixed {
         op === '>' ||
         op === '<=' ||
         op === '>=' ? compileComparisonOp(filter[1], filter[2], op) :
-        op === 'any' ? compileLogicalOp(filter.slice(1), '||') :
-        op === 'all' ? compileLogicalOp(filter.slice(1), '&&') :
-        op === 'none' ? compileNegation(compileLogicalOp(filter.slice(1), '||')) :
+        op === 'any' ? compileDisjunctionOp(filter.slice(1)) :
+        op === 'all' ? ['&&'].concat(filter.slice(1).map(convertFilter)) :
+        op === 'none' ? ['&&'].concat(filter.slice(1).map(convertFilter).map(compileNegation)) :
         op === 'in' ? compileInOp(filter[1], filter.slice(2)) :
         op === '!in' ? compileNegation(compileInOp(filter[1], filter.slice(2))) :
         op === 'has' ? compileHasOp(filter[1]) :
@@ -71,20 +67,36 @@ function compilePropertyReference(property: string, type?: ?string) {
 }
 
 function compileComparisonOp(property: string, value: any, op: string) {
-    const fallback = op === '!=';
+    const type = typeOf(value).kind;
+    const untypedReference = compilePropertyReference(property);
+    const typedReference = compilePropertyReference(property, typeof value);
+
     if (value === null) {
+        const expression = [
+            '&&',
+            compileHasOp(property),
+            ['==', ['typeof', untypedReference], 'Null']
+        ];
+        return op === '!=' ? ['!', expression] : expression;
+    }
+
+    if (op === '!=') {
         return [
-            'coalesce',
-            [op, ['typeof', compilePropertyReference(property)], 'Null'],
-            fallback
+            '||',
+            ['!=', ['typeof', untypedReference], type],
+            ['!=', typedReference, value]
         ];
     }
-    const ref = compilePropertyReference(property, typeof value);
-    return ['coalesce', [op, ref, value], fallback];
+
+    return [
+        '&&',
+        ['==', ['typeof', untypedReference], type],
+        [op, typedReference, value]
+    ];
 }
 
-function compileLogicalOp(expressions: Array<Array<any>>, op: string) {
-    return [op].concat(expressions.map(convertFilter));
+function compileDisjunctionOp(filters: Array<Array<any>>) {
+    return ['||'].concat(filters.map(convertFilter));
 }
 
 function compileInOp(property: string, values: Array<any>) {
@@ -93,17 +105,26 @@ function compileInOp(property: string, values: Array<any>) {
     }
 
     const input = compilePropertyReference(property);
-    return ["coalesce", ["contains", input, ["array", ["literal", values]]], false];
+    return [
+        '&&',
+        compileHasOp(property),
+        ["contains", input, ["array", ["literal", values]]]
+    ];
 }
 
 function compileHasOp(property: string) {
-    const has = property === '$id' ?
-        ['!=', ['typeof', ['id']], 'Null'] :
-        ['has', property];
-    return has;
+    if (property === '$id') {
+        return ['!=', ['typeof', ['id']], 'Null'];
+    }
+
+    if (property === '$type') {
+        return true;
+    }
+
+    return ['has', property];
 }
 
-function compileNegation(filter: boolean | Array<any>) {
+function compileNegation(filter: mixed) {
     return ['!', filter];
 }
 

src/style-spec/function/evaluation_context.js

@@ -93,7 +93,7 @@ module.exports = () => ({
     },
 
     typeOf: function (x: Value): string {
-        return toString(typeOf(x));
+        return toString(typeOf(typeof x === 'undefined' ? null : x));
     },
 
     asJSType: function (expectedType: string, args: Array<()=>Value>) {

test/unit/style-spec/feature_filter.test.js

@@ -86,7 +86,7 @@ test('==, null', (t) => {
     t.equal(f({zoom: 0}, {properties: {foo: true}}), false);
     t.equal(f({zoom: 0}, {properties: {foo: false}}), false);
     t.equal(f({zoom: 0}, {properties: {foo: null}}), true);
-    t.equal(f({zoom: 0}, {properties: {foo: undefined}}), false);
+    t.equal(f({zoom: 0}, {properties: {foo: undefined}}), true);
     t.equal(f({zoom: 0}, {properties: {}}), false);
     t.end();
 });
@@ -136,7 +136,7 @@ test('!=, null', (t) => {
     t.equal(f({zoom: 0}, {properties: {foo: true}}), true);
     t.equal(f({zoom: 0}, {properties: {foo: false}}), true);
     t.equal(f({zoom: 0}, {properties: {foo: null}}), false);
-    t.equal(f({zoom: 0}, {properties: {foo: undefined}}), true);
+    t.equal(f({zoom: 0}, {properties: {foo: undefined}}), false);
     t.equal(f({zoom: 0}, {properties: {}}), true);
     t.end();
 });
@@ -308,7 +308,7 @@ test('in, null', (t) => {
     t.equal(f({zoom: 0}, {properties: {foo: true}}), false);
     t.equal(f({zoom: 0}, {properties: {foo: false}}), false);
     t.equal(f({zoom: 0}, {properties: {foo: null}}), true);
-    t.equal(f({zoom: 0}, {properties: {foo: undefined}}), false);
+    t.equal(f({zoom: 0}, {properties: {foo: undefined}}), true);
     t.end();
 });
 
@@ -373,7 +373,7 @@ test('!in, null', (t) => {
     t.equal(f({zoom: 0}, {properties: {foo: 0}}), true);
     t.equal(f({zoom: 0}, {properties: {foo: '0'}}), true);
     t.equal(f({zoom: 0}, {properties: {foo: null}}), false);
-    t.equal(f({zoom: 0}, {properties: {foo: undefined}}), true);
+    t.equal(f({zoom: 0}, {properties: {foo: undefined}}), false);
     t.end();
 });