arrow icon indicating copy to clipboard operation
arrow copied to clipboard

GH-46410: [C++] Add parquet options to Meson configuration

Open WillAyd opened this issue 7 months ago • 1 comments

Rationale for this change

This continues adding functionality to the Meson configuration

What changes are included in this PR?

This adds the parquet directory to the Meson configuration

Are these changes tested?

Yes

Are there any user-facing changes?

No

  • GitHub Issue: #46410

WillAyd avatar May 30 '25 13:05 WillAyd

:warning: GitHub issue #46410 has been automatically assigned in GitHub to PR creator.

github-actions[bot] avatar May 30 '25 13:05 github-actions[bot]

@kou this one might have gotten missed - mind taking a look when you have time?

WillAyd avatar Jun 27 '25 12:06 WillAyd

Hey @kou just wanted to toss a friendly ping on this. If you have time, please let me know what you think of the latest updates. Thanks!

WillAyd avatar Jul 24 '25 16:07 WillAyd

Looks like the current failures have to do with symbol visibility issues with template instantiations, which I don't believe the CMake configuration is testing. A simple workaround would be to remove the hidden symbol visibility from the Meson configuration, although I'd ideally like to resolve instead

WillAyd avatar Jul 28 '25 14:07 WillAyd

The Alpine Linux failure is related to https://github.com/apache/arrow/issues/47052 not this PR, and I've seen the rhub failure on other PRs as well, so I think unrelated

WillAyd avatar Jul 28 '25 17:07 WillAyd

OK all green here, sans one expected failure.

This became bigger than originally anticipated because I wanted to keep the consistency of the Meson libraries using hidden symbol visibility, but that in turn required some changes to the parquet sources and CMake configuration. If easier for review, I'd be happy to try and split this into two different changes

WillAyd avatar Jul 29 '25 14:07 WillAyd

Hey @kou let me know what you think about this. As mentioned, I am happy to split up some of the symbol fixing issues into a separate PR if that makes things easier

WillAyd avatar Aug 07 '25 13:08 WillAyd

Sorry for not responding this...

Let's split C++/CMake changes to a separated PR for easy to review and track.

kou avatar Aug 08 '25 05:08 kou

OK I've removed any of the symbol changes. Let me know what you think @kou - thanks!

WillAyd avatar Sep 04 '25 19:09 WillAyd

I think all feedback here has been addressed - let me know what you think @kou

WillAyd avatar Sep 10 '25 14:09 WillAyd

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 05e64889fac700eda737e6fb7410382f0ae7219d.

There weren't enough matching historic benchmark results to make a call on whether there were regressions.

The full Conbench report has more details.