-
Notifications
You must be signed in to change notification settings - Fork 18
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 tests for overlapping ranges #428
Conversation
This pull request has been linked to Shortcut Story #18608: Add tests for QueryCondition with overlapping ranges. |
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.
👍 with one question.
expect_equal(NROW(arr[]), | ||
sum(with(penguins, year == 2009))) | ||
|
||
qc <- parse_query_condition(year < 2009 || year < 2010) |
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.
Shouldn't this be |
rather than ||
for element-wise comparisons? Ditto for &&
above.
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.
Whooops. Excellent catch. Will fix.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see now:
Lines 132 to 135 in 8f1d2fa
.mapBoolToCharacter <- function(x) switch(x, | |
`&&` = "AND", | |
`||` = "OR", | |
`!` = "NOT") |
Maybe we should translate |
/&
to ||
/&&
behind the scenes for R folks?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.