Feature/describe prior to write #313

detule · 2019-10-30T02:05:37Z

Please consider this patch-set as potentially a more robust alternative to #70.

With the current solution (master) FreeTDS users face truncation when inserting values longer than 255 characters.
An additional benefit could be performance enhancement when writing to wide tables, as this approach makes less round-trip calls to the server.

Looking forward to hearing your feedback.

…beParam Backport of e99606bc8d01257c7c1316dd06f3a9c30e0a71fd

This is an exported function that is envisioned as a thin wrapper around SQLColumns. Default method points to renamed/reordered connection_sql_columns. We hope to call this method in dbWriteTable, to set parameter descriptions prior to binding values. Given that drivers may have idiosyncratic implementations of SQLColumns (for example, some may not offer the ability to call this function on a table outside of the current catalog) offer an S4 method giving the end-user to write an implementation that works for them.

Calls nanodbc::describe_parameters

Call to result_describe_parameters, which in turn calls nanodbc::describe_parameters, is wrapped in a tryCatch block - if there is a failure, we fallback to the nanodbc code-path where binding uses a call to SQLDescribeParam

jimhester · 2019-10-30T13:11:34Z

Thanks for working on this!

It looks like this change is causing the tests to fail when using PostgreSQL, would you be able to look into this?

detule · 2019-10-30T16:38:06Z

Will do - thanks.

Also fixup documentation

detule · 2019-10-31T11:58:08Z

@jimhester Should be ready for a second look.

The R-devel pipeilne failure seems to be unrelated to this patch-set.

jimhester · 2019-11-04T13:13:28Z

R/Table.R

+      if (!is.null(fieldDetails) && nrow(fieldDetails))
+        result_describe_parameters(rs@ptr, fieldDetails)


Can you put braces around this conditional?

Will do - thanks.

jimhester · 2019-11-04T13:15:10Z

R/Table.R

+      tryCatch({
+        details <- odbcConnectionColumns(conn, name)
+        datails <- details[match(names(values), details$column_name)]
+        details[, c("ordinal_position", "data_type", "column_size", "decimal_digits")]


This call has no effect, did you mean to assign it back to details?

Jim - the output of the tryCatch block is assigned to fieldDetails - the line you quoted there is what gets assigned in the success code-path.

Let me know if you think there is a better approach.

It was due to the indentation style used, I would normally put the tryCatch() line on the same line as the assignment.

Will update - thanks.

jimhester · 2019-11-04T13:15:58Z

R/Table.R

+    fieldDetails <-
+      tryCatch({
+        details <- odbcConnectionColumns(conn, name)
+        datails <- details[match(names(values), details$column_name)]


This could just be

datails <- details[names(values)]

But do we need to reorder the columns at all?

Jim, the intention here was to:

Subset the rows of the output of SQLColumns to those (columns) we are writing to / we need to describe.

I thought some care was needed with the ordering - AFAICT there is no guarantee that the columns in values (and therefore the parameters in the INSERT statement) are ordered the same way as the columns in the table we are writing to (i.e. the rows in details).

Having said that, I think part of the confusion is the use of ordinal_position to index the parameters we are describing - this seems incorrect. After re-ordering the rows in details to match the column order in values as is done here, the index that gets passed nanodbc::statement::describe_parameters should be 1:ncol(values)

Will update.

jimhester · 2019-11-04T13:19:12Z

R/Connection.R

+      "sql_datetime_subtype", "char_octet_length", "ordinal_position",
+      "nullable")
+    detail <-
+      detail[, c(5, 4, 3, 1, 6, 2, 7, 8, 9, 10, 17, 11, 12, 13, 14, 15, 16)]


Indexing by number seems too brittle, could we reorder by name first, then rename the columns if necessary.

Will change - thanks.

jimhester · 2019-11-04T13:22:24Z

R/Connection.R

+    if (!is.null(column_name))
+      detail <- subset(detail, detail$column_name == column_name)


Could we add braces here,

Also generally it is best to avoid using subset inside functions, the type of non-standard evaluation in subset() is not robust enough.

detail <- detail[detail$column_name == column_name, ] is the equivalent.

Also it is somewhat surprising that you would need to do an additional filtering, if you search for a specific column in SQLColumns only that column should be returned.

Will change - thanks.

- Wrap conditional expressions in braces - Avoid using numeric indexing when re/ordering output of connection_sql_columns - Bugfix: Appropriate index passed down to nanodbc::statement::describe_parameters

Check the case when, via `dbWriteTable`, we are writing a data.frame with columns ordered differently than the table we are writing to. In `dbWriteTable` we attempt to describe parameters (types, length) of the INSERT query using information about the columns of the table being written to. Some care is needed to make sure that the table column descriptions get paired with the appropriate parameters.

detule · 2019-11-06T03:10:25Z

@jimhester Thanks for the feedback!

Patched conditional braces, numeric column indexing
Fixed bug with pairing table column descriptions with INSERT statement parameters. Added a test.

test_roundtrip is perhaps not the best home for this test - happy to move it to a more appropriate location if you have any suggestions.

jimhester · 2019-11-08T14:55:25Z

R/Connection.R

+      "decimal_digits", "numeric_precision_radix", "nullable", "remarks",
+      "column_default", "sql_data_type", "sql_datetime_subtype",
+      "char_octet_length", "ordinal_position")]
+    names(detail)[c(1, 2, 4, 6, 13)] <- c("table_cat", "table_schem",


I think I would prefer to not rename. And do we need to reorder the columns?

Jim my original thinking here was to make the output as close to the output of the ODBC SQLColumns function as possible. It seemed to me that, since we are offering the end-user the ability to write their own implementation, then it is more natural to anchor on the field naming of the ODBC's SQLColumns API, than on the choices made when implementing the internal connection_sql_columns.

If you also think that's worthwhile then we either re-name the columns here, or change the output of connection_sql_columns. The latter seemed less desirable since even though internal, I wasn not sure if there aren't folks that have developed dependencies.

The re-ordering was more thinking in the same direction - making the output align with what I can see when I execute sp_columns in my SQL Server client for example.

I don't feel strongly here - happy to make this just a wrapper around connection_sql_columns if you think that's the best way forward.

I would prefer it to just be a wrapper around connection_sql_columns

jimhester · 2019-11-08T14:59:47Z

I would prefer a separate test outside of the roundtrip that creates a table and appends to it.

Also move test checking for out-of-order column write to tests-PostgreSQL

detule · 2019-11-11T19:28:49Z

@jimhester Thanks again - appreciate the comments.

Let me know if I missed anything.

jimhester · 2019-11-19T22:03:42Z

detule · 2019-11-20T00:46:01Z

Jim: thank you very much - I know it was a chunky PR, and I appreciate the thoughtful comments.

Any thoughts on when this might make its way to CRAN?

jimhester · 2019-11-20T18:12:59Z

I was aiming for a CRAN release on odbc next week sometime.

detule added 4 commits October 29, 2019 21:55

nanodbc: Add describe_parameters function as alternative to SQLDescri…

7a1fd81

…beParam Backport of e99606bc8d01257c7c1316dd06f3a9c30e0a71fd

Add result_describe_parameters R/cpp function

e038cf0

Calls nanodbc::describe_parameters

detule added 2 commits October 31, 2019 01:27

odbcConnectionColumns: Respect column_names parameter

ff1ac8c

Also fixup documentation

dbWriteTable: Use concise data types when describing parameters

40510f5

jimhester suggested changes Nov 4, 2019

View reviewed changes

detule added 2 commits November 6, 2019 02:50

code-review: Incorporate feedback

ec0c38d

- Wrap conditional expressions in braces - Avoid using numeric indexing when re/ordering output of connection_sql_columns - Bugfix: Appropriate index passed down to nanodbc::statement::describe_parameters

jimhester reviewed Nov 8, 2019

View reviewed changes

code-review ctd: No re-naming/-ordering in odbcConnectionColumns

d09b9e1

Also move test checking for out-of-order column write to tests-PostgreSQL

jimhester added 3 commits November 19, 2019 16:37

Add note to NEWS

74c9a55

Merge branch 'master' into feature/describe_prior_to_write

c7e5ee1

Merge branch 'master' into feature/describe_prior_to_write

8e258eb

jimhester merged commit 7c4bc6e into r-dbi:master Nov 19, 2019

jimhester mentioned this pull request Nov 19, 2019

dbWriteTable() throwing an "Invalid precision value" error with Access #79

Closed

jimhester mentioned this pull request Nov 21, 2019

Support for ODBC drivers that do not allow binding array of parameters (e.g. MS Access) #263

Closed

shrektan mentioned this pull request Nov 25, 2019

after #313 dbWriteTable() is case-sensitive in terms of columns #319

Closed

jimhester mentioned this pull request Aug 24, 2022

Avoid querying column types from the database in dbWriteTable() and dbAppendTable() #458

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/describe prior to write #313

Feature/describe prior to write #313

detule commented Oct 30, 2019 •

edited

jimhester commented Oct 30, 2019

detule commented Oct 30, 2019

detule commented Oct 31, 2019 •

edited

jimhester Nov 4, 2019

detule Nov 5, 2019

jimhester Nov 4, 2019

detule Nov 5, 2019

jimhester Nov 8, 2019

detule Nov 8, 2019

jimhester Nov 4, 2019

detule Nov 5, 2019 •

edited

jimhester Nov 4, 2019

detule Nov 5, 2019

jimhester Nov 4, 2019

detule Nov 5, 2019

detule commented Nov 6, 2019

jimhester Nov 8, 2019

detule Nov 8, 2019

jimhester Nov 8, 2019

jimhester commented Nov 8, 2019

detule commented Nov 11, 2019

jimhester commented Nov 19, 2019

detule commented Nov 20, 2019

jimhester commented Nov 20, 2019

		if (!is.null(fieldDetails) && nrow(fieldDetails))
		result_describe_parameters(rs@ptr, fieldDetails)

		if (!is.null(column_name))
		detail <- subset(detail, detail$column_name == column_name)

Feature/describe prior to write #313

Feature/describe prior to write #313

Conversation

detule commented Oct 30, 2019 • edited

jimhester commented Oct 30, 2019

detule commented Oct 30, 2019

detule commented Oct 31, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

detule Nov 5, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

detule commented Nov 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimhester commented Nov 8, 2019

detule commented Nov 11, 2019

jimhester commented Nov 19, 2019

detule commented Nov 20, 2019

jimhester commented Nov 20, 2019

detule commented Oct 30, 2019 •

edited

detule commented Oct 31, 2019 •

edited

detule Nov 5, 2019 •

edited