You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Joe McDonnell (Code Review)" <ge...@cloudera.org> on 2022/12/20 18:40:15 UTC
[Impala-ASF-CR] IMPALA-11713: Switch to C++17
Joe McDonnell has uploaded this change for review. ( http://gerrit.cloudera.org:8080/19183
Change subject: IMPALA-11713: Switch to C++17
......................................................................
IMPALA-11713: Switch to C++17
This switches from C++14 to C++17, which allows Impala
code to interact with libraries written in C++17.
This fixes a few minor compilation differences, such
as the need for comparators to be const. Since C++17
includes -faligned-new, this removes the CMake code
that specified it.
Testing:
- Ran core job
Change-Id: Iadac41817fe5eaaa469a5f0e9f94056a409c14b9
---
M be/CMakeLists.txt
M be/src/exec/file-metadata-utils.cc
M be/src/gutil/endian.h
M be/src/runtime/krpc-data-stream-mgr.h
M be/src/service/impala-server.h
M be/src/udf_samples/CMakeLists.txt
6 files changed, 8 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/83/19183/4
--
To view, visit http://gerrit.cloudera.org:8080/19183
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iadac41817fe5eaaa469a5f0e9f94056a409c14b9
Gerrit-Change-Number: 19183
Gerrit-PatchSet: 4
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-11713: Switch to C++17
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/19183 )
Change subject: IMPALA-11713: Switch to C++17
......................................................................
Patch Set 5:
Rebased, will do a performance run
--
To view, visit http://gerrit.cloudera.org:8080/19183
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iadac41817fe5eaaa469a5f0e9f94056a409c14b9
Gerrit-Change-Number: 19183
Gerrit-PatchSet: 5
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Thu, 16 Feb 2023 01:29:46 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11713: Switch to C++17
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19183 )
Change subject: IMPALA-11713: Switch to C++17
......................................................................
Patch Set 6:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/9066/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/19183
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iadac41817fe5eaaa469a5f0e9f94056a409c14b9
Gerrit-Change-Number: 19183
Gerrit-PatchSet: 6
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Feb 2023 22:20:40 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11713: Switch to C++17
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/19183 )
Change subject: IMPALA-11713: Switch to C++17
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/19183
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iadac41817fe5eaaa469a5f0e9f94056a409c14b9
Gerrit-Change-Number: 19183
Gerrit-PatchSet: 5
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Feb 2023 22:19:04 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11713: Switch to C++17
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/19183 )
Change subject: IMPALA-11713: Switch to C++17
......................................................................
Patch Set 5:
Neither of the performance runs seem to make a big difference:
https://jenkins.impala.io/job/perf-AB-test/394/artifact/Impala/perf_results/latest/performance_result.txt
https://jenkins.impala.io/job/perf-AB-test/393/artifact/Impala/perf_results/latest/performance_result.txt
--
To view, visit http://gerrit.cloudera.org:8080/19183
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iadac41817fe5eaaa469a5f0e9f94056a409c14b9
Gerrit-Change-Number: 19183
Gerrit-PatchSet: 5
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Feb 2023 22:17:52 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11713: Switch to C++17
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19183 )
Change subject: IMPALA-11713: Switch to C++17
......................................................................
Patch Set 6: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/19183
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iadac41817fe5eaaa469a5f0e9f94056a409c14b9
Gerrit-Change-Number: 19183
Gerrit-PatchSet: 6
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Feb 2023 22:20:39 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11713: Switch to C++17
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/19183 )
Change subject: IMPALA-11713: Switch to C++17
......................................................................
IMPALA-11713: Switch to C++17
This switches from C++14 to C++17, which allows Impala
code to interact with libraries written in C++17.
This fixes a few minor compilation differences, such
as the need for comparators to be const. Since C++17
includes -faligned-new, this removes the CMake code
that specified it.
Testing:
- Ran core job
Change-Id: Iadac41817fe5eaaa469a5f0e9f94056a409c14b9
Reviewed-on: http://gerrit.cloudera.org:8080/19183
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/CMakeLists.txt
M be/src/exec/file-metadata-utils.cc
M be/src/gutil/endian.h
M be/src/runtime/krpc-data-stream-mgr.h
M be/src/service/impala-server.h
M be/src/udf_samples/CMakeLists.txt
6 files changed, 8 insertions(+), 18 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/19183
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Iadac41817fe5eaaa469a5f0e9f94056a409c14b9
Gerrit-Change-Number: 19183
Gerrit-PatchSet: 7
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
[Impala-ASF-CR] IMPALA-11713: Switch to C++17
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/19183 )
Change subject: IMPALA-11713: Switch to C++17
......................................................................
Patch Set 4: Code-Review+1
lgtm
--
To view, visit http://gerrit.cloudera.org:8080/19183
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iadac41817fe5eaaa469a5f0e9f94056a409c14b9
Gerrit-Change-Number: 19183
Gerrit-PatchSet: 4
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Tue, 20 Dec 2022 18:54:49 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11713: Switch to C++17
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/19183 )
Change subject: IMPALA-11713: Switch to C++17
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19183/4/be/CMakeLists.txt
File be/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/19183/4/be/CMakeLists.txt@48
PS4, Line 48: SET(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -std=c++17")
I wouldn't expect c++14 vs c++17 with the same compiler to have any performance impact. But maybe we should do an A-B perf test just to check.
--
To view, visit http://gerrit.cloudera.org:8080/19183
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iadac41817fe5eaaa469a5f0e9f94056a409c14b9
Gerrit-Change-Number: 19183
Gerrit-PatchSet: 4
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Tue, 20 Dec 2022 22:40:22 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11713: Switch to C++17
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/19183 )
Change subject: IMPALA-11713: Switch to C++17
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19183/4/be/CMakeLists.txt
File be/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/19183/4/be/CMakeLists.txt@48
PS4, Line 48: SET(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -std=c++17")
> I wouldn't expect c++14 vs c++17 with the same compiler to have any perform
I will run one and post the results on this review (or incorporate the results into the commit message).
--
To view, visit http://gerrit.cloudera.org:8080/19183
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iadac41817fe5eaaa469a5f0e9f94056a409c14b9
Gerrit-Change-Number: 19183
Gerrit-PatchSet: 4
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Tue, 20 Dec 2022 23:01:39 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11713: Switch to C++17
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19183 )
Change subject: IMPALA-11713: Switch to C++17
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/12045/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/19183
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iadac41817fe5eaaa469a5f0e9f94056a409c14b9
Gerrit-Change-Number: 19183
Gerrit-PatchSet: 4
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Tue, 20 Dec 2022 19:00:13 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11713: Switch to C++17
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19183 )
Change subject: IMPALA-11713: Switch to C++17
......................................................................
Patch Set 6: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/19183
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iadac41817fe5eaaa469a5f0e9f94056a409c14b9
Gerrit-Change-Number: 19183
Gerrit-PatchSet: 6
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Thu, 23 Feb 2023 03:22:47 +0000
Gerrit-HasComments: No