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.

Sign up for GitHub

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

Jump to bottom

ARROW-15974: [C++] Migrate flight/types.h header definitions to use Result<> #12669

Closed
wants to merge 32 commits into from
Closed

ARROW-15974: [C++] Migrate flight/types.h header definitions to use Result<> #12669

wants to merge 32 commits into from

Conversation

zagto
Copy link
Contributor

@zagto zagto commented Mar 18, 2022

No description provided.

@github-actions
Copy link

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

@github-actions
Copy link

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

lidavidm
lidavidm reviewed Mar 18, 2022
View reviewed changes
Copy link
Member

@lidavidm lidavidm left a comment

Choose a reason for hiding this comment

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

It looks like test_definitions.cc has to be updated as well, and you'll want to format the code

cpp/src/arrow/python/flight.cc Outdated Show resolved Hide resolved
cpp/src/arrow/flight/types.cc Outdated Show resolved Hide resolved
cpp/src/arrow/flight/types.h Outdated Show resolved Hide resolved
@lidavidm
Copy link
Member

thanks for taking this on!

lidavidm
lidavidm approved these changes Mar 21, 2022
View reviewed changes
Copy link
Member

@lidavidm lidavidm left a comment

Choose a reason for hiding this comment

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

Thanks!

@lidavidm lidavidm changed the title ARROW-15974: Migrate flight/types.h header definitions to use Result<> ARROW-15974: [C++] Migrate flight/types.h header definitions to use Result<> Mar 21, 2022
@zagto
Copy link
Contributor Author

zagto commented Mar 21, 2022

looks like the CI fails when using deprecated APIs, in AMD64 MacOS 10.15 Python 3

I thought I could change the usage inside arrow code later, in a separate PR. Does this mean I should better include everything here?

@lidavidm
Copy link
Member

Yes, we'll have to update the usages of these APIs at the same time we deprecate them.

lidavidm
lidavidm reviewed Mar 22, 2022
View reviewed changes
cpp/src/arrow/flight/sql/server_test.cc Outdated Show resolved Hide resolved
cpp/src/arrow/flight/sql/test_server_cli.cc Outdated Show resolved Hide resolved
cpp/src/arrow/flight/test_definitions.cc Outdated Show resolved Hide resolved
cpp/src/arrow/flight/transport/grpc/grpc_server.cc Outdated Show resolved Hide resolved
cpp/src/arrow/flight/transport/grpc/grpc_server.cc Outdated Show resolved Hide resolved
lidavidm
lidavidm approved these changes Mar 23, 2022
View reviewed changes
Copy link
Member

@lidavidm lidavidm left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@lidavidm
Copy link
Member

It looks like there's a few more deprecations to take care of:

/Users/runner/work/arrow/arrow/cpp/src/arrow/flight/types.cc
/Users/runner/work/arrow/arrow/cpp/src/arrow/flight/types.cc:414:32: error: 'Next' is deprecated: Deprecated in 8.0.0. Use Result-returning overload instead. [-Werror,-Wdeprecated-declarations]
      RETURN_NOT_OK(delegate_->Next(&next));
                               ^
/Users/runner/work/arrow/arrow/cpp/src/arrow/flight/types.h:539:3: note: 'Next' has been explicitly marked deprecated here
  ARROW_DEPRECATED("Deprecated in 8.0.0. Use Result-returning overload instead.")
  ^
/Users/runner/work/arrow/arrow/cpp/src/arrow/util/macros.h:113:48: note: expanded from macro 'ARROW_DEPRECATED'
#  define ARROW_DEPRECATED(...) __attribute__((deprecated(__VA_ARGS__)))
                                               ^
1 error generated.

@zagto
Copy link
Contributor Author

zagto commented Mar 24, 2022

This should now be almost(?) complete. I also included Read() and GetSchema() methods of Flight that are not part of types.h since it's easier to find them all in one go.

I'm wondering if it is necessary also create new tests for all the deprecated methods, since they are no longer used anywhere?

lidavidm
lidavidm reviewed Mar 25, 2022
View reviewed changes
cpp/src/arrow/flight/client.h Show resolved Hide resolved
cpp/src/arrow/flight/transport/grpc/grpc_client.cc Outdated Show resolved Hide resolved
cpp/src/arrow/flight/server.h Outdated Show resolved Hide resolved
cpp/src/arrow/flight/test_definitions.cc Outdated Show resolved Hide resolved
@lidavidm
Copy link
Member

I don't think we have to add tests of the deprecated methods

@lidavidm
Copy link
Member

Filed ARROW-16053 for the AppVeyor failure.

@zagto
Copy link
Contributor Author

zagto commented Mar 29, 2022

It looks like the Python MacOS failures happen on other PRs to, so there are also unrelated to this PR. Seing no more CI failures I think this PR is good to go from my side now

@pitrou
Copy link
Member

pitrou commented Mar 29, 2022

Yes, the S3FS crashes on macOS are unrelated to this PR.

@pitrou pitrou closed this in d214455 Mar 29, 2022
@ursabot
Copy link

ursabot commented Mar 29, 2022 •
edited
Loading

Benchmark runs are scheduled for baseline = be45ec6 and contender = d214455. d214455 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
[Finished ⬇️0.13% ⬆️0.0%] test-mac-arm
[Failed ⬇️0.36% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.47% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/405| d214455c ec2-t3-xlarge-us-east-2>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/391| d214455c test-mac-arm>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/391| d214455c ursa-i9-9960x>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/401| d214455c ursa-thinkcentre-m75q>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/404| be45ec60 ec2-t3-xlarge-us-east-2>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/390| be45ec60 test-mac-arm>
[Failed] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/390| be45ec60 ursa-i9-9960x>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/400| be45ec60 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

@lidavidm
Copy link
Member

Thanks for tackling this @zagto!

jcralmeida pushed a commit to rafael-telles/arrow that referenced this pull request Apr 19, 2022
@zagto @jcralmeida
ARROW-15974: [C++] Migrate flight/types.h header definitions to use R…
7cb2a57 
…esult<>

Closes apache#12669 from zagto/flight-api-result-types

Authored-by: Tobias Zagorni <tobias@zagorni.eu>
Signed-off-by: Antoine Pitrou <antoine@python.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pitrou pitrou pitrou left review comments

@lidavidm lidavidm lidavidm approved these changes

Assignees
No one assigned
Labels
Component: C++ Component: FlightRPC Component: GLib Component: Python
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@zagto @lidavidm @pitrou @ursabot