ARROW-16032: [C++] Migrate FlightClient API to Result<> #12719

zagto · 2022-03-25T20:57:17Z

No description provided.

github-actions · 2022-03-25T20:57:36Z

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

c_glib/arrow-flight-glib/client.cpp

cpp/src/arrow/flight/client.h

cpp/src/arrow/flight/flight_benchmark.cc

cpp/src/arrow/flight/integration_tests/test_integration_client.cc

cpp/src/arrow/flight/sql/client.h

cpp/src/arrow/flight/sql/server_test.cc

cpp/src/arrow/flight/sql/test_app_cli.cc

cpp/src/arrow/flight/test_definitions.cc

lidavidm

Thanks, let's just make sure we keep the GLib code consistently formatted

lidavidm · 2022-04-05T12:25:34Z

c_glib/arrow-flight-glib/client.cpp

+     arrow::Result<std::unique_ptr<arrow::flight::FlightClient>> result = arrow::flight::FlightClient::Connect(*flight_location,
+                                                        *flight_options);
+    status = std::move(result).Value(&flight_client);


A few things:

There's an extra space at the beginning of the line.

Why not use auto as before and keep things aligned, in a consistent style with everything else?

For formatting, I thought archery would fix these things. Does it not check GLib code?
The explicit type was left over from when I was debugging why the original suggestion wouldn't work, I'll change it back

GLib doesn't use an auto-formatter, unfortunately. So, sorry for the nit-picking here :(

@kou have we considered automated formatting for the GLib code?

We have considered it a few times but we didn't do any actions for it.
It may be a good time to work on it: https://issues.apache.org/jira/browse/ARROW-16137

c_glib/arrow-flight-glib/client.cpp

ursabot · 2022-04-06T11:51:04Z

Benchmark runs are scheduled for baseline = 8eaa995 and contender = 9e08c50. 9e08c50 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.29% ⬆️0.0%] test-mac-arm
[Failed ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.04% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/450| 9e08c503 ec2-t3-xlarge-us-east-2>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/435| 9e08c503 test-mac-arm>
[Failed] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/436| 9e08c503 ursa-i9-9960x>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/445| 9e08c503 ursa-thinkcentre-m75q>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/449| 8eaa995f ec2-t3-xlarge-us-east-2>
[Failed] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/434| 8eaa995f test-mac-arm>
[Failed] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/435| 8eaa995f ursa-i9-9960x>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/444| 8eaa995f 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

kou · 2022-04-09T07:26:47Z

@github-actions crossbow submit verify-rc-source-cpp-linux-ubuntu-18.04-amd64

github-actions · 2022-04-09T07:27:53Z

Revision: 9518ca7

Submitted crossbow builds: ursacomputing/crossbow @ actions-1836

Task	Status
verify-rc-source-cpp-linux-ubuntu-18.04-amd64

kou · 2022-04-09T20:23:55Z

@zagto It seems that this breaks a CI job on Ubuntu 18.04:

https://github.com/ursacomputing/crossbow/runs/5953173410?check_suite_focus=true#step:5:8623

FAILED: src/arrow/flight/CMakeFiles/arrow_flight_objlib.dir/client.cc.o 
/usr/bin/c++  -DARROW_FLIGHT_EXPORTING -DARROW_HAVE_RUNTIME_AVX2 -DARROW_HAVE_RUNTIME_AVX512 -DARROW_HAVE_RUNTIME_BMI2 -DARROW_HAVE_RUNTIME_SSE4_2 -DARROW_HAVE_SSE4_2 -DARROW_HDFS -DARROW_JEMALLOC -DARROW_JEMALLOC_INCLUDE_DIR="" -DARROW_WITH_BROTLI -DARROW_WITH_BZ2 -DARROW_WITH_LZ4 -DARROW_WITH_RE2 -DARROW_WITH_SNAPPY -DARROW_WITH_TIMING_TESTS -DARROW_WITH_UTF8PROC -DARROW_WITH_ZLIB -DARROW_WITH_ZSTD -DGRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS=grpc::experimental -DGRPC_USE_TLS_CHANNEL_CREDENTIALS_OPTIONS -DGRPC_USE_TLS_CHANNEL_CREDENTIALS_OPTIONS_ROOT_CERTS -DGTEST_LINKED_AS_SHARED_LIBRARY=1 -DURI_STATIC_BUILD -DUTF8PROC_STATIC -isystem /arrow/cpp/thirdparty/flatbuffers/include -isystem boost_ep-prefix/src/boost_ep -isystem snappy_ep/src/snappy_ep-install/include -isystem brotli_ep/src/brotli_ep-install/include -isystem gflags_ep-prefix/src/gflags_ep/include -isystem thrift_ep-install/include -isystem protobuf_ep-install/include -isystem jemalloc_ep-prefix/src -isystem googletest_ep-prefix/include -isystem rapidjson_ep/src/rapidjson_ep-install/include -isystem xsimd_ep/src/xsimd_ep-install/include -isystem lz4_ep-prefix/include -isystem zstd_ep-install/include -isystem re2_ep-install/include -isystem bzip2_ep-install/include -isystem utf8proc_ep-install/include -isystem cares_ep-install/include -isystem grpc_ep-install/include -isystem /arrow/cpp/thirdparty/hadoop/include -isystem orc_ep-install/include -Isrc -I/arrow/cpp/src -I/arrow/cpp/src/generated -Wno-noexcept-type  -fdiagnostics-color=always -O3 -DNDEBUG  -Wall -fno-semantic-interposition -msse4.2  -O3 -DNDEBUG -fPIC   -std=c++11 -MD -MT src/arrow/flight/CMakeFiles/arrow_flight_objlib.dir/client.cc.o -MF src/arrow/flight/CMakeFiles/arrow_flight_objlib.dir/client.cc.o.d -o src/arrow/flight/CMakeFiles/arrow_flight_objlib.dir/client.cc.o -c /arrow/cpp/src/arrow/flight/client.cc
/arrow/cpp/src/arrow/flight/client.cc: In member function 'arrow::Result<std::unique_ptr<arrow::flight::FlightStreamReader> > arrow::flight::FlightClient::DoGet(const arrow::flight::FlightCallOptions&, const arrow::flight::Ticket&)':
/arrow/cpp/src/arrow/flight/client.cc:618:10: error: could not convert 'stream_reader' from 'std::unique_ptr<arrow::flight::ClientStreamReader, std::default_delete<arrow::flight::ClientStreamReader> >' to 'arrow::Result<std::unique_ptr<arrow::flight::FlightStreamReader> >'
   return stream_reader;
          ^~~~~~~~~~~~~

Could you confirm this?

lidavidm · 2022-04-11T11:42:55Z

Thank you @kou. I can confirm this. I filed ARROW-16162/#12856.

Closes apache#12719 from zagto/flight-api-result-client Authored-by: Tobias Zagorni <tobias@zagorni.eu> Signed-off-by: David Li <li.davidm96@gmail.com>

use Result<> in FlightClient API

1ff4bbb

github-actions bot added Component: FlightRPC Component: C++ Component: GLib Component: Python labels Mar 25, 2022

zagto added 6 commits March 25, 2022 22:03

pyarrow: add missing &

27ea2d8

fix GLib bindings

154e6aa

fix missing integration test client

a658e8f

fix GLib binding syntax

6f6f2e4

fix formatting

4aec579

simplify test

43975c1

zagto marked this pull request as ready for review March 28, 2022 19:03

zagto added 3 commits March 29, 2022 22:45

Merge branch 'master' into flight-api-result-client

65d9553

fix merge problems

2059c70

formatting

42459a6

lidavidm self-requested a review March 30, 2022 14:08

Merge branch 'master' into flight-api-result-client

01f7385

lidavidm reviewed Mar 31, 2022

View reviewed changes

zagto added 6 commits April 4, 2022 14:30

nicer formatting of GLib binding

9061897

rename stream -> writer in DoPut API

9d235e2

apply style changes from PR suggestions

b43e097

Merge branch 'master' into flight-api-result-client

9581cc2

correct new style in GLib client.cpp

9062f07

fix Cuda tests

8f8fa69

lidavidm approved these changes Apr 5, 2022

View reviewed changes

improve style of GLib code

9518ca7

lidavidm approved these changes Apr 5, 2022

View reviewed changes

lidavidm closed this in 9e08c50 Apr 5, 2022

wjones127 mentioned this pull request Apr 6, 2022

ARROW-16065: [FlightRPC][Docs] Improve Flight documentation #12815

Closed

asfimport mentioned this pull request Jul 7, 2023

[C++][FlightRPC] Migrate SQL Client API to Result<> #31521

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARROW-16032: [C++] Migrate FlightClient API to Result<> #12719

ARROW-16032: [C++] Migrate FlightClient API to Result<> #12719

zagto commented Mar 25, 2022

github-actions bot commented Mar 25, 2022

lidavidm left a comment

lidavidm Apr 5, 2022

zagto Apr 5, 2022

lidavidm Apr 5, 2022

kou Apr 7, 2022

ursabot commented Apr 6, 2022

kou commented Apr 9, 2022

github-actions bot commented Apr 9, 2022

kou commented Apr 9, 2022

lidavidm commented Apr 11, 2022 •

edited

ARROW-16032: [C++] Migrate FlightClient API to Result<> #12719

ARROW-16032: [C++] Migrate FlightClient API to Result<> #12719

Conversation

zagto commented Mar 25, 2022

github-actions bot commented Mar 25, 2022

lidavidm left a comment

Choose a reason for hiding this comment

lidavidm Apr 5, 2022

Choose a reason for hiding this comment

zagto Apr 5, 2022

Choose a reason for hiding this comment

lidavidm Apr 5, 2022

Choose a reason for hiding this comment

kou Apr 7, 2022

Choose a reason for hiding this comment

ursabot commented Apr 6, 2022

kou commented Apr 9, 2022

github-actions bot commented Apr 9, 2022

kou commented Apr 9, 2022

lidavidm commented Apr 11, 2022 • edited

lidavidm commented Apr 11, 2022 •

edited