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

ARROW-13768: [R] Allow JSON to be an optional component #11046

Closed
wants to merge 3 commits into from

Conversation

karldw
Copy link
Contributor

@karldw karldw commented Sep 1, 2021

I templated from ARROW-11735. Let's see how all the tests go!

@github-actions
Copy link

github-actions bot commented Sep 1, 2021

@github-actions
Copy link

github-actions bot commented Sep 1, 2021

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

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.

I think this looks good, thanks for doing this! @ianmcook since you did the dataset/parquet changes, could you glance over this too?

r/R/arrow-package.R Outdated Show resolved Hide resolved
@karldw
Copy link
Contributor Author

karldw commented Sep 1, 2021

I should add that I'm more confident in my R than my C++ programming. There are almost certainly mistakes here.

The standard checks all have JSON on. Something like test-r-minimal-build might exercise these changes more.

@nealrichardson
Copy link
Member

The minimal build checks ran and look good

r/configure Show resolved Hide resolved
@ianmcook
Copy link
Member

ianmcook commented Sep 1, 2021

Thanks for doing this @karldw!

You've waded into a tricky part of the package that has some dependencies on external systems :) When we add new cflags to the arrow R package build scripts, we also need to open PRs add them in two other repositories:

autobrew/scripts

You can template this on autobrew/scripts#10

conda-forge/r-arrow-feedstock

You can template this on conda-forge/r-arrow-feedstock#30

@karldw
Copy link
Contributor Author

karldw commented Sep 1, 2021

Should there be a change to PKG_LIBS here? I couldn't find a -l.*json flag in use in the code. These other changes include adding e.g. -lparquet or -larrow_dataset

@ianmcook
Copy link
Member

ianmcook commented Sep 1, 2021

Should there be a change to PKG_LIBS here?

I believe not; I don’t think there is any -l flag for that

@nealrichardson
Copy link
Member

we also need to open PRs add them in two other repositories

Good catch, I had this feeling that I was forgetting some other place this would break. We can merge this PR now though right? Just need to make sure those other scripts are updated before the October release (though we can do it sooner than that).

@karldw
Copy link
Contributor Author

karldw commented Sep 2, 2021

The minimal build checks ran and look good

I was looking to test the case where the R side is compiled against a C++ library built with ARROW_JSON=OFF. Did that get run? Sorry if I missed it.

@nealrichardson
Copy link
Member

Check the "R package without arrow" build

@ianmcook
Copy link
Member

ianmcook commented Sep 2, 2021

we also need to open PRs add them in two other repositories

Good catch, I had this feeling that I was forgetting some other place this would break. We can merge this PR now though right? Just need to make sure those other scripts are updated before the October release (though we can do it sooner than that).

Yes, the only other thing that I think needs to be done is to apply the same change made in autobrew/scripts#11 to the file r/tools/autobrew in this repo. @karldw could you do that please? Thank you!

@nealrichardson
Copy link
Member

Should there be a change to PKG_LIBS here?

I believe not; I don’t think there is any -l flag for that

rapidjson is (I believe) vendored on the fly and pulled into libarrow itself; if it were bundled like other third-party dependencies, it would get zipped up into libarrow_bundled_dependencies. So we're good.

@nealrichardson
Copy link
Member

@github-actions crossbow submit test-r-minimal-build

@github-actions
Copy link

github-actions bot commented Sep 2, 2021

Revision: 2355196

Submitted crossbow builds: ursacomputing/crossbow @ actions-810

Task Status
test-r-minimal-build Azure

r/R/arrow-package.R Outdated Show resolved Hide resolved
@nealrichardson
Copy link
Member

Also it looks like there is an example in read_json_arrow that needs the @examplesIf clause updated: https://dev.azure.com/ursacomputing/crossbow/_build/results?buildId=10941&view=logs&j=0da5d1d9-276d-5173-c4c4-9d4d4ed14fdb&t=d9b15392-e4ce-5e4c-0c8c-b69645229181&l=370

You will need the dev version of roxygen2 to update the docs because the released version doesn't have @examplesIf.

Looks like there's also a merge conflict now but fortunately it's on a generated file (arrowExports.cpp) so you can rebase and resolve that conflict however you want, then next time you build and install, the codegen script will run and fix it. (Happy to help if that all gets too messy too.)

@karldw
Copy link
Contributor Author

karldw commented Sep 2, 2021

Also it looks like there is an example in read_json_arrow that needs the @examplesIf clause updated: https://dev.azure.com/ursacomputing/crossbow/_build/results?buildId=10941&view=logs&j=0da5d1d9-276d-5173-c4c4-9d4d4ed14fdb&t=d9b15392-e4ce-5e4c-0c8c-b69645229181&l=370

You will need the dev version of roxygen2 to update the docs because the released version doesn't have @examplesIf.

Ah, thanks! I'm surprised by that, since I thought I had already set @examplesIf arrow_with_json(). Maybe because I hadn't exported arrow_with_json? Let's see how things go now that I'm exporting.

@ianmcook
Copy link
Member

ianmcook commented Sep 2, 2021

@github-actions crossbow submit test-r-minimal-build

@github-actions
Copy link

github-actions bot commented Sep 2, 2021

Revision: 88ff084

Submitted crossbow builds: ursacomputing/crossbow @ actions-811

Task Status
test-r-minimal-build Azure

Copy link
Member

@ianmcook ianmcook left a comment

Choose a reason for hiding this comment

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

Looks great, thanks @karldw! I will merge.

@ianmcook ianmcook closed this in 1440d5a Sep 3, 2021
@karldw karldw deleted the arrow-12981 branch September 3, 2021 00:39
zeroshade pushed a commit to zeroshade/arrow that referenced this pull request Sep 12, 2021
I templated from ARROW-11735. Let's see how all the tests go!

Closes apache#11046 from karldw/arrow-12981

Authored-by: karldw <karldw@users.noreply.github.com>
Signed-off-by: Ian Cook <ianmcook@gmail.com>
ViniciusSouzaRoque pushed a commit to s1mbi0se/arrow that referenced this pull request Oct 20, 2021
I templated from ARROW-11735. Let's see how all the tests go!

Closes apache#11046 from karldw/arrow-12981

Authored-by: karldw <karldw@users.noreply.github.com>
Signed-off-by: Ian Cook <ianmcook@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.

3 participants