-
Notifications
You must be signed in to change notification settings - Fork 4k
ARROW-16778: [C++] Fix build/unit test issues in msvc/win32 #13376
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
Conversation
|
Thanks for opening a pull request! If this is not a minor PR. Could you open an issue for this pull request on JIRA? https://issues.apache.org/jira/browse/ARROW Opening JIRAs ahead of time contributes to the Openness of the Apache Arrow project. Then could you also rename pull request title in the following format? or See also: |
|
|
kou
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also add a CI job for building 32bit binaries with Visual C++?
The following patch will work:
diff --git a/.github/workflows/cpp.yml b/.github/workflows/cpp.yml
index 82e99160c3..6eb2213c93 100644
--- a/.github/workflows/cpp.yml
+++ b/.github/workflows/cpp.yml
@@ -192,12 +192,15 @@ jobs:
strategy:
fail-fast: false
matrix:
- os:
- - windows-2019
include:
- os: windows-2019
- name: Windows 2019
+ name: Windows 2019 x64
generator: Visual Studio 16 2019
+ platform: x64
+ - os: windows-2019
+ name: Windows 2019 Win32
+ generator: Visual Studio 16 2019
+ platform: Win32
env:
ARROW_BOOST_USE_SHARED: OFF
ARROW_BUILD_BENCHMARKS: ON
@@ -222,7 +225,7 @@ jobs:
ARROW_WITH_ZLIB: ON
ARROW_WITH_ZSTD: ON
BOOST_SOURCE: BUNDLED
- CMAKE_ARGS: '-A x64 -DOPENSSL_ROOT_DIR=C:\Program Files\OpenSSL-Win64'
+ CMAKE_ARGS: '-A ${{ matrix.platform }} -DOPENSSL_ROOT_DIR=C:\Program Files\OpenSSL-Win64'
CMAKE_GENERATOR: ${{ matrix.generator }}
CMAKE_INSTALL_LIBDIR: bin
CMAKE_INSTALL_PREFIX: /usr
cpp/src/arrow/util/bit_util.h
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure which is better for performance but it seems that we can use bit operations to split uint64_t value to uint32_t values:
return ARROW_POPCOUNT32((bitmap >> 32) & UINT32_MAX) +
ARROW_POPCOUNT32(bitmap & UINT32_MAX);There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If performance is critical, I would assume this version would just add a couple of bit operation. But if you feel it looks cleaner I can change this. Please let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kou updated PR
186c484 to
2994f66
Compare
|
I just found out that I need to follow certain process while submitting fixes to open source projects. I will re-submit these changes soon. |
|
I see that the win32 workflow added here, is now removed from |
|
This wasn't merged. |
No description provided.