Autoincrement 2

[wibeasley]

(Hi @krlmlr, this PR replaces #58)

Can someone confirm that this is the intended behavior? I'd hate to be writing tests that approve an unintended effect.

When the autoincrement column isn't in the data.frame sent to the database, it's populated by SQLite.

But when it is populated before getting to the database, SQLite won't assign/update values. And when retrieved from SQLite, the data.frame will be re-ordered (corresponding to the values in the (ignored) autoincrement column).

> ds_local
    id name score
1  126    a     1
2  125    b     2
3  124    c     3
...
24 103    x    24
25 102    y    25
26 101    z    26

> dbWriteTable(con, name = 'tbl', value = ds_local, append = TRUE, row.names = FALSE)
> ds_remote <- dbReadTable(con, "tbl")

> ds_remote
    id name score
1  101    z    26
2  102    y    25
3  103    x    24
...
24 124    c     3
25 125    b     2
26 126    a     1

[krlmlr]

SQL tables don't have an intrinsic order unless you supply ORDER BY with the query (which dbReadTable() doesn't). So the SQL engine is free to do whatever it wants to in this case.

I would expect the following behavior:

• If the autoincrement column doesn't exist in the data frame passed to dbWriteTable(), it's populated by the SQL engine
• If it exists, the values are taken from the data; NA values should be converted to explicit NULL in this case, and it's up to the SQL engine to decide if further conversion happens there

Does this align with your observations?

[wibeasley]

@krlmlr, yes, that is consistent. And to be specific on the second bullet, the engine apparently decides to autopopulate the column of NAs (which makes sense to me).

>   ds_local <- data.frame(
+     #Notice the 'id' column is set locally, which overrides the autoincrement assignment in the DB.
+     id               = NA_integer_,
+     name             = letters,
+     score            = 1:26,
+     stringsAsFactors = FALSE
+   )

>   dbWriteTable(con, name = 'tbl', value = ds_local, append = TRUE, row.names = FALSE)
[1] TRUE
>   
>   ds_remote <- dbReadTable(con, "tbl")
>   ds_remote
   id name score
1   1    a     1
2   2    b     2
3   3    c     3
...
24 24    x    24
25 25    y    25
26 26    z    26

[krlmlr]

Changes minus roxygen noise: 7997aca...df4d55d

[krlmlr]

Does this also apply for append = TRUE?

[krlmlr]

You don't need to have the column in the data frame if the table already exists. Your text seems to imply otherwise.

[wibeasley]

@krlmlr

• I rewrote some of the tests so labels aren't necessary. I kept the labels in other places where the intent is less clear without it (and I couldn't think of a way to rewrite it).
• Datasets are now sorted before the tests (in response to https://github.com/rstats-db/RSQLite/pull/119/files#r44763094)
• Two more scenarios are tested (where the id has some values and some NAs). As before, I think everything is working as people intended. The new tests are included more for the sake of detecting future regressions.

Question:
In response to this question of yours, do you want the append=TRUE and append=FALSE comparisons to be within the same test/function? In other words, should the function "autoincrement correctly populated by database" address both scenarios? Or should they have separate set ups?

I'd favor separation/isolation, even though it would increase the amount of code. But I wanted to check with you guys first, since you guys tend to have strong opinions about these things.

[krlmlr]

"in reverse order": There's no intrinsic order. Same above.

[krlmlr]

Very thorough tests, thanks! Isolated tests are fine with me.

Could you please also add a bullet to NEWS.md?

[wibeasley]

• "in reverse order": There's no intrinsic order. Same above.

  That test description was intended to communicate that the id assignment in the local dataset was (approximately) reverse ordered ...and thus exposed the potential for a sequential-assigning-database to duplicate previous values. I see your point, and will try to think of a less ambiguous phrase. In the meantime, I'm open to suggestions if one has already occurred to you.

  c(NA_integer_, 306, NA_integer_, -204, 103, 102, NA_integer_)

• You don't need to have the column in the data frame if the table already exists. Your text seems to imply otherwise.

  Agreed. My comfort and understanding of this functionality is a lot different than it was a year ago when I wrote the description. I'll update the documentation after I write the append=FALSE tests. I may learn something that informs the documentation and makes it clearer.

• Could you please also add a bullet to NEWS.md?

  Sure, I'd forgotten about that. Thanks for being so polite while making good suggestions.

[wibeasley]

@krlmlr, I believe I've made all the modifications you've requested. Please tell me if there's something else I can do.

I didn't add any tests for append=FALSE in this PR because it's not possible unless overwrite is true, and I assume at autoincrement DDL is irrelevant when overwriting. I have started a batch of overwrite tests, but I think those are different enough to justify a different PR.

[krlmlr]

Thanks, looks good to me. I think overwrite = TRUE tests are generic enough for the new DBItest package (r-dbi/DBItest#12).

@hadley: Can we merge this? See changes minus roxygen noise: 7997aca...df4d55d

[krlmlr]

Thanks!