Skip to content

ARROW-17846: [C++] Use if constexpr in CSV subsystem - #14241

Merged
pitrou merged 2 commits into
apache:masterfrom
pitrou:ARROW-17846-csv-if-constexpr
Sep 27, 2022
Merged

ARROW-17846: [C++] Use if constexpr in CSV subsystem#14241
pitrou merged 2 commits into
apache:masterfrom
pitrou:ARROW-17846-csv-if-constexpr

Conversation

@pitrou

@pitroupitrou commented Sep 26, 2022

Copy link
Copy Markdown
Member

No description provided.

pitrou
pitrou commented Sep 26, 2022
View reviewed changes
Comment threadcpp/src/arrow/visit_type_inline.h
/// The intent is for this to be called on a generic lambda
/// that may internally use `if constexpr` or similar constructs.
template <typename VISITOR>
inline auto VisitType(const DataType& type, VISITOR&& visitor)

@pitroupitrouSep 26, 2022

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this API and its implementation seem right? @bkietz@lidavidm

@lidavidmlidavidmSep 26, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still getting up to speed with the new features but this looks right to me/looks to be the same as std::visit in usage.

@bkietzbkietzSep 26, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@pitrou

pitrou commented Sep 26, 2022

Copy link
Copy Markdown
MemberAuthor

Revision: 9f83b81

Submitted crossbow builds: ursacomputing/crossbow @ csv-if-constexpr-0

TaskStatus
test-alpine-linux-cppGithub Actions
test-build-cpp-fuzzGithub Actions
test-conda-cppGithub Actions
test-conda-cpp-valgrindAzure
test-debian-10-cpp-amd64Github Actions
test-debian-10-cpp-i386Github Actions
test-debian-11-cpp-amd64Github Actions
test-debian-11-cpp-i386Github Actions
test-fedora-35-cppGithub Actions
test-ubuntu-18.04-cppGithub Actions
test-ubuntu-18.04-cpp-releaseGithub Actions
test-ubuntu-18.04-cpp-staticGithub Actions
test-ubuntu-20.04-cppGithub Actions
test-ubuntu-20.04-cpp-17Github Actions
test-ubuntu-20.04-cpp-bundledGithub Actions
test-ubuntu-20.04-cpp-thread-sanitizerGithub Actions
test-ubuntu-22.04-cppGithub Actions

lidavidm
lidavidm approved these changes Sep 26, 2022
View reviewed changes
Comment threadcpp/src/arrow/visit_type_inline.h
/// The intent is for this to be called on a generic lambda
/// that may internally use `if constexpr` or similar constructs.
template <typename VISITOR>
inline auto VisitType(const DataType& type, VISITOR&& visitor)

@lidavidmlidavidmSep 26, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still getting up to speed with the new features but this looks right to me/looks to be the same as std::visit in usage.

@github-actions

github-actionsBot commented Sep 26, 2022

Copy link
Copy Markdown

https://issues.apache.org/jira/browse/ARROW-17846

@github-actions

github-actionsBot commented Sep 26, 2022

Copy link
Copy Markdown

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

bkietz
bkietz requested changes Sep 26, 2022
View reviewed changes
Comment threadcpp/src/arrow/type_traits.h Outdated
@pitrou
pitrou merged commit df7babb into apache:masterSep 27, 2022
@pitrou
pitrou deleted the ARROW-17846-csv-if-constexpr branch September 27, 2022 07:58
@ursabot

ursabot commented Sep 27, 2022

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = f277f2e and contender = df7babb. df7babb is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.72% ⬆️0.0%] test-mac-arm
[Failed ⬇️0.82% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.21% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] df7babbf ec2-t3-xlarge-us-east-2
[Finished] df7babbf test-mac-arm
[Failed] df7babbf ursa-i9-9960x
[Finished] df7babbf ursa-thinkcentre-m75q
[Finished] f277f2e5 ec2-t3-xlarge-us-east-2
[Failed] f277f2e5 test-mac-arm
[Failed] f277f2e5 ursa-i9-9960x
[Finished] f277f2e5 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@ursabot

ursabot commented Sep 27, 2022

Copy link
Copy Markdown

['Python', 'R'] benchmarks have high level of regressions.
test-mac-arm

@asfimportasfimport mentioned this pull request Sep 27, 2022
Closed
@js8544js8544 mentioned this pull request Jan 31, 2023
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bkietzbkietzbkietz requested changes
@lidavidmlidavidmlidavidm approved these changes

Assignees

No one assigned

Labels

Component: C++

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@pitrou@ursabot@lidavidm@bkietz