Skip to content
This repository has been archived by the owner on Apr 14, 2018. It is now read-only.

problem with copying build_query from dplyr #40

Closed
bluaze opened this issue Jul 3, 2015 · 3 comments
Closed

problem with copying build_query from dplyr #40

bluaze opened this issue Jul 3, 2015 · 3 comments
Assignees
Labels
Milestone

Comments

@bluaze
Copy link

bluaze commented Jul 3, 2015

build_query is a slightly revised version of build_query in dplyr, and the difference is basically the change from limit parameter to top papameter.

the problem is, most verbs in dplyr such as select, filter, setdiff will call update, which in turn will call build_query, but the build_query is just an ordinary function, not a generic function, and because of the namespace mechanism, the build_query called in these verbs is dplyr:::build_query, not RSQLServer:::build_query. At present, RSQLServer:::build_query is just called by RSQLServer:::head, as it's defined in RSQLServer.

to solve the confict between top and limit, it's not neccensary to keep a RSQLServer version of build_query. it will work that using limit parameter to generate TOP keyword in select or FETCH clause depending on whether offset is not null and db.version>=11 in sql_select.SQLServerConnection, like this,

if (!is.null(limit)) {
assertthat::assert_that(assertthat::is.number(limit))
limit <- build_sql("TOP ", dplyr::escape(limit))
}

assertthat::assert_that(is.character(select), length(select) > 0L)
out$select <- dplyr::build_sql("SELECT ", limit, " ",
dplyr::escape(select, collapse = ", ", con = con))
assertthat::assert_that(assertthat::is.string(from))
out$from <- dplyr::build_sql("FROM ", from, con = con)

if to include a RSQLServer version of build_query is neccesary, it's better to have a method of update for SQLServerConnection.

@imanuelcostigan
Copy link
Owner

Diff of dplyr and RSQLServer mod of build_query:

diff -r  
1,2c1,2
< build_query <- function(x, limit = NULL) {
<   assert_that(is.null(limit) || (is.numeric(limit) && length(limit) == 1))
---
> build_query <- function (x, top = NULL) {
>   assertthat::assert_that(is.null(top) || (is.numeric(top) && length(top) == 1))
4c4
<     translate_sql_q(expr, tbl = x, env = NULL, ...)
---
>     dplyr::translate_sql_q(expr, tbl = x, env = NULL, ...)
13c12
<     vars <- auto_names(select)
---
>     vars <- dplyr:::auto_names(select)
19,20c18,20
<     select_sql <- translate(x$select, window = uses_window_fun(x$select, x))
<     vars <- auto_names(x$select)
---
>     select_sql <- translate(x$select,
>       window = dplyr:::uses_window_fun(x$select, x))
>     vars <- dplyr:::auto_names(x$select)
34c34
<   if (!uses_window_fun(x$where, x)) {
---
>   if (!dplyr:::uses_window_fun(x$where, x)) {
40,42c40
<     base_query <- update(x,
<       group_by = NULL,
<       where = NULL,
---
>     base_query <- update(x, group_by = NULL, where = NULL,
45,46c43,44
<     from_sql <- build_sql("(", base_query$sql, ") AS ", ident(unique_name()),
<       con = x$src$con)
---
>     from_sql <- dplyr::build_sql("(", base_query$sql, ") AS ",
>       dplyr::ident(unique_name()), con = x$src$con)
53,54c50,51
<     limit = limit)
<   query(x$src$con, sql, vars)
---
>     top = top)
>   dplyr::query(x$src$con, sql, vars)

@imanuelcostigan
Copy link
Owner

Only differences are qualified calls to dplyr functions and renaming parameter from limit to top

@imanuelcostigan
Copy link
Owner

Removed build_query in 7954825 and will reintroduce properly along with an implementation of update to fix #49

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

2 participants