You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@orc.apache.org by GitBox <gi...@apache.org> on 2022/11/17 05:49:31 UTC
[GitHub] [orc] wgtmac opened a new pull request, #1317: [C++] Remove pre-C++11 defines
wgtmac opened a new pull request, #1317:
URL: https://github.com/apache/orc/pull/1317
<!--
Thanks for sending a pull request! Here are some tips for you:
1. File a JIRA issue first and use it as a prefix of your PR title, e.g., `ORC-001: Fix ABC`.
2. Use your PR title to summarize what this PR proposes instead of describing the problem.
3. Make PR title and description complete because these will be the permanent commit log.
4. If possible, provide a concise and reproducible example to reproduce the issue for a faster review.
5. If the PR is unfinished, use GitHub PR Draft feature.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If there is a discussion in the mailing list, please add the link.
-->
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] coderex2522 commented on a diff in pull request #1317: ORC-1314: [C++] Remove macros defined before C++11
Posted by GitBox <gi...@apache.org>.
coderex2522 commented on code in PR #1317:
URL: https://github.com/apache/orc/pull/1317#discussion_r1024948137
##########
c++/src/ColumnPrinter.cc:
##########
@@ -319,10 +319,8 @@ namespace orc {
if (hasNulls && !notNull[rowId]) {
writeString(buffer, "null");
} else {
- char numBuffer[64];
- snprintf(numBuffer, sizeof(numBuffer), "%" INT64_FORMAT_STRING "d",
- static_cast<int64_t>(data[rowId]));
- writeString(buffer, numBuffer);
+ const auto numBuffer = std::to_string(static_cast<int64_t>(data[rowId]));
+ writeString(buffer, numBuffer.c_str());
Review Comment:
+1 LGTM. And I suggest that writeString(std::string& file, const char* ptr) function can be optimized as writeString(std::string& file, const std::string& str).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] wgtmac commented on pull request #1317: ORC-1314: [C++] Remove macros defined before C++11
Posted by GitBox <gi...@apache.org>.
wgtmac commented on PR #1317:
URL: https://github.com/apache/orc/pull/1317#issuecomment-1328061507
Update:
- I have upgraded centos7 to use gcc9 thanks to this link: https://stackoverflow.com/questions/67090507/how-to-install-gcc-g-9-on-centos-7-docker-centos7. But I haven't figured out a way to enforce bash to use gcc9 by default instead of the default gcc4. Not sure if we can use centos8 and deprecate this older version. @dongjoon-hyun
- Fixed C++ test failures on several ubuntu and debian images.
Outstanding issues:
- ubuntu18 is still on an old version of cmake. Since ubuntu20 and ubuntu22 are passing, I do not think this is an issue.
- fedora37 and centos7 cannot build due to same error: `make[2]: *** No rule to make target 'c++/libs/thirdparty/googletest_ep-install/lib/libgtest.a', needed by 'c++/test/orc-test'. Stop.`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] wgtmac commented on pull request #1317: ORC-1314: [C++] Remove macros defined before C++11
Posted by GitBox <gi...@apache.org>.
wgtmac commented on PR #1317:
URL: https://github.com/apache/orc/pull/1317#issuecomment-1328181950
I just committed this. Thanks @dongjoon-hyun and @coderex2522!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on pull request #1317: ORC-1314: [C++] Remove macros defined before C++11
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on PR #1317:
URL: https://github.com/apache/orc/pull/1317#issuecomment-1325608114
Thank you for checking. It means the CentOS users need extra steps to upgrade it, right? Is it easy, @wgtmac ?
> We may need fix the docker images before proceeding.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on pull request #1317: ORC-1314: [C++] Remove macros defined before C++11
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on PR #1317:
URL: https://github.com/apache/orc/pull/1317#issuecomment-1325881488
Thank you!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on pull request #1317: ORC-1314: [C++] Remove macros defined before C++11
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on PR #1317:
URL: https://github.com/apache/orc/pull/1317#issuecomment-1328338345
Okay. Thank you. Let me double-check the main branch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on pull request #1317: ORC-1314: [C++] Remove macros defined before C++11
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on PR #1317:
URL: https://github.com/apache/orc/pull/1317#issuecomment-1330106466
Hi, @wgtmac and all.
Unfortunately, the following analysis seems to be a little out-of-sync.
> I can confirm that centos7 (w/ gcc-9) is running into the same issue of fedora37 when building test with google test. This failure exists in main, branch-1.8 and branch-1.7 branches.
Let me add my analysis.
- First, `branch-1.7`'s CentOS7 was tested during Apache ORC 1.7.7 release recently as you see here (#1313). So, this failure doesn't exist in branch-1.7.
- Second, `branch-1.8`'s CentOS7 and `Fedora37` was also tested during Apache ORC 1.8.0 release here (#1107) and the current failure is due to the recent change, #1298. After reverting it, the branch is recovered.
```
Test project /root/build
Start 1: orc-test
1/8 Test #1: orc-test ......................... Passed 6.24 sec
Start 2: java-test
2/8 Test #2: java-test ........................ Passed 213.58 sec
Start 3: java-tools-test
3/8 Test #3: java-tools-test .................. Passed 0.15 sec
Start 4: java-bench-gen-test
4/8 Test #4: java-bench-gen-test .............. Passed 1.54 sec
Start 5: java-bench-scan-test
5/8 Test #5: java-bench-scan-test ............. Passed 1.30 sec
Start 6: java-bench-hive-test
6/8 Test #6: java-bench-hive-test ............. Passed 14.21 sec
Start 7: java-bench-spark-test
7/8 Test #7: java-bench-spark-test ............ Passed 5.14 sec
Start 8: tool-test
8/8 Test #8: tool-test ........................ Passed 11.28 sec
100% tests passed, 0 tests failed out of 8
Total Test time (real) = 253.46 sec
Built target test-out
Finished fedora37 at Mon Nov 28 21:38:15 PST 2022
```
It was my bad to land #1298 to branch-1.8. I'll revert it to recover `branch-1.8` and prepare Apache ORC 1.8.1 vote.
Also, cc @williamhyun .
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] wgtmac merged pull request #1317: ORC-1314: [C++] Remove macros defined before C++11
Posted by GitBox <gi...@apache.org>.
wgtmac merged PR #1317:
URL: https://github.com/apache/orc/pull/1317
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] wgtmac commented on pull request #1317: ORC-1314: [C++] Remove macros defined before C++11
Posted by GitBox <gi...@apache.org>.
wgtmac commented on PR #1317:
URL: https://github.com/apache/orc/pull/1317#issuecomment-1327990851
The centos7 build is passing now thanks to this link: https://stackoverflow.com/questions/67090507/how-to-install-gcc-g-9-on-centos-7-docker-centos7
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] wgtmac commented on pull request #1317: ORC-1314: [C++] Remove macros defined before C++11
Posted by GitBox <gi...@apache.org>.
wgtmac commented on PR #1317:
URL: https://github.com/apache/orc/pull/1317#issuecomment-1321730420
> Does this pass our Docker tests?
>
> ```
> $ cd docker
> $ ./run-all.sh apache main
> ```
Thanks for the review! @dongjoon-hyun
I found that the centos7 docker image is failing with an old gcc version:
```
Cloning into 'orc'...
-- The C compiler identification is GNU 4.8.5
-- The CXX compiler identification is GNU 4.8.5
-- Check for working C compiler: /usr/bin/cc
-- Check for working C compiler: /usr/bin/cc -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
-- Check for working CXX compiler: /usr/bin/c++
-- Check for working CXX compiler: /usr/bin/c++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- No build type selected, default to ReleaseWithDebugInfo
-- compiler GNU version 4.8.5
-- Configuring incomplete, errors occurred!
See also "/root/orc/build/CMakeFiles/CMakeOutput.log".
CMake Error at CMakeLists.txt:134 (message):
A c++17-compliant compiler is required, please use at least GCC 5
```
We may need fix the docker images before proceeding.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] wgtmac commented on pull request #1317: ORC-1314: [C++] Remove macros defined before C++11
Posted by GitBox <gi...@apache.org>.
wgtmac commented on PR #1317:
URL: https://github.com/apache/orc/pull/1317#issuecomment-1328067498
After I digging into fedora37 a little bit more, it seems that I am hitting a gcc bug:
```
[ 34%] Building CXX object c++/src/CMakeFiles/orc.dir/wrap/orc-proto-wrapper.cc.o
In file included from /root/orc/build/c++/libs/thirdparty/protobuf_ep-install/include/google/protobuf/wire_format_lite_inl.h:44,
from /root/orc/build/c++/libs/thirdparty/protobuf_ep-install/include/google/protobuf/map_type_handler.h:35,
from /root/orc/build/c++/libs/thirdparty/protobuf_ep-install/include/google/protobuf/map.h:48,
from /root/orc/build/c++/libs/thirdparty/protobuf_ep-install/include/google/protobuf/generated_message_table_driven.h:34,
from /root/orc/build/c++/src/orc_proto.pb.h:25,
from /root/orc/build/c++/src/orc_proto.pb.cc:4,
from /root/orc/c++/src/wrap/orc-proto-wrapper.cc:45:
In member function 'void google::protobuf::internal::ElementCopier<Element, true>::operator()(Element*, const Element*, int) [with Element = long unsigned int]',
inlined from 'void google::protobuf::RepeatedField<Element>::CopyArray(Element*, const Element*, int) [with Element = long unsigned int]' at /root/orc/build/c++/libs/thirdparty/protobuf_ep-install/include/google/protobuf/repeated_field.h:1428:37,
inlined from 'void google::protobuf::RepeatedField<Element>::MoveArray(Element*, Element*, int) [with Element = long unsigned int]' at /root/orc/build/c++/libs/thirdparty/protobuf_ep-install/include/google/protobuf/repeated_field.h:1422:12,
inlined from 'void google::protobuf::RepeatedField<Element>::Reserve(int) [with Element = long unsigned int]' at /root/orc/build/c++/libs/thirdparty/protobuf_ep-install/include/google/protobuf/repeated_field.h:1403:14:
/root/orc/build/c++/libs/thirdparty/protobuf_ep-install/include/google/protobuf/repeated_field.h:1442:11: error: 'void* memcpy(void*, const void*, size_t)' reading between 8 and 17179869176 bytes from a region of size 0 [-Werror=stringop-overread]
1442 | memcpy(to, from, static_cast<size_t>(array_size) * sizeof(Element));
| ~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In member function 'void google::protobuf::internal::ElementCopier<Element, true>::operator()(Element*, const Element*, int) [with Element = unsigned int]',
inlined from 'void google::protobuf::RepeatedField<Element>::CopyArray(Element*, const Element*, int) [with Element = unsigned int]' at /root/orc/build/c++/libs/thirdparty/protobuf_ep-install/include/google/protobuf/repeated_field.h:1428:37,
inlined from 'void google::protobuf::RepeatedField<Element>::MoveArray(Element*, Element*, int) [with Element = unsigned int]' at /root/orc/build/c++/libs/thirdparty/protobuf_ep-install/include/google/protobuf/repeated_field.h:1422:12,
inlined from 'void google::protobuf::RepeatedField<Element>::Reserve(int) [with Element = unsigned int]' at /root/orc/build/c++/libs/thirdparty/protobuf_ep-install/include/google/protobuf/repeated_field.h:1403:14:
/root/orc/build/c++/libs/thirdparty/protobuf_ep-install/include/google/protobuf/repeated_field.h:1442:11: error: 'void* memcpy(void*, const void*, size_t)' reading between 4 and 8589934588 bytes from a region of size 0 [-Werror=stringop-overread]
1442 | memcpy(to, from, static_cast<size_t>(array_size) * sizeof(Element));
| ~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
make[2]: *** [c++/src/CMakeFiles/orc.dir/build.make:195: c++/src/CMakeFiles/orc.dir/wrap/orc-proto-wrapper.cc.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:415: c++/src/CMakeFiles/orc.dir/all] Error 2
make: *** [Makefile:166: all] Error 2
```
The gcc version is not that old, though
```
sh-5.1# gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/libexec/gcc/aarch64-redhat-linux/12/lto-wrapper
Target: aarch64-redhat-linux
Configured with: ../configure --enable-bootstrap --enable-languages=c,c++,fortran,objc,obj-c++,ada,go,d,lto --prefix=/usr --mandir=/usr/share/man --infodir=/usr/share/info --with-bugurl=http://bugzilla.redhat.com/bugzilla --enable-shared --enable-threads=posix --enable-checking=release --enable-multilib --with-system-zlib --enable-__cxa_atexit --disable-libunwind-exceptions --enable-gnu-unique-object --enable-linker-build-id --with-gcc-major-version-only --enable-libstdcxx-backtrace --with-linker-hash-style=gnu --enable-plugin --enable-initfini-array --with-isl=/builddir/build/BUILD/gcc-12.2.1-20221121/obj-aarch64-redhat-linux/isl-install --enable-gnu-indirect-function --build=aarch64-redhat-linux --with-build-config=bootstrap-lto --enable-link-serialization=1
Thread model: posix
Supported LTO compression algorithms: zlib zstd
gcc version 12.2.1 20221121 (Red Hat 12.2.1-4) (GCC)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] wgtmac commented on pull request #1317: ORC-1314: [C++] Remove macros defined before C++11
Posted by GitBox <gi...@apache.org>.
wgtmac commented on PR #1317:
URL: https://github.com/apache/orc/pull/1317#issuecomment-1325865839
> Thank you for checking. It means the CentOS users need extra steps to upgrade it, right? Is it easy, @wgtmac ? Could you file a JIRA issue for CentOS?
>
> > We may need fix the docker images before proceeding.
Some other images fail as well due to different reasons. I will look into it when I have time. Will update when I have made progress.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] wgtmac commented on pull request #1317: ORC-1314: [C++] Remove macros defined before C++11
Posted by GitBox <gi...@apache.org>.
wgtmac commented on PR #1317:
URL: https://github.com/apache/orc/pull/1317#issuecomment-1325881593
Actually the failure of docker images is not related to my patch, therefore I am thinking of submitting this patch first and then fix those images one by one. WDYT? @dongjoon-hyun
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] wgtmac commented on pull request #1317: ORC-1314: [C++] Remove macros defined before C++11
Posted by GitBox <gi...@apache.org>.
wgtmac commented on PR #1317:
URL: https://github.com/apache/orc/pull/1317#issuecomment-1328179322
I have moved the fix of testing failures on **debian** docker images to a separate PR: https://github.com/apache/orc/pull/1324
I can confirm that **centos7** (w/ gcc-9) is running into the same issue of **fedora37** when building test with google test. This failure exists in **main**, **branch-1.8** and **branch-1.7** branches. Therefore, it does not relate to the current patch (as well as the recent C++17 upgrade & google test upgrade). A separate JIRA has been filed to follow up the remaining issues: https://issues.apache.org/jira/browse/ORC-1320
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org