Relax typing for equality and inequality expressions #6961

anandthakker · 2018-07-13T14:52:24Z

Relax ==/!= typing by allowing ["==", value, value] (e.g., ["==", ["get", "x"], ["get", "y"])
Relax inequality typing by also allowing arguments of type value (e.g. ["get", "x"] without a wrapping type assertion). At runtime, if the feature property doesn't have the same type as the other argument, or if the argument type is not string or number, then the expression will evaluate to an error.
briefly describe the changes in this PR
write tests for all new functionality
document any changes to public APIs
post benchmark scores
manually test the debug page
tagged @mapbox/studio and/or @mapbox/maps-design if this PR includes style spec changes

jfirebaugh · 2018-07-13T17:40:35Z

Relax inequality typing by allowing one of the two arguments to be value (e.g. ["get", "x"] without a wrapping type assertion).

What do you think about relaxing this to the same level as ==/!=, i.e. allowing both arguments to be value (but still erroring if they're not the same runtime type).

anandthakker · 2018-07-13T17:51:36Z

@jfirebaugh I lean against it, but I don't feel super strongly about it. My thinking is that requiring one argument to be concretely typed lets us know--and requires the user to know--at compile time what kind of comparison we're doing. And that [">", ["get", "pop_2010"], ["get", "pop_2000"]] could silently produce surprising results for a messy dataset in which, say, some features had strings for both pop_2010 and pop_2000.

jfirebaugh · 2018-07-13T19:05:48Z

The advantage is it eliminates another case where explicit assertions are required -- I think the last major one besides #6190. Given that feedback indicates that assertions and coercions are one of the most confusing and unergonomic features of expressions, I think that's a win that outweighs the drawbacks in the relatively less common "messy data" case.

ChrisLoer · 2018-07-13T19:54:54Z

docs/components/expression-metadata.js

+    parameters: ['value', 'value']
+}];
+
+const inequalitySignatures = [{


The name here tripped me up for a bit because inequalitySignatures sounds like it applies to !=. I guess "comparison" already includes both types... maybe something like "orderingComparisonSignatures"? or "sortComparison"?

ChrisLoer · 2018-07-13T19:57:58Z

docs/components/expression-metadata.js

-    }],
+    '==': [].concat(equalitySignatures),
+    '!=': [].concat(equalitySignatures),
+    '<': [].concat(inequalitySignatures),


Why do the arrays have to be copied here instead of shared? Not that I see a problem with it, I think I'm just missing some hidden requirement.

Eh they don't -- I don't even know why I did it

Seems like explicitly listing out the possible combinations might be more confusing than collapsing the "formal" signature down to just (value, value), and explaining the constraints in prose. Something like...

For ==, !=:

The comparison is strictly typed; arguments of different runtime types are considered unequal. Cases where the types are known to be different at parse time produce a parse error.

For others:

The arguments are required to be either both strings or both numbers; if during evaluation they are not, expression evaluation produces an error. Cases where this constraint is known not to hold at parse time produce a parse error.

ChrisLoer · 2018-07-13T20:26:39Z

src/style-spec/expression/definitions/comparison.js

+    lhs: Expression;
+    rhs: Expression;
+    collator: ?Expression;
+    compare: (EvaluationContext, Expression, Expression, ?Expression) => any;


We need to explicitly omit this from the serialization registration, right? I think this is what's causing the test failure in regressions/mapbox-gl-js#5947.

Yep, good catch -- fixed

anandthakker · 2018-07-16T12:57:17Z

Given that feedback indicates that assertions and coercions are one of the most confusing and unergonomic features of expressions, I think that's a win that outweighs the drawbacks in the relatively less common "messy data" case.

Sold. Added the (value, value) case.

jfirebaugh · 2018-07-16T15:52:35Z

src/style-spec/expression/definitions/comparison.js

+            let collator = null;
+            if (args.length === 4) {
+                if (lhs.type.kind !== 'string' && rhs.type.kind !== 'string') {
+                    return context.error(`Cannot use collator to compare non-string types.`);


Does this need to be deferred to runtime for (value, value) comparisons, given that one of the key uses for collator ["==", ["get", "name"], ["get", "name_en"], ["collator", {...}]]?

Oh, good point

🤔 if there's a collator argument, what should ["==", ["get", "name"], ["get", "name_en"], ["collator", {...}]] do on a feature where one or both of name or name_en aren't strings?

Two possibilities:

When there's a collator, infer ["string", ...] annotations, so that it's a runtime error if either one is not a string. (So: no relaxed type semantics when there's a collator)

Use collator.compare() if both arguments evaluate to strings, and === otherwise.

A third possibility:

Effectively do the first option, but special-case null, returning false if one is null and the other is string, and true both are null. Other types would produce a runtime type error just as with a ["string", ...] wrapper.

@nickidlugash ^ any gut feeling on this issue?

@anandthakker For the use case of ["==", ["get", "name"], ["get", "name_en"], ["collator", {...}]], it will be common for there to be null values in either/both string. Given that, I think the desired behavior would be for this expression to return false if there is a null value, rather than producing an error. I think either the second or third option would give the desired result?

Are we also special-casing null values for other comparisons? E.g. does ["==", ["get", "name"], ["get", "name_en"]] return false if one of the values is null? What about if the comparison was between a number and a null value?

Are we also special-casing null values for other comparisons? E.g. does ["==", ["get", "name"], ["get", "name_en"]] return false if one of the values is null? What about if the comparison was between a number and a null value?

Yep, these will both now return false

jfirebaugh · 2018-07-16T15:55:09Z

src/style-spec/expression/definitions/comparison.js

+        evaluate(ctx: EvaluationContext) {
+            const lhs = this.lhs.evaluate(ctx);
+            const rhs = this.rhs.evaluate(ctx);
+            if (this.needsRuntimeTypeCheck) {


What do the benchmark scores look like? One potential optimization is to specialize evaluate in subclasses to eliminate needsRuntimeTypeCheck and collator branching.

jfirebaugh · 2018-07-16T16:06:41Z

src/style-spec/expression/definitions/comparison.js

+                return context.concat(2).error(`"${op}" comparisons are not supported for type '${toString(rhs.type)}'.`);
+            }
+
+            if (!(


I find it more difficult to understand this condition than if the ! was distributed.

jfirebaugh · 2018-07-16T16:13:34Z

docs/components/expression-metadata.js

-    }],
+    '==': [].concat(equalitySignatures),
+    '!=': [].concat(equalitySignatures),
+    '<': [].concat(inequalitySignatures),


Seems like explicitly listing out the possible combinations might be more confusing than collapsing the "formal" signature down to just (value, value), and explaining the constraints in prose. Something like...

For ==, !=:

The comparison is strictly typed; arguments of different runtime types are considered unequal. Cases where the types are known to be different at parse time produce a parse error.

For others:

The arguments are required to be either both strings or both numbers; if during evaluation they are not, expression evaluation produces an error. Cases where this constraint is known not to hold at parse time produce a parse error.

Use collator.compare() if both arguments evaluate to strings, and === otherwise

anandthakker · 2018-07-24T13:34:23Z

Benchmarks show no significant changes.

@jfirebaugh the new == semantics almost match filter-==: the only difference is that checking for equality between an undefined/missing property and null, legacy filters treat it as unequal, and expressions treat it as equal

Ports mapbox/mapbox-gl-js#6961

* Relax typing for comparison operators Ports mapbox/mapbox-gl-js#6961 * Review comments * Lint fixes

anandthakker requested review from jfirebaugh and ChrisLoer July 13, 2018 14:52

anandthakker changed the title ~~Closes #6459~~ Relax typing for equality and inequality expressions Jul 13, 2018

ChrisLoer reviewed Jul 13, 2018

View reviewed changes

anandthakker force-pushed the relax-comparison-typing branch from cfbc92f to 0ccf28f Compare July 16, 2018 12:55

anandthakker force-pushed the relax-comparison-typing branch from 0ccf28f to 212844d Compare July 16, 2018 13:05

jfirebaugh reviewed Jul 16, 2018

View reviewed changes

ChrisLoer mentioned this pull request Jul 23, 2018

Multiple text formats for symbols via "format" expression #6994

Merged

13 tasks

Anand Thakker added 7 commits July 24, 2018 08:41

Update wording for == expression type error

63e64dc

Allow ['==', value, value]

6967e5b

equals.js -> comparison.js

1cf1e63

Relax typing for order comparison operators

232bc32

Allow (value, value) for order comparisons

8009c79

Allow [==, value, value, collator]

afc7d29

Use collator.compare() if both arguments evaluate to strings, and === otherwise

Simplify documentation for comparison operators

a964dcc

anandthakker force-pushed the relax-comparison-typing branch from ccb6e1d to a964dcc Compare July 24, 2018 12:55

anandthakker requested review from jfirebaugh and emilymdubois July 24, 2018 16:28

jfirebaugh approved these changes Jul 24, 2018

View reviewed changes

anandthakker merged commit 22a0345 into master Jul 24, 2018

anandthakker deleted the relax-comparison-typing branch July 24, 2018 17:27

jfirebaugh mentioned this pull request Jul 25, 2018

Upgrade test/integration to ES6 modules #6880

Merged

ChrisLoer mentioned this pull request Jul 31, 2018

Re-implement "avoid edges" for MapMode::Tile mapbox/mapbox-gl-native#12520

Merged

4 tasks

anandthakker pushed a commit to mapbox/mapbox-gl-native that referenced this pull request Aug 2, 2018

Relax typing for comparison operators

f95a6b5

Ports mapbox/mapbox-gl-js#6961

anandthakker mentioned this pull request Aug 2, 2018

Relax typing for comparison operators mapbox/mapbox-gl-native#12537

Merged

anandthakker added a commit to mapbox/mapbox-gl-native that referenced this pull request Aug 2, 2018

Relax typing for comparison operators (#12537)

d25e1a1

* Relax typing for comparison operators Ports mapbox/mapbox-gl-js#6961 * Review comments * Lint fixes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Relax typing for equality and inequality expressions #6961

Relax typing for equality and inequality expressions #6961

anandthakker commented Jul 13, 2018 •

edited

Loading

jfirebaugh commented Jul 13, 2018

anandthakker commented Jul 13, 2018 •

edited

Loading

jfirebaugh commented Jul 13, 2018 •

edited

Loading

ChrisLoer Jul 13, 2018

ChrisLoer Jul 13, 2018

anandthakker Jul 13, 2018

jfirebaugh Jul 16, 2018

ChrisLoer Jul 13, 2018

anandthakker Jul 16, 2018

anandthakker commented Jul 16, 2018

jfirebaugh Jul 16, 2018

anandthakker Jul 16, 2018

anandthakker Jul 16, 2018

anandthakker Jul 16, 2018

anandthakker Jul 18, 2018

anandthakker Jul 18, 2018

nickidlugash Jul 18, 2018

anandthakker Jul 23, 2018

jfirebaugh Jul 16, 2018

jfirebaugh Jul 16, 2018

jfirebaugh Jul 16, 2018

anandthakker commented Jul 24, 2018

Relax typing for equality and inequality expressions #6961

Relax typing for equality and inequality expressions #6961

Conversation

anandthakker commented Jul 13, 2018 • edited Loading

jfirebaugh commented Jul 13, 2018

anandthakker commented Jul 13, 2018 • edited Loading

jfirebaugh commented Jul 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anandthakker commented Jul 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anandthakker commented Jul 24, 2018

anandthakker commented Jul 13, 2018 •

edited

Loading

anandthakker commented Jul 13, 2018 •

edited

Loading

jfirebaugh commented Jul 13, 2018 •

edited

Loading