ORC-1314: [C++] Remove macros defined before C++11 #1317

wgtmac · 2022-11-17T05:49:29Z

What changes were proposed in this pull request?

Remove any unnecessary macros defined in the old days to work around missing support of C++11.

Why are the changes needed?

We have officially supported C++ 17, which means these macros are useless.

How was this patch tested?

No new tests were added. Make sure all CI checks pass.
In addition, this patch also fixes all the complier warnings from g++ on ubuntu.

coderex2522 · 2022-11-17T09:30:41Z

c++/src/ColumnPrinter.cc

-               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());


+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).

dongjoon-hyun

Does this pass our Docker tests?

$ cd docker
$ ./run-all.sh apache main

dongjoon-hyun

+1, LGTM (if this passes Docker tests), @wgtmac .

Also, cc @williamhyun

wgtmac · 2022-11-21T09:08:21Z

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.

dongjoon-hyun · 2022-11-23T20:13:46Z

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.

wgtmac · 2022-11-24T02:22:37Z

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.

dongjoon-hyun · 2022-11-24T02:53:03Z

Thank you!

wgtmac · 2022-11-26T15:02:35Z

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.

wgtmac · 2022-11-26T15:38:23Z

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)

wgtmac · 2022-11-27T06:11:36Z

I have moved the fix of testing failures on debian docker images to a separate PR: #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

wgtmac · 2022-11-27T06:37:15Z

I just committed this. Thanks @dongjoon-hyun and @coderex2522!

dongjoon-hyun · 2022-11-27T20:58:46Z

Okay. Thank you. Let me double-check the main branch.

dongjoon-hyun · 2022-11-29T05:38:59Z

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 (Release Apache ORC 1.7.7 #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 (Release Apache ORC 1.8.0 #1107) and the current failure is due to the recent change, ORC-1303: [C++] Upgrade GoogleTest to 1.12.1 #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 closes apache#1317

github-actions bot added BUILD CPP labels Nov 17, 2022

wgtmac changed the title ~~[C++] Remove pre-C++11 defines~~ ORC-1314: [C++] Remove macros defined before C++11 Nov 17, 2022

coderex2522 reviewed Nov 17, 2022

View reviewed changes

dongjoon-hyun reviewed Nov 21, 2022

View reviewed changes

dongjoon-hyun approved these changes Nov 21, 2022

View reviewed changes

[C++] Remove prev-c++11 defines

6ca98b4

wgtmac force-pushed the remove_prev_cpp11 branch from 57108b5 to 6ca98b4 Compare November 25, 2022 14:53

fix centos7 to install gcc9

94358d9

github-actions bot added the INFRA label Nov 26, 2022

wgtmac added 3 commits November 26, 2022 20:56

fix unsigned char conversion

0802d96

fix unsigned char conversion

c3eb8c5

fix unsigned char ut failure

4368459

wgtmac added 3 commits November 27, 2022 00:29

fix fedora37 build

0af43e9

revert unsigned fix to a separate patch

4046a2b

move centos7 dockerfile fix to a separate patch

51f8752

coderex2522 approved these changes Nov 27, 2022

View reviewed changes

wgtmac merged commit e0c8d1b into apache:main Nov 27, 2022

dongjoon-hyun mentioned this pull request Nov 29, 2022

ORC-1322: Upgrade centos7 docker image to use gcc 9 #1325

Merged

cxzl25 pushed a commit to cxzl25/orc that referenced this pull request Jan 11, 2024

ORC-1314: [C++] Remove macros defined before C++11

fabb3c7

This closes apache#1317

wgtmac deleted the remove_prev_cpp11 branch February 27, 2024 02:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ORC-1314: [C++] Remove macros defined before C++11 #1317

ORC-1314: [C++] Remove macros defined before C++11 #1317

wgtmac commented Nov 17, 2022 •

edited

coderex2522 Nov 17, 2022

dongjoon-hyun left a comment

dongjoon-hyun left a comment

wgtmac commented Nov 21, 2022

dongjoon-hyun commented Nov 23, 2022 •

edited

wgtmac commented Nov 24, 2022

dongjoon-hyun commented Nov 24, 2022

wgtmac commented Nov 26, 2022

wgtmac commented Nov 26, 2022

wgtmac commented Nov 27, 2022

wgtmac commented Nov 27, 2022

dongjoon-hyun commented Nov 27, 2022

dongjoon-hyun commented Nov 29, 2022

ORC-1314: [C++] Remove macros defined before C++11 #1317

ORC-1314: [C++] Remove macros defined before C++11 #1317

Conversation

wgtmac commented Nov 17, 2022 • edited

What changes were proposed in this pull request?

Why are the changes needed?

How was this patch tested?

coderex2522 Nov 17, 2022

Choose a reason for hiding this comment

dongjoon-hyun left a comment

Choose a reason for hiding this comment

dongjoon-hyun left a comment

Choose a reason for hiding this comment

wgtmac commented Nov 21, 2022

dongjoon-hyun commented Nov 23, 2022 • edited

wgtmac commented Nov 24, 2022

dongjoon-hyun commented Nov 24, 2022

wgtmac commented Nov 26, 2022

wgtmac commented Nov 26, 2022

wgtmac commented Nov 27, 2022

wgtmac commented Nov 27, 2022

dongjoon-hyun commented Nov 27, 2022

dongjoon-hyun commented Nov 29, 2022

wgtmac commented Nov 17, 2022 •

edited

dongjoon-hyun commented Nov 23, 2022 •

edited