-
Notifications
You must be signed in to change notification settings - Fork 173
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
Use TOP 100 PERCENT as stop-gap to allow subqueries with ORDER BY #277
Conversation
Re-building |
test_that("ORDER BY in subqueries uses TOP 100 PERCENT (#175)", { | ||
mf <- lazy_frame(x = 1:3, con = simulate_mssql()) | ||
|
||
expect_equal( |
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.
Maybe it's better to use expect_known_output()
here?
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.
This code is consistent with the rest of the file. I filed #285 to address this.
@@ -103,7 +103,8 @@ sql_render.select_query <- function(query, con, ..., bare_identifier_ok = FALSE) | |||
con, query$select, from, where = query$where, group_by = query$group_by, | |||
having = query$having, order_by = query$order_by, limit = query$limit, | |||
distinct = query$distinct, | |||
... | |||
..., |
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.
Since you touched this function, can you please put each of the other arguments on its own line?
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.
Done. I kept the unnamed arguments on one line.
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.
Can you please add a bullet to NEWS? It should briefly describe the change and end with (@yourname, #issuenumber)
.
Azure Data Warehouse doesn't support TOP [...] PERCENT so this broke dbplyr for ADW. Would it be feasible to emit this only when the subquery actually has an order by clause? |
Any workaround for Azure Data Warehouse besides sticking to an older version? |
This also breaks code where the connection is a Parallell Data Warehouse (PDW) with MSSQL, doesn't support |
for MSSQL.
Closes #275.