Skip to content
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

ARROW-17078: [C++] Clean up error handling in C++ Examples #13598

Merged
merged 14 commits into from Jul 18, 2022

Conversation

vibhatha
Copy link
Collaborator

A simple util file was added to include a macro used in each example and may be in future we can include common utils like data generators, writers, etc in the here. This is just a suggestion.

@vibhatha
Copy link
Collaborator Author

cc @pitrou @lidavidm

@pitrou
Copy link
Member

pitrou commented Jul 13, 2022

While usually I'm strongly in favour of avoiding duplication and building up reusable facilities, in this case I'm not sure it's a good idea to introduce a specific header file for examples, instead of a single .cc file per example.
@westonpace @cyb70289 @kou What do you think?

@lidavidm
Copy link
Member

If anything I'd rather have examples ported to the cookbook, but yes, I'd rather try to keep examples here reasonably self-contained.

@kou
Copy link
Member

kou commented Jul 14, 2022

In general, I want to keep examples as simple as possible for easy to understand by users. For example, minimal dependencies, straightforward implementations and so on.

In this case, I don't want to do this because it increases a (small) dependency (example_utils.h).

How about removing ABORT_ON_FAILURE() and using ARROW_RETURN_NOT_OK() with non-main() function like https://github.com/apache/arrow/blob/master/cpp/examples/arrow/join_example.cc instead of using ABORT_ON_FAILURE() in main()?

@vibhatha
Copy link
Collaborator Author

@kou may be I can do that here.

@kou
Copy link
Member

kou commented Jul 14, 2022

Yes, please. :-)

@lidavidm
Copy link
Member

We should also file a JIRA since this isn't really a minor change

@vibhatha vibhatha changed the title MINOR: [C++] Clean up cpp examples [DO NOT MERGE] MINOR: [C++] Clean up cpp examples Jul 14, 2022
@vibhatha vibhatha marked this pull request as ready for review July 15, 2022 02:36
@vibhatha vibhatha changed the title MINOR: [C++] Clean up cpp examples ARROW-17078: [C++] Cleaning up C++ Examples Jul 15, 2022
@vibhatha
Copy link
Collaborator Author

@lidavidm I renamed the PR to match with the filed JIRA. Should we do something to track it here?

@github-actions
Copy link

@lidavidm
Copy link
Member

It will automatically link the JIRA, just be patient and let the Dev CI pipeline run to make the link.

@vibhatha vibhatha requested a review from lidavidm July 18, 2022 09:18
@lidavidm
Copy link
Member

Looks like failures are primarily due to the Protobuf linkage issue.

@vibhatha
Copy link
Collaborator Author

Looks like failures are primarily due to the Protobuf linkage issue.

Is this independent of this change?

@lidavidm lidavidm changed the title ARROW-17078: [C++] Cleaning up C++ Examples ARROW-17078: [C++] Clean up error handling in C++ Examples Jul 18, 2022
@lidavidm lidavidm merged commit ffd31d8 into apache:master Jul 18, 2022
@ursabot
Copy link

ursabot commented Jul 18, 2022

Benchmark runs are scheduled for baseline = 4db3222 and contender = ffd31d8. ffd31d8 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Failed ⬇️0.0% ⬆️2.27%] ec2-t3-xlarge-us-east-2
[Failed ⬇️2.61% ⬆️0.0%] test-mac-arm
[Failed ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.21% ⬆️0.04%] ursa-thinkcentre-m75q
Buildkite builds:
[Failed] ffd31d8c ec2-t3-xlarge-us-east-2
[Failed] ffd31d8c test-mac-arm
[Failed] ffd31d8c ursa-i9-9960x
[Finished] ffd31d8c ursa-thinkcentre-m75q
[Failed] 4db32223 ec2-t3-xlarge-us-east-2
[Failed] 4db32223 test-mac-arm
[Failed] 4db32223 ursa-i9-9960x
[Finished] 4db32223 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants