Conversation
|
This pull request has been linked to Shortcut Story #18608: Add tests for QueryCondition with overlapping ranges. |
| expect_equal(NROW(arr[]), | ||
| sum(with(penguins, year == 2009))) | ||
|
|
||
| qc <- parse_query_condition(year < 2009 || year < 2010) |
There was a problem hiding this comment.
Shouldn't this be | rather than || for element-wise comparisons? Ditto for && above.
There was a problem hiding this comment.
Whooops. Excellent catch. Will fix.
There was a problem hiding this comment.
Sorry, actually no: that is a 'token' for parse_query_condidtion() and not an R element-by-element operator.
There was a problem hiding this comment.
Oh I see now:
Lines 132 to 135 in 8f1d2fa
Maybe we should translate |/& to ||/&& behind the scenes for R folks?
There was a problem hiding this comment.
Actually, any reason not to use the short-form of these as the tokens for AND/OR? (Sorry, I know we've wandered away from the point of this PR).
There was a problem hiding this comment.
Funny you went to the source, I did too :)
My feeling was we should definitely also support AND and OR (and, though currently a no-op) NOT. I feel less strongly about & and | but I suppose we could. In many other programming languages the double && or || is common which is why I went with it.
Maybe this discussion should be another issue or SC ticket. Lemme me merge this now first.
2 participants
This PR adds tests for overlapping ranges in query conditions.
No new code.