R implementation of sqlParseVariablesImpl #83

hannes · 2016-02-15T11:52:55Z

Fixes #82.

hannes · 2016-02-23T12:15:45Z

@hadley any comments?

krlmlr · 2016-02-23T12:18:43Z

R/interpolate.R

+            in_quote <- q
+            break
+          }
+      } else {


Indentation looks odd here.

krlmlr · 2016-02-23T12:23:03Z

What's the run time of the new code on a 1000, 10000, 100000-char string, compared to the old code?

hannes · 2016-02-23T12:39:17Z

did a roxygen run, will check indents next

hannes · 2016-02-23T12:52:14Z

whee the checks have passed. quick benchmark next.

hannes · 2016-02-23T13:26:13Z

Tested with the following line on my MacBook:

system.time(sqlParseVariables(ANSI(), paste0(rep("S", 100000), collapse="")))

Runtimes old code:
1000-char: 0.00s
10000-char: 0.00s
100000-char: 0.02

Runtimes new code:
1000-char: 0.03s
10000-char: 0.48s
100000-char: 34.06s (!)

So, as to expected, its way slower. But I would say since this is just a fallback and queries are expected to be shorter than 10K characters its not going to matter much.

krlmlr · 2016-02-23T13:33:26Z

It might turn out that this function will be called for all parametrized queries, regardless of backend support, to provide a backend-independent syntax for parametrized queries. Anyway, the C++ implementation might as well live in a separate package.

The apparent non-linear behavior of the R implementation bothers me. Any chance we can get rid of it, perhaps through some clever strsplit() preprocessing?

~~Could you please post/upload the testing code?~~

Does the presence of quotes/comments in the text affect run time?

hannes · 2016-02-23T14:26:57Z

Presence of quotes/comments only changes state bits, so I would not expect that to make any difference.

hannes · 2016-02-23T15:43:52Z

Strange, when run in Rstudio it only takes 4.23 seconds on the largest string as one would expect form linearity.

hannes · 2016-02-23T15:52:37Z

looks like substr is the main culprit here. Will try to optimize.

hannes · 2016-02-24T10:54:13Z

Got the slowest time (100K char string) down to 1.5 sec. Pushing today...

krlmlr · 2016-02-24T11:38:29Z

This is very nice indeed! Looking forward to it.

hannes · 2016-02-24T11:54:44Z

Runtimes faster new code:
1000-char: 0.01s
10000-char: 0.14s
100000-char: 1.52s
1000000-char: 14.39s

krlmlr · 2016-02-24T14:39:52Z

R/interpolate.R

+#' @export
+#' @rdname sqlParseVariables
+sqlParseVariablesImpl <- function(sql, quotes, comments) {
+  sql_arr <- strsplit(as.character(sql), "", fixed=T)[[1]]


Could you please use TRUE instead of T?

krlmlr · 2016-02-24T14:42:03Z

R/interpolate.R

+          }
+        } else {
+          # only check the end of the active quote definition
+          # TODO: support end quote escaping (e.g. \")


Was end quote escaping supported by the Rcpp code?

I think there was support for it, although not tested: https://codecov.io/github/rstats-db/DBI/src/sqldelim.cpp?ref=ef1a72121ed1f34e5bf94a913ea3410d9b289f42#l-61 . I'd like to review the old code and have it fully covered by tests before proceeding here. Also, to me it looks like the construct "SELECT ? FROM A" will be interpreted in a strange way by the C++ implementation.

hannes · 2016-02-24T14:42:48Z

Not sure about the end quote escaping.

krlmlr · 2016-03-02T14:16:35Z

Run time is still quadratic in the number of variables, but I guess we can live with that:

> s <- paste0(rep("?a", 10000), collapse=""); system.time(sqlParseVariables(ANSI(), s))
   user  system elapsed 
  0.520   0.000   0.521 
> s <- paste0(rep("?a", 20000), collapse=""); system.time(sqlParseVariables(ANSI(), s))
   user  system elapsed 
  1.840   0.000   1.846

hannes · 2016-03-02T14:18:09Z

Not using Rstudio here.

hannes · 2016-03-02T14:23:12Z

No idea what happened to the alias. This file is generated after all.

krlmlr · 2016-03-02T14:27:02Z

It's not the alias, just the name (which is good as it is now).

Looks good to me. @hadley: Any further comments?

hadley · 2016-03-02T14:28:11Z

R/interpolate.R

+  in_comment <- 0L
+  i <- 1
+  while(i <= length(sql_arr)) {
+    # only check for variables if neither commented nor quoted


Indenting looks slightly off here

hannes · 2016-03-02T14:29:21Z

added a missing space.

hadley · 2016-03-02T14:30:36Z

My main concern the C++ used a state machine which is easy to reason about, and easy to adapt to changing requirements. I can't easily identify the strategy used in the new code, which means that if requirements change, @hannesmuehleisen will have to make the changes. That said, I don't think this code is likely to change much in the future, so it's not a huge concern.

hannes · 2016-03-02T14:40:48Z

Can rewrite to state machine if it helps

hadley · 2016-03-02T14:47:43Z

If you're willing, I'd really appreciate it.

hannes · 2016-03-02T14:48:54Z

Sure, be back in an hour or so.

hannes · 2016-03-02T16:31:50Z

Ok, its now a state machine. Runtime is unchanged.

hadley · 2016-03-02T16:57:35Z

LGTM

krlmlr · 2016-03-02T17:02:27Z

R/interpolate.R

+
+  # prepare comments & quotes for quicker comparisions
+  for(c in seq_along(comments)) {
+    comments[[c]][[1]] <- strsplit(comments[[c]][[1]], "", fixed = TRUE)[[1]]


The [[n]] access is not very intuitive, but fastest:

a <- list(start = "a", end = "b", endRequired = FALSE); microbenchmark::microbenchmark(a[[3]], a[["start"]], a[["end"]], a$s, a$start, a$e, a$end, times = 100000) Unit: nanoseconds expr min lq mean median uq max neval cld a[[3]] 125 142 201.7776 153 167 14234 1e+05 a a[["start"]] 136 156 215.8544 167 184 21100 1e+05 a a[["end"]] 145 164 225.6624 173 191 62662 1e+05 a a$s 186 206 299.5662 218 297 43130 1e+05 ab a$start 151 171 265.3310 183 261 22357 1e+05 ab a$e 188 207 371.8202 219 298 6883402 1e+05 b a$end 167 188 281.5594 201 278 21501 1e+05 ab

I wonder how much this affects overall performance (we're talking nanoseconds here). Let's look at this in a separate PR.

- Use pure R implementation of `sqlParseVariablesImpl()` (#83, @hannesmuehleisen).

krlmlr · 2016-03-02T18:34:34Z

Thanks for your efforts.

hannes · 2016-03-02T18:40:02Z

Whee, thanks for merging!

* New package maintainer: Kirill Müller. * `dbGetInfo()` gains a default method that extracts the information from `dbGetStatement()`, `dbGetRowsAffected()`, `dbHasCompleted()`, and `dbGetRowCount()`. This means that most drivers should no longer need to implement `dbGetInfo()` (which may be deprecated anyway at some point) (#55). * `dbDataType()` and `dbQuoteString()` are now properly exported. * Default `dbGetQuery()` method now always calls `dbFetch()`, in a `tryCatch()` block. * New generic `dbBind()` for binding values to a parameterised query. * DBI gains a number of SQL generation functions. These make it easier to write backends by implementing common operations that are slightly tricky to do absolutely correctly. * `sqlCreateTable()` and `sqlAppendTable()` create tables from a data frame and insert rows into an existing table. These will power most implementations of `dbWriteTable()`. `sqlAppendTable()` is useful for databases that support parameterised queries. * `sqlRownamesToColumn()` and `sqlColumnToRownames()` provide a standard way of translating row names to and from the database. * `sqlInterpolate()` and `sqlParseVariables()` allows databases without native parameterised queries to use parameterised queries to avoid SQL injection attacks. * `sqlData()` is a new generic that converts a data frame into a data frame suitable for sending to the database. This is used to (e.g.) ensure all character vectors are encoded as UTF-8, or to convert R varible types (like factor) to types supported by the database. * The `sqlParseVariablesImpl()` is now implemented purely in R, with full test coverage (#83, @hannesmuehleisen). * `dbiCheckCompliance()` has been removed, the functionality is now available in the `DBItest` package (#80). * Added default `show()` methods for driver, connection and results. * New concrete `ANSIConnection` class and `ANSI()` function to generate a dummy ANSI compliant connection useful for testing. * Default `dbQuoteString()` and `dbQuoteIdentifer()` methods now use `encodeString()` so that special characters like `\n` are correctly escaped. `dbQuoteString()` converts `NA` to (unquoted) NULL. * The initial DBI proposal and DBI version 1 specification are now included as a vignette. These are there mostly for historical interest. * The new `DBItest` package is described in the vignette. * Removed unused `dbi_dep()` and `print.list.names()`.

* New package maintainer: Kirill Müller. * `dbGetInfo()` gains a default method that extracts the information from `dbGetStatement()`, `dbGetRowsAffected()`, `dbHasCompleted()`, and `dbGetRowCount()`. This means that most drivers should no longer need to implement `dbGetInfo()` (which may be deprecated anyway at some point) (#55). * `dbDataType()` and `dbQuoteString()` are now properly exported. * The default implementation for `dbDataType()` (powered by `dbiDataType()`) now also supports `difftime` and `AsIs` objects and lists of `raw` (#70). * Default `dbGetQuery()` method now always calls `dbFetch()`, in a `tryCatch()` block. * New generic `dbBind()` for binding values to a parameterised query. * DBI gains a number of SQL generation functions. These make it easier to write backends by implementing common operations that are slightly tricky to do absolutely correctly. * `sqlCreateTable()` and `sqlAppendTable()` create tables from a data frame and insert rows into an existing table. These will power most implementations of `dbWriteTable()`. `sqlAppendTable()` is useful for databases that support parameterised queries. * `sqlRownamesToColumn()` and `sqlColumnToRownames()` provide a standard way of translating row names to and from the database. * `sqlInterpolate()` and `sqlParseVariables()` allows databases without native parameterised queries to use parameterised queries to avoid SQL injection attacks. * `sqlData()` is a new generic that converts a data frame into a data frame suitable for sending to the database. This is used to (e.g.) ensure all character vectors are encoded as UTF-8, or to convert R varible types (like factor) to types supported by the database. * The `sqlParseVariablesImpl()` is now implemented purely in R, with full test coverage (#83, @hannesmuehleisen). * `dbiCheckCompliance()` has been removed, the functionality is now available in the `DBItest` package (#80). * Added default `show()` methods for driver, connection and results. * New concrete `ANSIConnection` class and `ANSI()` function to generate a dummy ANSI compliant connection useful for testing. * Default `dbQuoteString()` and `dbQuoteIdentifer()` methods now use `encodeString()` so that special characters like `\n` are correctly escaped. `dbQuoteString()` converts `NA` to (unquoted) NULL. * The initial DBI proposal and DBI version 1 specification are now included as a vignette. These are there mostly for historical interest. * The new `DBItest` package is described in the vignette. * Deprecated `print.list.pairs()`. * Removed unused `dbi_dep()`.

jangorecki · 2016-10-19T01:59:32Z

@hannesmuehleisen thanks for this PR! just spotted that I can upgrade DBI and was worried as I was sure DBI will depend on Rcpp based on #40. Great surprise to see it is still lightweight.

hannes added 6 commits February 10, 2016 15:49

zap Rcpp

4513fc2

R implementation of sqlInterpolate

4b97486

fix merge conflict

0f8eb74

compliance.R was removed apparently

6fb3f08

namespace fix

4a4648e

doc fix

f1fdb28

krlmlr reviewed Feb 23, 2016
View reviewed changes

R/interpolate.R

in_quote <- q

break

}

} else {

Copy link

Member

krlmlr Feb 23, 2016

Choose a reason for hiding this comment

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

Indentation looks odd here.

roxygen run

cd59e57

re-indent

368673c

faster implementation

6ecf6a1

krlmlr reviewed Feb 24, 2016
View reviewed changes

hannes added 2 commits February 24, 2016 15:41

TRUE

82b0766

letters

30e0c0c

krlmlr reviewed Feb 24, 2016
View reviewed changes

nitpicking

c6bf8ed

hadley reviewed Mar 2, 2016
View reviewed changes

missing space

5a77230

state machine version

e8c46d3

hannes added 3 commits March 2, 2016 17:33

minor offset change

61cc267

todo has been done

eb935e7

one space is enough

a9b7a33

krlmlr reviewed Mar 2, 2016
View reviewed changes

krlmlr added a commit that referenced this pull request Mar 2, 2016

Merge pull request #83 from hannesmuehleisen/master

ca4e977

- Use pure R implementation of `sqlParseVariablesImpl()` (#83, @hannesmuehleisen).

krlmlr merged commit ca4e977 into r-dbi:master Mar 2, 2016

krlmlr mentioned this pull request Mar 2, 2016

Improve sqlParseVariablesImpl() #86

Closed

2 tasks

krlmlr mentioned this pull request Apr 6, 2017

sqlInterpolate zero-length variable #167

Closed

github-actions bot locked and limited conversation to collaborators Oct 8, 2020

R implementation of sqlParseVariablesImpl #83

R implementation of sqlParseVariablesImpl #83

hannes commented Feb 15, 2016

hannes commented Feb 23, 2016

krlmlr Feb 23, 2016

krlmlr commented Feb 23, 2016

hannes commented Feb 23, 2016

hannes commented Feb 23, 2016

hannes commented Feb 23, 2016

krlmlr commented Feb 23, 2016

hannes commented Feb 23, 2016

hannes commented Feb 23, 2016

hannes commented Feb 23, 2016

hannes commented Feb 24, 2016

krlmlr commented Feb 24, 2016

hannes commented Feb 24, 2016

krlmlr Feb 24, 2016

krlmlr Feb 24, 2016

krlmlr Feb 24, 2016

hannes commented Feb 24, 2016

krlmlr commented Mar 2, 2016

hannes commented Mar 2, 2016

hannes commented Mar 2, 2016

krlmlr commented Mar 2, 2016

hadley Mar 2, 2016

hannes commented Mar 2, 2016

hadley commented Mar 2, 2016

hannes commented Mar 2, 2016

hadley commented Mar 2, 2016

hannes commented Mar 2, 2016

hannes commented Mar 2, 2016

hadley commented Mar 2, 2016

krlmlr Mar 2, 2016

krlmlr commented Mar 2, 2016

hannes commented Mar 2, 2016

jangorecki commented Oct 19, 2016

R implementation of sqlParseVariablesImpl #83

R implementation of sqlParseVariablesImpl #83

Conversation

hannes commented Feb 15, 2016

hannes commented Feb 23, 2016

krlmlr Feb 23, 2016

Choose a reason for hiding this comment

krlmlr commented Feb 23, 2016

hannes commented Feb 23, 2016

hannes commented Feb 23, 2016

hannes commented Feb 23, 2016

krlmlr commented Feb 23, 2016

hannes commented Feb 23, 2016

hannes commented Feb 23, 2016

hannes commented Feb 23, 2016

hannes commented Feb 24, 2016

krlmlr commented Feb 24, 2016

hannes commented Feb 24, 2016

krlmlr Feb 24, 2016

Choose a reason for hiding this comment

krlmlr Feb 24, 2016

Choose a reason for hiding this comment

krlmlr Feb 24, 2016

Choose a reason for hiding this comment

hannes commented Feb 24, 2016

krlmlr commented Mar 2, 2016

hannes commented Mar 2, 2016

hannes commented Mar 2, 2016

krlmlr commented Mar 2, 2016

hadley Mar 2, 2016

Choose a reason for hiding this comment

hannes commented Mar 2, 2016

hadley commented Mar 2, 2016

hannes commented Mar 2, 2016

hadley commented Mar 2, 2016

hannes commented Mar 2, 2016

hannes commented Mar 2, 2016

hadley commented Mar 2, 2016

krlmlr Mar 2, 2016

Choose a reason for hiding this comment

krlmlr commented Mar 2, 2016

hannes commented Mar 2, 2016

jangorecki commented Oct 19, 2016