Skip to content

GH-34843: [R] Fix R build failed caused by Acero refactor #34844

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.

Sign up for GitHub

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

Merged
assignUser merged 11 commits into from
Apr 5, 2023
Merged

GH-34843: [R] Fix R build failed caused by Acero refactor #34844

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
36d22ce
GH-34843: Fix windows R build
icexelloss Apr 2, 2023
00ac7c3
Try fixing r/configure.win
icexelloss Apr 3, 2023
129f2d9
Try fixing arrow_types.h
icexelloss Apr 3, 2023
c5da945
Try fixing homebrew
icexelloss Apr 3, 2023
8f56065
Try fixing window build
icexelloss Apr 3, 2023
71e8584
Some additional changes (#1)
westonpace Apr 4, 2023
26c73f2
Removed incorrectly added [[acero::export]]
westonpace Apr 4, 2023
946c701
Change acero/dataset linker order since dataset depends on acero
westonpace Apr 4, 2023
8bfa93e
Make sure that Acero doesn't mark symbols with dllimport since that c…
westonpace Apr 4, 2023
0f8daea
One more spot that needed -DARROW_R_WITH_ACERO
westonpace Apr 5, 2023
fdbc370
Removed excess blank line per lint.
westonpace Apr 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions dev/tasks/homebrew-formulae/apache-arrow.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ def install
# link against system libc++ instead of llvm provided libc++
ENV.remove "HOMEBREW_LIBRARY_PATHS", Formula["llvm"].opt_lib
args = %W[
-DARROW_ACERO=ON
Copy link
Member

@kou kou Apr 5, 2023

Choose a reason for hiding this comment

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

Do we need this?
ARROW_DATASET=ON must enable ARROW_ACERO automatically:

arrow/cpp/cmake_modules/DefineOptions.cmake

Lines 308 to 313 in ee42b90

define_option(ARROW_DATASET
"Build the Arrow Dataset Modules"
OFF
DEPENDS
ARROW_ACERO
ARROW_FILESYSTEM)

Copy link
Member

@westonpace westonpace Apr 5, 2023

Choose a reason for hiding this comment

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

I'm not sure what the guidelines are. R uses Acero directly, in addition to transitively through datasets. So if R decided to stop using datasets then it would still need the acero dependency. However, if we want to leave it out so things are concise then it should work. I am fine either way.

Copy link
Member

@kou kou Apr 5, 2023

Choose a reason for hiding this comment

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

I see. Thanks.

For dev/tasks/homebrew-formulae/apache-arrow.rb, this is a file for Homebrew's CI. (We should sync with https://github.com/Homebrew/homebrew-core/blob/master/Formula/apache-arrow.rb .) This is not only for R. In general, Homebrew enables all features as much as possible. So I think that we don't need to assume that we may disable only ARROW_DATASET in this file. (We should omit redundant -DARROW_ACERO=ON.)

For dev/tasks/homebrew-formulae/autobrew/, they are only for R. So I think that your point is valid.

-DARROW_COMPUTE=ON
-DARROW_CSV=ON
-DARROW_DATASET=ON
Expand Down
1 change: 1 addition & 0 deletions dev/tasks/homebrew-formulae/autobrew/apache-arrow-static.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ def install
args = %W[
-DARROW_BUILD_SHARED=OFF
-DARROW_BUILD_UTILITIES=ON
-DARROW_ACERO=ON
Copy link
Member

@kou kou Apr 5, 2023

Choose a reason for hiding this comment

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

ditto.

-DARROW_COMPUTE=ON
-DARROW_CSV=ON
-DARROW_DATASET=ON
Expand Down
1 change: 1 addition & 0 deletions dev/tasks/homebrew-formulae/autobrew/apache-arrow.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ def install
args = %W[
-DARROW_BUILD_SHARED=OFF
-DARROW_BUILD_UTILITIES=ON
-DARROW_ACERO=ON
Copy link
Member

@kou kou Apr 5, 2023

Choose a reason for hiding this comment

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

ditto.

-DARROW_COMPUTE=ON
-DARROW_CSV=ON
-DARROW_CXXFLAGS="-D_LIBCPP_DISABLE_AVAILABILITY"
Expand Down
1 change: 1 addition & 0 deletions r/NAMESPACE
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,7 @@ export(all_of)
export(arrow_available)
export(arrow_info)
export(arrow_table)
export(arrow_with_acero)
export(arrow_with_dataset)
export(arrow_with_gcs)
export(arrow_with_json)
Expand Down
8 changes: 8 additions & 0 deletions r/R/arrow-info.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,14 @@ arrow_available <- function() {
TRUE
}

#' @rdname arrow_info
#' @export
arrow_with_acero <- function() {
tryCatch(.Call(`_acero_available`), error = function(e) {
return(FALSE)
})
}

#' @rdname arrow_info
#' @export
arrow_with_dataset <- function() {
Expand Down
2 changes: 1 addition & 1 deletion r/R/type.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,7 @@ NestedType <- R6Class("NestedType", inherit = DataType)
#' @return An Arrow type object inheriting from [DataType].
#' @export
#' @seealso [dictionary()] for creating a dictionary (factor-like) type.
#' @examples
#' @examplesIf arrow_with_acero()
#' bool()
#' struct(a = int32(), b = double())
#' timestamp("ms", timezone = "CEST")
Expand Down
12 changes: 6 additions & 6 deletions r/configure
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,18 +265,18 @@ if [ $? -eq 0 ]; then
# NOTE: parquet is assumed to have the same -L flag as arrow
# so there is no need to add its location to PKG_DIRS
fi
if arrow_built_with ARROW_ACERO; then
PKG_CFLAGS="$PKG_CFLAGS -DARROW_R_WITH_ACERO"
PKG_LIBS="-larrow_acero $PKG_LIBS"
# NOTE: arrow-acero is assumed to have the same -L flag as arrow
# so there is no need to add its location to PKG_DIRS
fi
if arrow_built_with ARROW_DATASET; then
PKG_CFLAGS="$PKG_CFLAGS -DARROW_R_WITH_DATASET"
PKG_LIBS="-larrow_dataset $PKG_LIBS"
# NOTE: arrow-dataset is assumed to have the same -L flag as arrow
# so there is no need to add its location to PKG_DIRS
fi
if arrow_built_with ARROW_ACERO; then
PKG_CFLAGS="$PKG_CFLAGS -DARROW_R_WITH_ACERO"
PKG_LIBS="-larrow_acero $PKG_LIBS"
# NOTE: arrow-acero is assumed to have the same -L flag as arrow
# so there is no need to add its location to PKG_DIRS
fi
if arrow_built_with ARROW_SUBSTRAIT; then
PKG_CFLAGS="$PKG_CFLAGS -DARROW_R_WITH_SUBSTRAIT"
PKG_LIBS="-larrow_substrait $PKG_LIBS"
Expand Down
11 changes: 8 additions & 3 deletions r/configure.win
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,11 +59,11 @@ function configure_release() {
# NOTE: If you make changes to the libraries below, you should also change
# ci/scripts/r_windows_build.sh and ci/scripts/PKGBUILD
PKG_CFLAGS="-I${RWINLIB}/include -DARROW_STATIC -DPARQUET_STATIC -DARROW_DS_STATIC \
-DARROW_R_WITH_PARQUET -DARROW_R_WITH_DATASET \
-DARROW_R_WITH_JSON"
-DARROW_ACERO_STATIC -DARROW_R_WITH_PARQUET -DARROW_R_WITH_ACERO \
-DARROW_R_WITH_DATASET -DARROW_R_WITH_JSON"
PKG_LIBS="-L${RWINLIB}/lib"'$(subst gcc,,$(COMPILED_BY))$(R_ARCH) '
PKG_LIBS="$PKG_LIBS -L${RWINLIB}/lib"'$(R_ARCH)$(CRT) '
PKG_LIBS="$PKG_LIBS -larrow_dataset -lparquet -larrow -larrow_bundled_dependencies \
PKG_LIBS="$PKG_LIBS -larrow_dataset -larrow_acero -lparquet -larrow -larrow_bundled_dependencies \
-lutf8proc -lthrift -lsnappy -lz -lzstd -llz4 -lbz2 ${BROTLI_LIBS} -lole32 \
${MIMALLOC_LIBS} ${OPENSSL_LIBS}"

Expand Down Expand Up @@ -98,6 +98,11 @@ function configure_dev() {
PKG_CONFIG_PACKAGES="$PKG_CONFIG_PACKAGES parquet"
fi

if [ $(cmake_option ARROW_ACERO) -eq 1 ]; then
PKG_CFLAGS="$PKG_CFLAGS -DARROW_R_WITH_ACERO"
PKG_CONFIG_PACKAGES="$PKG_CONFIG_PACKAGES arrow-acero"
fi

if [ $(cmake_option ARROW_DATASET) -eq 1 ]; then
PKG_CFLAGS="$PKG_CFLAGS -DARROW_R_WITH_DATASET"
PKG_CONFIG_PACKAGES="$PKG_CONFIG_PACKAGES arrow-dataset"
Expand Down
2 changes: 1 addition & 1 deletion r/data-raw/codegen.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
# Ensure that all machines are sorting the same way
invisible(Sys.setlocale("LC_COLLATE", "C"))

features <- c("dataset", "substrait", "parquet", "s3", "gcs", "json")
features <- c("acero", "dataset", "substrait", "parquet", "s3", "gcs", "json")

suppressPackageStartupMessages({
library(decor)
Expand Down
1 change: 1 addition & 0 deletions r/inst/build_arrow_static.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ ${CMAKE} -DARROW_BOOST_USE_SHARED=OFF \
-DARROW_BUILD_TESTS=OFF \
-DARROW_BUILD_SHARED=OFF \
-DARROW_BUILD_STATIC=ON \
-DARROW_ACERO=${ARROW_ACERO:-$ARROW_DEFAULT_PARAM} \
-DARROW_COMPUTE=ON \
-DARROW_CSV=ON \
-DARROW_DATASET=${ARROW_DATASET:-ON} \
Expand Down
3 changes: 3 additions & 0 deletions r/man/arrow_info.Rd
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions r/man/data-type.Rd
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions r/src/array.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

#include <arrow/array.h>
#include <arrow/array/concatenate.h>
#include <arrow/record_batch.h>
#include <arrow/util/bitmap_reader.h>
#include <arrow/util/byte_size.h>

Expand Down
Loading