Skip to content

Conversation

@nealrichardson
Copy link
Member

No description provided.

@github-actions
Copy link

Copy link
Member

@jonkeane jonkeane left a comment

Choose a reason for hiding this comment

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

This looks good — one comment about messaging if we do encounter non-R metadata it would be nice to at least flag that something is getting lost

if (ncol(tab)) {
# Apply any column metadata from the original schema, where appropriate
original_schema <- source_data(.data)$schema
# TODO: do we care about other (non-R) metadata preservation?
Copy link
Member

Choose a reason for hiding this comment

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

Could we detect that it is there and warn/message that we've discarded it? It might be a little bit confusing, but I think that would be preferable to "Arrow lost all my metadata!"

Copy link
Member Author

Choose a reason for hiding this comment

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

We already do discard other metadata when you convert from Arrow to R. Check out the example parquet file in inst/, for example: it has "pandas" metadata. I suspect it would be more surprising/alarming if we started messaging about this metadata all of the sudden.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, Arrow to R conversion losing the metadata sounds like what I expected. TBH I am a little surprised that the following happens silently, since the tab below does have the pandas metadata there. Though that's been in Arrow for a while and if no one has complained, I guess it's not that important.

> tab <- read_parquet(system.file("v0.7.1.parquet", package = "arrow"), as_data_frame = FALSE)
> 
> dplyr::select(tab, carat)$metadata
NULL

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, on 5.0:

new <- tab %>% 
  select(carat) %>% 
  compute()
new$metadata

$pandas
[1] "{\"index_columns\": [\"__index_level_0__\"], \"column_indexes\": [{\"name\":
...

I still don't think this is necessarily meaningful--especially when you're dealing with multiple files with potentially different metadata.

@nealrichardson nealrichardson deleted the exec-plan-metadata branch September 21, 2021 12:43
ViniciusSouzaRoque pushed a commit to s1mbi0se/arrow that referenced this pull request Oct 20, 2021
Closes apache#11183 from nealrichardson/exec-plan-metadata

Authored-by: Neal Richardson <neal.p.richardson@gmail.com>
Signed-off-by: Neal Richardson <neal.p.richardson@gmail.com>
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.

2 participants