Skip to content
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.

Sign up for GitHub

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 'rows affected' output when using sql engine #2051

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add 'rows affected' output when using sql engine #2051

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f78e1c6
Update engine.R
edalfon Sep 24, 2021
d1f8cbf
Update engine.R
edalfon Sep 24, 2021
e45a204
Update NEWS.md
edalfon Sep 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
- The `sql` engine now produces an output for update-like SQL queries, indicating the number of rows affected (as returned by DBI::dbExecute). So far, `knitr` produces a nicely formatted output for queries returning a result set (typically SELECT...), but gives no feedback at all for queries making changes to the DB (e.g. INSERT|UPDATE|DELETE|CREATE|DROP; see not exported function knitr:::is_sql_update_query). For such queries, `knitr` now shows the number of affected rows. You can also set the chunk option `output.var` to assign the number of affected rows to a variable.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just so you know, this should be under the last header 1 you see.
1.35 is the next version so this should be moved under.

I'll do it before merging.




# CHANGES IN knitr VERSION 1.35

## BUG FIXES
Expand Down
9 changes: 7 additions & 2 deletions R/engine.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -555,8 +555,8 @@ eng_sql = function(options) {

data = tryCatch({
if (is_sql_update_query(query)) {
DBI::dbExecute(conn, query)
NULL
data = DBI::dbExecute(conn, query)
data
} else if (is.null(varname) && max.print > 0) {
# execute query -- when we are printing with an enforced max.print we
# use dbFetch so as to only pull down the required number of records
Expand Down Expand Up @@ -633,6 +633,11 @@ eng_sql = function(options) {

} else print(display_data) # fallback to standard print
})
if (is.numeric(data) && length(data) == 1 && is.null(varname)) {
options$results = 'asis'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really know if we want to force results = 'asis' here. The user would have not control over it.

I understand why it makes sens to have it as markdown text in the output document, but it may not please everyone who would prefer to have it code output.

I don't really know but usually users have control over how the output is show. 🤔

@yihui what do you think ?

# format = "fg" instead of "d". Row counts on DB may be greater than integer max value
output = paste0("Number of affected rows: ", formatC(data, format = "fg", big.mark = ','))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to previous comment, just wondering about internationalization:

  • We are enforcing a english sentence to be output in a report. This is not perfect for non English report. With result = 'asis', this sentence would be shown as usual text. If I am writing a French report, that would be weird to have such English output in the middle of other text, wouldn't it ?
    That is why at least, not enforcing asis output seems better to me
  • Also, about big.mark = ",", I am just wondering if this is necessary. It is not something usual worldwide, and could even be confusing for some international user. For example, we don't have that in Europe, we often have no big mark or a space. A comma is used as decimal too. However, the context of number of rows makes 12,500 clear enough for a european even if it can be confusing. We would use 12 500 or just 12500. Is it something common for such update query output ?

Maybe we could / should make at least all that configurable ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree!

What I did was trying to keep it similar to how the caption is formatted (line 614 on the same file)

rows_formatted = formatC(rows, format = "d", big.mark = ',')

but you are right that in that case, the user has options to configure the output

}
Comment on lines +636 to +640
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To follow the style of the previous output assignment, and making it more clear about the different output possible, I would use a else here for the previous if

Suggested change
if (is.numeric(data) && length(data) == 1 && is.null(varname)) {
options$results = 'asis'
# format = "fg" instead of "d". Row counts on DB may be greater than integer max value
output = paste0("Number of affected rows: ", formatC(data, format = "fg", big.mark = ','))
}
else if (is.numeric(data) && length(data) == 1 && is.null(varname)) {
options$results = 'asis'
# format = "fg" instead of "d". Row counts on DB may be greater than integer max value
paste0("Number of affected rows: ", formatC(data, format = "fg", big.mark = ','))
}

if (options$results == 'hide') output = NULL

# assign varname if requested
Expand Down