Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement in expression #7197

Closed
wants to merge 5 commits into from
Closed

Conversation

brncsk
Copy link
Contributor

@brncsk brncsk commented Aug 25, 2018

This PR implements an in expression to check if an array contains a value. Fixes #4698.

E.g.:

["in", 3, ["literal", [1, 2, 3]]]

Launch Checklist

  • 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

@brncsk
Copy link
Contributor Author

brncsk commented Sep 3, 2018

Anything I can do to get this merged?

Copy link
Contributor

@jfirebaugh jfirebaugh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @brncsk, thanks for the contribution! This looks good to me. Some considerations:

  • We were previously calling this operator "contains", but I like "in" too, for its brevity and continuity with the prior filter syntax. Also the argument order makes slightly more sense.
  • We removed "contains" because we weren't certain about the typing rules, but we were leaning toward typing it as (value, array<value>) => boolean as you've done here. I still think that's the right call.
  • However, since "in" is using the same equality semantics as "==" (i.e. JS ===), it should have the same constraints, so it can't be used with types where === doesn't make sense. Specifically the type of needle should be limited to these types. If the static type is not one of those five, it should be a parse error. Or, if the static type is value and the runtime type isn't one of the other four, it should be an evaluation error.
  • Could you expand the tests here to cover more of the cases that were removed in Remove "contains" expression support #5412?
  • Would you be able and willing to port this change to mapbox-gl-native as well?

Thanks again for the contribution and sorry it took us a while to get to!

@@ -21,6 +21,8 @@ function isExpressionFilter(filter: any) {
return filter.length >= 2 && filter[1] !== '$id' && filter[1] !== '$type';

case 'in':
return filter.length === 3 && Array.isArray(filter[2]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can imagine us adding a collator argument to "in". We introduced a bug when we we did that for string comparison operators. Let's future-proof this line by checking filter.length >= 3.

@jfirebaugh
Copy link
Contributor

/cc @mapbox/studio and/or @mapbox/maps-design

@nickidlugash
Copy link

@jfirebaugh thanks for looping Carto in. I don't have experience styling with data fields containing arrays, so I don't feel like I have much insight here. I read through the issue trail though, and this seems okay.

@jonseppanen
Copy link

Is there anything holding up the merging of this? We have some pretty essential stuff relying on this and have been holding off using mapbox until this feature comes through.

@mollymerp
Copy link
Contributor

@brncsk are you planning on continuing the implementation of this feature and taking John's suggestions above?

@andrey-hohlov
Copy link

Any updates?

@brncsk
Copy link
Contributor Author

brncsk commented Dec 17, 2018

I'll probably have the time to do it by the end of the month, until then you might wanna look at https://www.mapbox.com/mapbox-gl-js/style-spec/#expressions-match and see if the "array of literal values" case fulfils your needs.

@nextstopsun nextstopsun mentioned this pull request Jan 25, 2019
4 tasks
@stdmn
Copy link

stdmn commented Feb 21, 2019

@brncsk @jfirebaugh Any news on the implementation of this? This along with #7796 seem to be joined at the hip and are extremely valuable.

@mayteio
Copy link

mayteio commented Jul 4, 2019

Bump - any reason this hasn't been merged yet?

@mourner
Copy link
Member

mourner commented Jul 4, 2019

@mayteio yes — the PR feedback hasn't yet been addressed, and our team didn't have the bandwidth to take over the implementation yet. Any style spec changes affect the whole ecosystem (including native SDKs) and are almost permanent, so need great care and consideration of smallest details before potentially merging.

@NameFILIP
Copy link

It would be good to have this implemented. My workaround is the following:

[
  'match',
  ['get', 'id'],
  [
    'fake-value-to-make-the-match-validator-happy',
    ...arrayOfSelectedIds,
  ],
  2,
  1,
],

@ryanhamley
Copy link
Contributor

ryanhamley commented Oct 15, 2019

I'm closing this as stale in favor of #8876. Thanks for getting it started @brncsk; I've added you as a co-author on the new PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

An expression to check if an array contains a given value