-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
add field predicate types to selectionTest in vega-selections #3675
base: main
Are you sure you want to change the base?
Conversation
i believe this supersedes #1839 which is also very stale anyway |
@@ -41,6 +46,16 @@ function testPoint(datum, entry) { | |||
if (!inrange(dval, values[i], false, false)) return false; | |||
} else if (f.type === TYPE_RANGE_LE) { | |||
if (!inrange(dval, values[i], false, true)) return false; | |||
} else if (f.type === TYPE_PRED_LT) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this become a switch now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i personally don't find switch to be clearer in this instance because all of the returns are conditional, so we'd have to add break statements to every one and remember to keep adding them if we change it in the future. IMO, not shorter, possibly less maintainable, not a significant change in readability. however, happy to make the change if you feel otherwise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. I don't have a strong opinion. You could pull the logic here out into a function and then use a return in each branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the if statements only return false because it's an early exit from the loop (none of the comparisons can be false for the test to return true), so we can't use an unconditional return inside the switch without changing the loop too
presumably it is written this way for efficiency(tm) over readability? but it did take me trying to rewrite it to realize this so maybe it should just be made more readable?
what do you think of e.g.
fields.every(f => {
switch (f.type) {
case TYPE_ENUM:
return ...;
case ...
}
})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your intuition is correct @jonathanzong! Because this code path is typically called against every visualized data point on every interaction (eg potentially every mouse drag), we need it to be really performant. When this was first written, most browsers were still faster at evaluating an old school for loop. It was also one of the reasons we minimized functions calls (because they can—or at least could—incur performance overhead).
So, while I’m sympathetic to readability concerns, if we really wanted to make a change here (including extracting this to another function call), perhaps we could run a micro benchmark to validate whether it would cause a performance hit?
(My guess is browsers have advanced significantly since these code paths were first written, so I wouldn’t be surprised if some of these reasons no longer hold. On the other hand, back in the day, I do remember being surprised by what syntaxes did and didn’t incur performance penalties!)
bump @arvind |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me but I will defer to @arvind for the final review.
@arvind can you wrap up the review and merge if it looks good? |
I found a potential problem here: ops in I'm pretty confused but can look into this later but let's not merge this yet until then. |
I converted this pull request to a draft. Please mark it as ready. |
This change introduces the ability for
vlSelectionTest
to check for more types of field predicates.This additive change will have no immediate effect, since Vega-Lite doesn't currently produce selection tuples that use these predicate types. However, future proposed changes for Animated Vega-Lite will require us to support these field predicates in addition to the existing point and interval predicates.
cc @arvind