Skip to content

Conversation

@jonkeane
Copy link
Member

@jonkeane jonkeane commented Jul 1, 2021

No description provided.

@github-actions
Copy link

github-actions bot commented Jul 1, 2021

r/R/metadata.R Outdated
Copy link
Member

@thisisnic thisisnic Jul 13, 2021

Choose a reason for hiding this comment

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

nit: looks like a typo where you say "this is apply"?

@thisisnic
Copy link
Member

Other than minor thing about typo (or phrasing) LGTM

@jonkeane jonkeane requested a review from nealrichardson July 14, 2021 13:49
Copy link
Member

@nealrichardson nealrichardson 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 notes, though I don't understand this code all that well and defer to you

r/R/metadata.R Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Does this message deserve more explanation? Trying to think of what I would do if I saw that message.

Also:

Suggested change
warning("Row-level metadata has been discarded")
warning("Row-level metadata has been discarded", call. = FALSE)

Copy link
Member

Choose a reason for hiding this comment

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

What happens if I ds %>% select(part) %>% collect(), do I get the warning? I'd think I shouldn't.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, that works without warning (like it should as you say):

> ds %>% select(part) %>% collect()
# A tibble: 4 x 1
   part
* <int>
1     1
2     1
3     2
4     3
> 

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll add a test that asserts that though, it's a quick + easy test

@jonkeane jonkeane force-pushed the ARROW-13189-ds-row-metadata branch from ab7f0d7 to 0569f3e Compare July 15, 2021 18:06
…ed. Also warn/don't save row-level metadata with datasets.
@jonkeane
Copy link
Member Author

Ok, I've made a few changes (including avoiding the now deprecated call_stack()). I've also added this to prevent writing this metadata with datasets. I've expanded the warning messages a bit more. The messages themselves could be longer, though I think it might be better to have the longer explanation in NEWS and we can point to {sfarrow} there for saving sf data.

@jonkeane jonkeane requested a review from nealrichardson July 15, 2021 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants