You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/06/29 17:09:30 UTC
[GitHub] [arrow] lidavidm opened a new pull request #7582: ARROW-8190: [FlightRPC][C++] Expose IPC options
lidavidm opened a new pull request #7582:
URL: https://github.com/apache/arrow/pull/7582
- Python is not covered as I'm not sure how best to expose these structs to Python.
- Java is not covered as it doesn't use IpcOption at all currently; I'd rather hold off and see how the metadata change is implemented before trying to bind it. (The legacy/modern metadata format doesn't come into play since Flight uses Protobuf and not an IPC message exactly per se.)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #7582: ARROW-8190: [FlightRPC][C++] Expose IPC options
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #7582:
URL: https://github.com/apache/arrow/pull/7582#issuecomment-651251968
https://issues.apache.org/jira/browse/ARROW-8190
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on pull request #7582: ARROW-8190: [FlightRPC][C++] Expose IPC options
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #7582:
URL: https://github.com/apache/arrow/pull/7582#issuecomment-651813343
Thanks for the review! I've fixed the comment/test names.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #7582: ARROW-8190: [FlightRPC][C++] Expose IPC options
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #7582:
URL: https://github.com/apache/arrow/pull/7582#discussion_r447665599
##########
File path: cpp/src/arrow/flight/flight_test.cc
##########
@@ -1808,6 +1880,90 @@ TEST_F(TestMetadata, DoPutReadMetadata) {
ASSERT_OK(writer->Close());
}
+TEST_F(TestOptions, DoGetReadOptions) {
+ // Call DoGet, but with a very low read nesting depth set to fail the call.
+ Ticket ticket{""};
+ auto options = FlightCallOptions();
+ options.read_options.max_recursion_depth = 1;
+ std::unique_ptr<FlightStreamReader> stream;
+ ASSERT_OK(client_->DoGet(options, ticket, &stream));
+ FlightStreamChunk chunk;
+ ASSERT_RAISES(Invalid, stream->Next(&chunk));
+}
+
+TEST_F(TestOptions, DoPutWriteOptions) {
+ // Call DoPut, but with a very low write nesting depth set to fail the call.
+ std::unique_ptr<FlightStreamWriter> writer;
+ std::unique_ptr<FlightMetadataReader> reader;
+ BatchVector expected_batches;
+ ASSERT_OK(ExampleNestedBatches(&expected_batches));
+
+ auto options = FlightCallOptions();
+ options.write_options.max_recursion_depth = 1;
+ ASSERT_OK(client_->DoPut(options, FlightDescriptor{}, expected_batches[0]->schema(),
+ &writer, &reader));
+ for (const auto& batch : expected_batches) {
+ ASSERT_RAISES(Invalid, writer->WriteRecordBatch(*batch));
+ }
+}
+
+TEST_F(TestOptions, DoExchangeReadOptions) {
+ // Call DoExchange and write nested data, but read with a very low nesting depth set to
+ // fail the call.
+ auto options = FlightCallOptions();
+ options.write_options.max_recursion_depth = 1;
+ auto descr = FlightDescriptor::Command("");
+ std::unique_ptr<FlightStreamReader> reader;
+ std::unique_ptr<FlightStreamWriter> writer;
+ ASSERT_OK(client_->DoExchange(options, descr, &writer, &reader));
+ BatchVector batches;
+ ASSERT_OK(ExampleNestedBatches(&batches));
+ ASSERT_OK(writer->Begin(batches[0]->schema()));
+ for (const auto& batch : batches) {
+ ASSERT_RAISES(Invalid, writer->WriteRecordBatch(*batch));
+ }
+ ASSERT_OK(writer->DoneWriting());
+ ASSERT_OK(writer->Close());
+}
+
+TEST_F(TestOptions, DoExchangeReadOptionsBegin) {
+ // Call DoExchange and write nested data, but read with a very low nesting depth set to
+ // fail the call. Here the options are set explicitly when we write data and not in the
+ // call options.
+ auto descr = FlightDescriptor::Command("");
+ std::unique_ptr<FlightStreamReader> reader;
+ std::unique_ptr<FlightStreamWriter> writer;
+ ASSERT_OK(client_->DoExchange(descr, &writer, &reader));
+ BatchVector batches;
+ ASSERT_OK(ExampleNestedBatches(&batches));
+ auto options = ipc::IpcWriteOptions::Defaults();
+ options.max_recursion_depth = 1;
+ ASSERT_OK(writer->Begin(batches[0]->schema(), options));
+ for (const auto& batch : batches) {
+ ASSERT_RAISES(Invalid, writer->WriteRecordBatch(*batch));
+ }
+ ASSERT_OK(writer->DoneWriting());
+ ASSERT_OK(writer->Close());
+}
+
+TEST_F(TestOptions, DoExchangeWriteOptions) {
+ // Call DoExchange and write nested data, but with a very low nesting depth set to fail
+ // the call. (The low nesting depth is set on the server side.)
+ auto options = FlightCallOptions();
+ options.read_options.max_recursion_depth = 1;
Review comment:
Why do you set the read options here? AFAICT, this test raises its error on the server side.
##########
File path: cpp/src/arrow/flight/flight_test.cc
##########
@@ -1808,6 +1880,90 @@ TEST_F(TestMetadata, DoPutReadMetadata) {
ASSERT_OK(writer->Close());
}
+TEST_F(TestOptions, DoGetReadOptions) {
+ // Call DoGet, but with a very low read nesting depth set to fail the call.
+ Ticket ticket{""};
+ auto options = FlightCallOptions();
+ options.read_options.max_recursion_depth = 1;
+ std::unique_ptr<FlightStreamReader> stream;
+ ASSERT_OK(client_->DoGet(options, ticket, &stream));
+ FlightStreamChunk chunk;
+ ASSERT_RAISES(Invalid, stream->Next(&chunk));
+}
+
+TEST_F(TestOptions, DoPutWriteOptions) {
+ // Call DoPut, but with a very low write nesting depth set to fail the call.
+ std::unique_ptr<FlightStreamWriter> writer;
+ std::unique_ptr<FlightMetadataReader> reader;
+ BatchVector expected_batches;
+ ASSERT_OK(ExampleNestedBatches(&expected_batches));
+
+ auto options = FlightCallOptions();
+ options.write_options.max_recursion_depth = 1;
+ ASSERT_OK(client_->DoPut(options, FlightDescriptor{}, expected_batches[0]->schema(),
+ &writer, &reader));
+ for (const auto& batch : expected_batches) {
+ ASSERT_RAISES(Invalid, writer->WriteRecordBatch(*batch));
+ }
+}
+
+TEST_F(TestOptions, DoExchangeReadOptions) {
+ // Call DoExchange and write nested data, but read with a very low nesting depth set to
Review comment:
Comment seems off, it's the write options that are being set (and it's the `WriteRecordBatch` call that fails).
##########
File path: cpp/src/arrow/flight/flight_test.cc
##########
@@ -1808,6 +1880,90 @@ TEST_F(TestMetadata, DoPutReadMetadata) {
ASSERT_OK(writer->Close());
}
+TEST_F(TestOptions, DoGetReadOptions) {
+ // Call DoGet, but with a very low read nesting depth set to fail the call.
+ Ticket ticket{""};
+ auto options = FlightCallOptions();
+ options.read_options.max_recursion_depth = 1;
+ std::unique_ptr<FlightStreamReader> stream;
+ ASSERT_OK(client_->DoGet(options, ticket, &stream));
+ FlightStreamChunk chunk;
+ ASSERT_RAISES(Invalid, stream->Next(&chunk));
+}
+
+TEST_F(TestOptions, DoPutWriteOptions) {
+ // Call DoPut, but with a very low write nesting depth set to fail the call.
+ std::unique_ptr<FlightStreamWriter> writer;
+ std::unique_ptr<FlightMetadataReader> reader;
+ BatchVector expected_batches;
+ ASSERT_OK(ExampleNestedBatches(&expected_batches));
+
+ auto options = FlightCallOptions();
+ options.write_options.max_recursion_depth = 1;
+ ASSERT_OK(client_->DoPut(options, FlightDescriptor{}, expected_batches[0]->schema(),
+ &writer, &reader));
+ for (const auto& batch : expected_batches) {
+ ASSERT_RAISES(Invalid, writer->WriteRecordBatch(*batch));
+ }
+}
+
+TEST_F(TestOptions, DoExchangeReadOptions) {
+ // Call DoExchange and write nested data, but read with a very low nesting depth set to
+ // fail the call.
+ auto options = FlightCallOptions();
+ options.write_options.max_recursion_depth = 1;
+ auto descr = FlightDescriptor::Command("");
+ std::unique_ptr<FlightStreamReader> reader;
+ std::unique_ptr<FlightStreamWriter> writer;
+ ASSERT_OK(client_->DoExchange(options, descr, &writer, &reader));
+ BatchVector batches;
+ ASSERT_OK(ExampleNestedBatches(&batches));
+ ASSERT_OK(writer->Begin(batches[0]->schema()));
+ for (const auto& batch : batches) {
+ ASSERT_RAISES(Invalid, writer->WriteRecordBatch(*batch));
+ }
+ ASSERT_OK(writer->DoneWriting());
+ ASSERT_OK(writer->Close());
+}
+
+TEST_F(TestOptions, DoExchangeReadOptionsBegin) {
+ // Call DoExchange and write nested data, but read with a very low nesting depth set to
Review comment:
Comment seems off, it's the write options that are being set (and it's the `WriteRecordBatch` call that fails).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org