Add tests for overlapping ranges #428
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.
This PR adds tests for overlapping ranges in query conditions.
No new code.