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

Be explicit about ARROW_ACERO; add tests; remove use_local in migrators #1037

Merged
merged 4 commits into from
May 3, 2023

Conversation

raulcd
Copy link
Member

@raulcd raulcd commented May 2, 2023

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

Based on the Post-release mailing list for ARROW 12.0.0:
https://lists.apache.org/thread/2stklo8dgpgn4dkq2lmvy4483jqcxzgx

Arrow Acero has been split into libarrow_acero for 12.0.0. We should enable it on conda.
See original arrow issue:

Closes #1039

@conda-forge-webservices
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@raulcd
Copy link
Member Author

raulcd commented May 2, 2023

@conda-forge-admin, please rerender

@github-actions
Copy link
Contributor

github-actions bot commented May 2, 2023

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you, but it looks like there was nothing to do.

This message was generated by GitHub actions workflow run https://github.com/conda-forge/arrow-cpp-feedstock/actions/runs/4860351773.

@h-vetinari
Copy link
Member

Interestingly, this got picked up on main already, on all platforms (I checked that we're not missing options that should be on before merging #1034):

--   ARROW_ACERO=ON [default=OFF]
--       Build the Arrow Acero Engine Module

The new .so is packaged already as well; the one thing we haven't done yet is adding a test for this, but AFAICT we don't need to bump the build number.

Something like the below

--- a/recipe/meta.yaml
+++ b/recipe/meta.yaml
@@ -139,8 +139,8 @@ outputs:
     test:
       commands:
         {% set headers = [
-            "arrow/api.h", "arrow/flight/types.h", "arrow/flight/sql/api.h",
-            "gandiva/engine.h", "parquet/api/reader.h"
+            "arrow/api.h", "arrow/acero/groupby.h", "arrow/flight/types.h",
+            "arrow/flight/sql/api.h", "gandiva/engine.h", "parquet/api/reader.h"
         ] %}
         {% for each_header in headers %}
         # headers
@@ -149,8 +149,8 @@ outputs:
         {% endfor %}

         {% set libs = (cuda_compiler_version != "None") * ["arrow_cuda"] + [
-            "arrow", "arrow_dataset", "arrow_flight", "arrow_flight_sql",
-            "arrow_substrait", "gandiva", "parquet"
+            "arrow", "arrow_acero", "arrow_dataset", "arrow_flight",
+            "arrow_flight_sql", "arrow_substrait", "gandiva", "parquet"
         ] %}
         {% for each_lib in libs %}
         # shared

@nealrichardson
Copy link

I believe ARROW_DATASET enables ACERO, so that must be why it's already on. Good to be explicit though.

@raulcd
Copy link
Member Author

raulcd commented May 2, 2023

Interestingly, this got picked up on main already, on all platforms (I checked that we're not missing options that should be on before merging #1034):

--   ARROW_ACERO=ON [default=OFF]
--       Build the Arrow Acero Engine Module

Thanks, it seems that as ARROW_DATASET depends on ARROW_ACERO, this was enabled as dependent:

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

and

      foreach(depended_option_name ${${option_name}_OPTION_DEPENDS})
        set(${depended_option_name} ON)
      endforeach()

Copy link
Member

@h-vetinari h-vetinari left a comment

Choose a reason for hiding this comment

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

Assuming green CI of course.

Thanks for the PR! :)

@h-vetinari
Copy link
Member

I piggy-backed a small change into this PR to fix #1039, so we don't need another separate build for that.

@h-vetinari h-vetinari added the automerge Merge the PR when CI passes label May 2, 2023
@h-vetinari h-vetinari changed the title Enable ARROW_ACERO Be explicit about ARROW_ACERO; add tests; remove use_local in migrators May 3, 2023
@github-actions github-actions bot merged commit 8c63548 into conda-forge:main May 3, 2023
@github-actions
Copy link
Contributor

github-actions bot commented May 3, 2023

Hi! This is the friendly conda-forge automerge bot!

I considered the following status checks when analyzing this PR:

  • linter: passed
  • azure: passed

Thus the PR was passing and merged! Have a great day!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge the PR when CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Drop use_local from migrators
3 participants