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/08/05 21:19:36 UTC
[GitHub] [arrow] lidavidm opened a new pull request #7908: ARROW-9358: [Integration] remove generated_large_batch.json
lidavidm opened a new pull request #7908:
URL: https://github.com/apache/arrow/pull/7908
This should speed up integration tests by moving the expensive large batch test to the individual Flight implementations.
----------------------------------------------------------------
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 pull request #7908: ARROW-9358: [Integration] remove generated_large_batch.json
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #7908:
URL: https://github.com/apache/arrow/pull/7908#issuecomment-671836605
No urgency at all. This is just an improvement in development comfort.
----------------------------------------------------------------
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 pull request #7908: ARROW-9358: [Integration] remove generated_large_batch.json
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #7908:
URL: https://github.com/apache/arrow/pull/7908#issuecomment-671380314
cc @emkornfield for the Java side.
----------------------------------------------------------------
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 #7908: ARROW-9358: [Integration] remove generated_large_batch.json
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #7908:
URL: https://github.com/apache/arrow/pull/7908#discussion_r467931586
##########
File path: cpp/src/arrow/flight/flight_test.cc
##########
@@ -1105,6 +1110,15 @@ TEST_F(TestFlightClient, DoGetDicts) {
CheckDoGet(descr, expected_batches, check_endpoints);
}
+// Ensure the gRPC client is configured to allow large messages
+// Tests a 32 MiB batch
+TEST_F(TestFlightClient, DoGetLargeBatch) {
+ BatchVector expected_batches;
+ ASSERT_OK(ExampleLargeBatches(&expected_batches));
+ Ticket ticket{"ticket-large-batch-1"};
+ CheckTicket(ticket, expected_batches);
Review comment:
I was a bit confused here because I didn't understand that `CheckTicket` actually issued a GET request. Can you call it `CheckDoGet` instead?
----------------------------------------------------------------
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] emkornfield commented on pull request #7908: ARROW-9358: [Integration] remove generated_large_batch.json
Posted by GitBox <gi...@apache.org>.
emkornfield commented on pull request #7908:
URL: https://github.com/apache/arrow/pull/7908#issuecomment-671835723
Is there an urgency for this? Have my hands full at work today and tomorrow at least, i can try to look later in the week?
----------------------------------------------------------------
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 #7908: ARROW-9358: [Integration] remove generated_large_batch.json
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #7908:
URL: https://github.com/apache/arrow/pull/7908#issuecomment-674180436
Addressed feedback and rebased.
@emkornfield this PR _removes_ generation of a large data file, which would have been read by Java later, and then adds a Java test that generates the data in-memory instead to replace it.
----------------------------------------------------------------
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] emkornfield commented on pull request #7908: ARROW-9358: [Integration] remove generated_large_batch.json
Posted by GitBox <gi...@apache.org>.
emkornfield commented on pull request #7908:
URL: https://github.com/apache/arrow/pull/7908#issuecomment-673877452
@lidavidm this looks fine to me, but based on PR description I would have expected java to have some sort of file read removed? Maybe its late but i just see added generation data which looks fine.
----------------------------------------------------------------
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 #7908: ARROW-9358: [Integration] remove generated_large_batch.json
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #7908:
URL: https://github.com/apache/arrow/pull/7908#discussion_r467932307
##########
File path: cpp/src/arrow/flight/flight_test.cc
##########
@@ -1515,6 +1529,26 @@ TEST_F(TestDoPut, DoPutDicts) {
CheckDoPut(descr, schema, batches);
}
+// Ensure the gRPC server is configured to allow large messages
+// Tests a 32 MiB batch
+TEST_F(TestDoPut, DoPutLargeBatch) {
+ const auto array_length = 32768;
+ auto descr = FlightDescriptor::Path({"floats"});
+ BatchVector batches;
+ std::vector<std::shared_ptr<arrow::Array>> arrays;
Review comment:
Can you delegate the record batch creation to a helper function, since it's done both for DoGet and DoPut tests?
##########
File path: cpp/src/arrow/flight/flight_test.cc
##########
@@ -1105,6 +1110,15 @@ TEST_F(TestFlightClient, DoGetDicts) {
CheckDoGet(descr, expected_batches, check_endpoints);
}
+// Ensure the gRPC client is configured to allow large messages
+// Tests a 32 MiB batch
+TEST_F(TestFlightClient, DoGetLargeBatch) {
+ BatchVector expected_batches;
+ ASSERT_OK(ExampleLargeBatches(&expected_batches));
+ Ticket ticket{"ticket-large-batch-1"};
+ CheckTicket(ticket, expected_batches);
Review comment:
I was a bit confused here because I didn't understand that `CheckTicket` actually issued a DoGet request. Can you call it `CheckDoGet` instead?
----------------------------------------------------------------
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] emkornfield commented on pull request #7908: ARROW-9358: [Integration] remove generated_large_batch.json
Posted by GitBox <gi...@apache.org>.
emkornfield commented on pull request #7908:
URL: https://github.com/apache/arrow/pull/7908#issuecomment-674346671
Thanks. +1 merging.
----------------------------------------------------------------
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 #7908: ARROW-9358: [Integration] remove generated_large_batch.json
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #7908:
URL: https://github.com/apache/arrow/pull/7908#issuecomment-669522949
https://issues.apache.org/jira/browse/ARROW-9358
----------------------------------------------------------------
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] emkornfield closed pull request #7908: ARROW-9358: [Integration] remove generated_large_batch.json
Posted by GitBox <gi...@apache.org>.
emkornfield closed pull request #7908:
URL: https://github.com/apache/arrow/pull/7908
----------------------------------------------------------------
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