Skip to content

Commit c91badf

Browse files
krlmlrkrlmlr
authored
Merge pull request #585 from duckdb/f-compare-flip
chore: Flip argument order for `expr_comparison()`
2 parents a062a74 + d13962e commit c91badf

File tree

8 files changed

+107
-19
lines changed

8 files changed

+107
-19
lines changed

‎DESCRIPTION‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
Package: duckdb
22
Title: DBI Package for the DuckDB Database Management System
3-
Version: 1.1.2.9900
3+
Version: 1.1.2.9901
44
Authors@R: c(
55
person("Hannes", "Mühleisen", , "[email protected]", role = "aut",
66
comment = c(ORCID = "0000-0001-8552-0029")),

‎R/cpp11.R‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -56,8 +56,8 @@ rapi_expr_constant <- function(val) {
5656
.Call(`_duckdb_rapi_expr_constant`, val)
5757
}
5858

59-
rapi_expr_comparison <- function(exprs, cmp_op) {
60-
.Call(`_duckdb_rapi_expr_comparison`, exprs, cmp_op)
59+
rapi_expr_comparison <- function(cmp_op, exprs) {
60+
.Call(`_duckdb_rapi_expr_comparison`, cmp_op, exprs)
6161
}
6262

6363
rapi_expr_function <- function(name, args, order_bys, filter_bys) {

‎R/relational.R‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ expr_constant <- rapi_expr_constant
3333
#' @return a comparison expression
3434
#' @noRd
3535
#' @examples
36-
#' comp_expr <- expr_comparison(list(expr_constant(-42), expr_constant(42)), ">")
36+
#' comp_expr <- expr_comparison(">", list(expr_constant(-42), expr_constant(42)))
3737
expr_comparison <- rapi_expr_comparison
3838

3939
#' Create a function call expression

‎R/rethrow-gen.R‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -126,9 +126,9 @@ rethrow_rapi_expr_constant <- function(val, call = parent.frame(2)) {
126126
)
127127
}
128128

129-
rethrow_rapi_expr_comparison <- function(exprs, cmp_op, call = parent.frame(2)) {
129+
rethrow_rapi_expr_comparison <- function(cmp_op, exprs, call = parent.frame(2)) {
130130
rlang::try_fetch(
131-
rapi_expr_comparison(exprs, cmp_op),
131+
rapi_expr_comparison(cmp_op, exprs),
132132
error = function(e) {
133133
rethrow_error_from_rapi(e, call)
134134
}

‎src/cpp11.cpp‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -111,10 +111,10 @@ extern "C" SEXP _duckdb_rapi_expr_constant(SEXP val) {
111111
END_CPP11
112112
}
113113
// relational.cpp
114-
SEXP rapi_expr_comparison(list exprs, std::string cmp_op);
115-
extern "C" SEXP _duckdb_rapi_expr_comparison(SEXP exprs, SEXP cmp_op) {
114+
SEXP rapi_expr_comparison(std::string cmp_op, list exprs);
115+
extern "C" SEXP _duckdb_rapi_expr_comparison(SEXP cmp_op, SEXP exprs) {
116116
BEGIN_CPP11
117-
return cpp11::as_sexp(rapi_expr_comparison(cpp11::as_cpp<cpp11::decay_t<list>>(exprs), cpp11::as_cpp<cpp11::decay_t<std::string>>(cmp_op)));
117+
return cpp11::as_sexp(rapi_expr_comparison(cpp11::as_cpp<cpp11::decay_t<std::string>>(cmp_op), cpp11::as_cpp<cpp11::decay_t<list>>(exprs)));
118118
END_CPP11
119119
}
120120
// relational.cpp

‎src/relational.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ using namespace cpp11;
5656
return make_external<ConstantExpression>("duckdb_expr", RApiTypes::SexpToValue(val, 0, false));
5757
}
5858

59-
[[cpp11::register]] SEXP rapi_expr_comparison(list exprs, std::string cmp_op) {
59+
[[cpp11::register]] SEXP rapi_expr_comparison(std::string cmp_op, list exprs) {
6060

6161
ExpressionType expr_type = OperatorToExpressionType(cmp_op);
6262
if (expr_type ==ExpressionType::INVALID) {

‎tests/testthat/_snaps/relational.md‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
# we can create comparison expressions with appropriate operators
2+
3+
Code
4+
expr_comparison("=", list(expr_constant(-42), expr_constant(42L)))
5+
Message
6+
DuckDB Expression: (-42.0 = 42)
7+
8+
---
9+
10+
Code
11+
expr_comparison("!=", list(expr_constant(-42), expr_constant(42L)))
12+
Message
13+
DuckDB Expression: (-42.0 != 42)
14+
15+
---
16+
17+
Code
18+
expr_comparison(">", list(expr_constant(-42), expr_constant(42L)))
19+
Message
20+
DuckDB Expression: (-42.0 > 42)
21+
22+
---
23+
24+
Code
25+
expr_comparison("<", list(expr_constant(-42), expr_constant(42L)))
26+
Message
27+
DuckDB Expression: (-42.0 < 42)
28+
29+
---
30+
31+
Code
32+
expr_comparison(">=", list(expr_constant(-42), expr_constant(42L)))
33+
Message
34+
DuckDB Expression: (-42.0 >= 42)
35+
36+
---
37+
38+
Code
39+
expr_comparison("<=", list(expr_constant(-42), expr_constant(42L)))
40+
Message
41+
DuckDB Expression: (-42.0 <= 42)
42+
43+
# we cannot create comparison expressions with inappropriate operators
44+
45+
Code
46+
expr_comparison("z", list(expr_constant(-42), expr_constant(42L)))
47+
Condition
48+
Error:
49+
! expr_comparison: Invalid comparison operator
50+
51+
---
52+
53+
Code
54+
expr_comparison("2", list(expr_constant(-42), expr_constant(42L)))
55+
Condition
56+
Error:
57+
! expr_comparison: Invalid comparison operator
58+
59+
---
60+
61+
Code
62+
expr_comparison("-", list(expr_constant(-42), expr_constant(42L)))
63+
Condition
64+
Error:
65+
! expr_comparison: Invalid comparison operator
66+

‎tests/testthat/test-relational.R‎

Lines changed: 31 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -77,19 +77,41 @@ test_that("we can create various expressions and don't crash", {
7777
})
7878

7979
test_that("we can create comparison expressions with appropriate operators", {
80-
expr_comparison(list(expr_constant(-42), expr_constant(42L)), "=")
81-
expr_comparison(list(expr_constant(-42), expr_constant(42L)), "!=")
82-
expr_comparison(list(expr_constant(-42), expr_constant(42L)), ">")
83-
expr_comparison(list(expr_constant(-42), expr_constant(42L)), "<")
84-
expr_comparison(list(expr_constant(-42), expr_constant(42L)), ">=")
85-
expr_comparison(list(expr_constant(-42), expr_constant(42L)), "<=")
80+
local_edition(3)
81+
82+
expect_snapshot({
83+
expr_comparison("=", list(expr_constant(-42), expr_constant(42L)))
84+
})
85+
expect_snapshot({
86+
expr_comparison("!=", list(expr_constant(-42), expr_constant(42L)))
87+
})
88+
expect_snapshot({
89+
expr_comparison(">", list(expr_constant(-42), expr_constant(42L)))
90+
})
91+
expect_snapshot({
92+
expr_comparison("<", list(expr_constant(-42), expr_constant(42L)))
93+
})
94+
expect_snapshot({
95+
expr_comparison(">=", list(expr_constant(-42), expr_constant(42L)))
96+
})
97+
expect_snapshot({
98+
expr_comparison("<=", list(expr_constant(-42), expr_constant(42L)))
99+
})
86100
expect_true(TRUE)
87101
})
88102

89103
test_that("we cannot create comparison expressions with inappropriate operators", {
90-
expect_error(expr_comparison(list(expr_constant(-42), expr_constant(42L)), "z"))
91-
expect_error(expr_comparison(list(expr_constant(-42), expr_constant(42L)), "2"))
92-
expect_error(expr_comparison(list(expr_constant(-42), expr_constant(42L)), "-"))
104+
local_edition(3)
105+
106+
expect_snapshot(error = TRUE, {
107+
expr_comparison("z", list(expr_constant(-42), expr_constant(42L)))
108+
})
109+
expect_snapshot(error = TRUE, {
110+
expr_comparison("2", list(expr_constant(-42), expr_constant(42L)))
111+
})
112+
expect_snapshot(error = TRUE, {
113+
expr_comparison("-", list(expr_constant(-42), expr_constant(42L)))
114+
})
93115
})
94116

95117

0 commit comments

Comments
 (0)