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

GH-35952: [R] Ensure that schema metadata can actually be set as a named character vector #35954

Merged
merged 2 commits into from
Jun 6, 2023

Conversation

paleolimbot
Copy link
Member

@paleolimbot paleolimbot commented Jun 6, 2023

This wasn't necessarily a regression (reprex fails in 12.0.0 as well), although the comments suggest that assigning a named character vector will work when assigning schema metadata and this feature appears to be used by at least one of our dependencies (sfarrow). Given that the sfarrow check passes on 12.0.0, there is possibly also a place in our code that returns a named character vector rather than a list. I've confirmed that this fix solves the reverse dependency failure building the sfarrow example vignette.

library(arrow, warn.conflicts = FALSE)

schema <- schema(x = int32())
schema$metadata <- c("name" = "value")
schema$metadata
#> $name
#> [1] "value"

Created on 2023-06-06 with reprex v2.0.2

@paleolimbot paleolimbot requested a review from thisisnic as a code owner June 6, 2023 16:38
@github-actions
Copy link

github-actions bot commented Jun 6, 2023

⚠️ GitHub issue #35952 has been automatically assigned in GitHub to PR creator.

@github-actions github-actions bot added Component: R awaiting committer review Awaiting committer review labels Jun 6, 2023
Copy link
Member

@thisisnic thisisnic left a comment

Choose a reason for hiding this comment

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

Fantastic, thanks!

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels Jun 6, 2023
@thisisnic thisisnic merged commit fe6c067 into apache:main Jun 6, 2023
thisisnic pushed a commit to thisisnic/arrow that referenced this pull request Jun 6, 2023
…s a named character vector (apache#35954)

This wasn't necessarily a regression (reprex fails in 12.0.0 as well), although the comments suggest that assigning a named character vector will work when assigning schema metadata and this feature appears to be used by at least one of our dependencies (sfarrow). Given that the sfarrow check passes on 12.0.0, there is possibly also a place in our code that returns a named character vector rather than a list. I've confirmed that this fix solves the reverse dependency failure building the sfarrow example vignette.

``` r
library(arrow, warn.conflicts = FALSE)

schema <- schema(x = int32())
schema$metadata <- c("name" = "value")
schema$metadata
#> $name
#> [1] "value"
```

<sup>Created on 2023-06-06 with [reprex v2.0.2](https://reprex.tidyverse.org)</sup>
* Closes: apache#35952

Authored-by: Dewey Dunnington <[email protected]>
Signed-off-by: Nic Crane <[email protected]>
@ursabot
Copy link

ursabot commented Jun 7, 2023

Benchmark runs are scheduled for baseline = e7a9b29 and contender = fe6c067. fe6c067 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.47% ⬆️0.09%] test-mac-arm
[Finished ⬇️2.61% ⬆️0.0%] ursa-i9-9960x
[Failed ⬇️0.39% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] fe6c067e ec2-t3-xlarge-us-east-2
[Failed] fe6c067e test-mac-arm
[Finished] fe6c067e ursa-i9-9960x
[Finished] fe6c067e ursa-thinkcentre-m75q
[Finished] e7a9b29b ec2-t3-xlarge-us-east-2
[Finished] e7a9b29b test-mac-arm
[Finished] e7a9b29b ursa-i9-9960x
[Failed] e7a9b29b ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@ursabot
Copy link

ursabot commented Jun 7, 2023

['Python', 'R'] benchmarks have high level of regressions.
ursa-i9-9960x

@paleolimbot paleolimbot deleted the r-key-value-metadata-fix branch June 12, 2023 12:58
thisisnic pushed a commit to thisisnic/arrow that referenced this pull request Jun 13, 2023
…s a named character vector (apache#35954)

This wasn't necessarily a regression (reprex fails in 12.0.0 as well), although the comments suggest that assigning a named character vector will work when assigning schema metadata and this feature appears to be used by at least one of our dependencies (sfarrow). Given that the sfarrow check passes on 12.0.0, there is possibly also a place in our code that returns a named character vector rather than a list. I've confirmed that this fix solves the reverse dependency failure building the sfarrow example vignette.

``` r
library(arrow, warn.conflicts = FALSE)

schema <- schema(x = int32())
schema$metadata <- c("name" = "value")
schema$metadata
#> $name
#> [1] "value"
```

<sup>Created on 2023-06-06 with [reprex v2.0.2](https://reprex.tidyverse.org)</sup>
* Closes: apache#35952

Authored-by: Dewey Dunnington <[email protected]>
Signed-off-by: Nic Crane <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[R] Ensure that schema metadata can actually be set as a named character vectory
3 participants