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.

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

Docs: edits to copy and modify vignettes #1098

Merged
merged 30 commits into from Jul 6, 2022
Merged

Docs: edits to copy and modify vignettes #1098

merged 30 commits into from Jul 6, 2022

Conversation

IndrajeetPatil
Copy link
Contributor

No description provided.

Copy link
Collaborator

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

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

Thanks, looks good!

vignettes/howto-dm-copy.Rmd Outdated Show resolved Hide resolved
vignettes/howto-dm-copy.Rmd Outdated Show resolved Hide resolved
vignettes/howto-dm-rows.Rmd Outdated Show resolved Hide resolved
vignettes/howto-dm-rows.Rmd Outdated Show resolved Hide resolved
vignettes/howto-dm-rows.Rmd Outdated Show resolved Hide resolved
vignettes/howto-dm-rows.Rmd Outdated Show resolved Hide resolved
vignettes/howto-dm-rows.Rmd Outdated Show resolved Hide resolved
@@ -138,7 +141,7 @@ dm_insert_out <-
dm_rows_insert(dm_insert_in)
``````

This gives us a warning that changes will not be persisted.
This gives us a warning that changes will not persist (i.e., they are temporary).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this still true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like it:

library(DBI)
library(dm)
library(tidyverse)

parent <- tibble(value = c("A", "B", "C"), pk = 1:3)
child <- tibble(value = c("a", "b", "c"), pk = 1:3, fk = c(1, 1, NA))
demo_dm <-
  dm(parent = parent, child = child) %>%
  dm_add_pk(parent, pk) %>%
  dm_add_pk(child, pk) %>%
  dm_add_fk(child, fk, parent)

sqlite_db <- dbConnect(RSQLite::SQLite())
demo_sql <- copy_dm_to(sqlite_db, demo_dm, temporary = FALSE)

new_parent <- tibble(value = "D", pk = 4)
new_child <- tibble(value = "d", pk = 4, fk = 4)

dm_insert_in <-
  dm(parent = new_parent, child = new_child) %>%
  copy_dm_to(sqlite_db, ., temporary = TRUE)

dm_insert_out <-
  demo_sql %>%
  dm_rows_insert(dm_insert_in)
#> Not persisting, use `in_place = FALSE` to turn off this message.

dbDisconnect(sqlite_db)

Created on 2022-06-19 by the reprex package (v2.0.1.9000)

@IndrajeetPatil
Copy link
Contributor Author

@krlmlr This should be ready for merge. Let me know if you see any new issues.

@krlmlr krlmlr requested a review from maelle June 20, 2022 18:08
Copy link
Member

@maelle maelle left a comment

Choose a reason for hiding this comment

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

A few comments. Thank you!

vignettes/howto-dm-copy.Rmd Outdated Show resolved Hide resolved
vignettes/howto-dm-rows.Rmd Outdated Show resolved Hide resolved
vignettes/howto-dm-rows.Rmd Show resolved Hide resolved
vignettes/setup/setup.R Outdated Show resolved Hide resolved
vignettes/setup/setup.R Outdated Show resolved Hide resolved
@IndrajeetPatil
Copy link
Contributor Author

I think there are no outstanding review comments for this PR now.

Copy link
Collaborator

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

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

Thanks, should we do another round?

vignettes/howto-dm-rows.Rmd Outdated Show resolved Hide resolved
@@ -18,25 +18,25 @@ source("setup/setup.R")


This tutorial introduces the methods {dm} provides for modifying the data in the tables of a relational model.
There are 6 methods:
There are 5 methods:

* [`dm_rows_insert()`](#insert) - adds new rows
Copy link
Collaborator

Choose a reason for hiding this comment

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

dm_rows_append()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should all instances of dm_rows_insert() be replaced by dm_rows_append()?
Or should the vignette mention both of these options?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We have both, but dm_rows_insert() seems to be much less useful than dm_rows_append() .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, in that case, I think we should only mention dm_rows_append()?

The fewer functions the user needs to learn about the better, I think, especially if two functions are doing the same thing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

They are different, though -- dm_rows_insert() will ditch duplicates or give an error, in some cases this is what we want. We can take this on in a separate PR though.

vignettes/howto-dm-rows.Rmd Show resolved Hide resolved
vignettes/setup/setup.R Show resolved Hide resolved
Copy link
Collaborator

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

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

Thanks, missed that. I'll review with dm_rows_append() added.

@@ -18,25 +18,25 @@ source("setup/setup.R")


This tutorial introduces the methods {dm} provides for modifying the data in the tables of a relational model.
There are 6 methods:
There are 5 methods:

* [`dm_rows_insert()`](#insert) - adds new rows
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have both, but dm_rows_insert() seems to be much less useful than dm_rows_append() .

When called on a whole dm object (without zoom), `compute()` materializes all tables into new (temporary or persistent) tables by executing the associated SQL query and storing the full results.
Depending on the size of your data this may take considerable time or be infeasible.
When called on a whole `dm` object (without zoom), `compute()` materializes all tables into new (temporary or persistent) tables by executing the associated SQL query and storing the full results.
Depending on the size of your data, this may take considerable time or may even be unfeasible.
Copy link
Collaborator

Choose a reason for hiding this comment

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

@krlmlr krlmlr merged commit 9d83057 into cynkra:main Jul 6, 2022
@krlmlr
Copy link
Collaborator

krlmlr commented Jul 6, 2022

Thanks!

@IndrajeetPatil IndrajeetPatil deleted the edits_dm_copy_and_update branch July 6, 2022 05:16
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants