ARROW-8853: [Rust] [Integration Testing] Enable Flight tests #8641

carols10cents · 2020-11-11T18:25:53Z

There are some parts of the tests that should probably be part of the arrow-flight crate instead, but that can be done in a future PR. Everything here is in the integration tests.

github-actions · 2020-11-11T18:31:51Z

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

nevi-me

LGTM subject to CI, just comments that don't need any further action beyond response/confirmation

nevi-me · 2020-11-11T19:01:53Z

rust/integration-testing/src/bin/flight-test-integration-client.rs

+    Ok(())
+}
+
+// TODO: should this be extended, abstracted, and moved out of test code and into production code?


my intuition says no, as I'm assuming that there'd be different ways of implementing authentication.
I have however not used Flight, so if the basic auth scenario is always this, we could move this into arrow-flight.

The C++ Flight client provides this Authenticate function and the ability to specify a ClientAuthHandler, but I'm not sure if that's the abstraction desired in Rust 🤷‍♀️

I think we can always add it later to the core Rust library -- for now, having code in the example that does the work that people can use / follow seems pretty good to me

rust/integration-testing/src/bin/flight-test-integration-server.rs

carols10cents · 2020-11-11T20:35:16Z

The archery unit tests are failing with this error:

E   ImportError: cannot import name 'HTTPHeaderDict'

In the step before the unit tests are run, there's this:

ERROR: After October 2020 you may experience errors when installing or updating packages. This is because pip will change the way that it resolves dependency conflicts.
Successfully installed MarkupSafe-1.1.1 archery attrs-20.3.0 certifi-2020.11.8 cffi-1.14.3 chardet-3.0.4 click-7.1.2 cryptography-3.2.1 defusedxml-0.6.0 deprecated-1.2.10 gitdb-4.0.5 gitpython-3.1.11 idna-2.10 importlib-metadata-2.0.0 iniconfig-1.1.1 jinja2-2.11.2 jira-2.0.0 numpy-1.18.5 oauthlib-3.1.0 packaging-20.4 pandas-0.25.3 pathlib2-2.3.5 pbr-5.5.1 pluggy-0.13.1 py-1.9.0 pycparser-2.20 pygithub-1.53 pyjwt-1.7.1 pyparsing-2.4.7 pytest-6.1.2 python-dateutil-2.8.1 python-dotenv-0.15.0 pytz-2020.4 requests-2.24.0 requests-oauthlib-1.3.0 requests-toolbelt-0.9.1 responses-0.12.0 ruamel.yaml-0.16.12 ruamel.yaml.clib-0.2.2 semver-2.13.0 six-1.15.0 smmap-3.0.4 toml-0.10.2 toolz-0.11.1 urllib3-1.26.0 wrapt-1.12.1 zipp-1.2.0

We recommend you use --use-feature=2020-resolver to test your packages with the new resolver before it becomes the default.

requests 2.24.0 requires urllib3!=1.25.0,!=1.25.1,<1.26,>=1.21.1, but you'll have urllib3 1.26.0 which is incompatible.

I'm not very familiar with Python but given it's November this seems likely...? I'm not sure the correct way to fix this, and I don't see any open PRs about it from a quick skim?

The other failing test job says it's out of disk space, is there anything I can do about that?

carols10cents · 2020-11-13T21:41:21Z

Ok, so, now the integration test job got cancelled after 360 min, and suspiciously it appears to be cancelled during the Flight tests after C++ -> C++ and C++ -> Java, right when I would expect C++ -> Rust to be, sooooo it's possible that it's my fault somehow. I will investigate more on Monday.

alamb

This is really cool @carols10cents . Thank you!

One very minor suggestion is to update https://github.com/integer32llc/arrow/blob/flight-integration-tests/rust/integration-testing/README.md to include mention of the new binaries, flight-test-integration

alamb · 2020-11-14T12:17:28Z

rust/integration-testing/src/bin/flight-test-integration-client.rs

+    Ok(())
+}
+
+// TODO: should this be extended, abstracted, and moved out of test code and into production code?


I think we can always add it later to the core Rust library -- for now, having code in the example that does the work that people can use / follow seems pretty good to me

alamb · 2020-11-14T12:35:30Z

Once the integration tests are passing, I think this PR is good to merge

jorgecarleitao

Really impressive, @carols10cents . Thank you so much for this ❤️

This adds Flight client and server implementations modeled after the behavior of the C++ Flight integration tests, and enables the Flight tests in the integration tests.

I had submitted this before but I think it got lost in a rebase somewhere. I think this is more correct and informative.

…ht data

carols10cents · 2020-12-11T13:36:58Z

I've decided to stop wasting CI time on this because it's not nearly ready :) Will reopen a new PR when it is.

jorgecarleitao · 2020-12-11T15:57:38Z

Just to say that I really appreciate that you are working on this. Thank you so much ❤️

…ies with that of a RecordBatch This PR contains a series of refactorings to extract code from the IPC `FileWriter` and `StreamWriter` that generates `EncodedData` instances. I highly recommend reviewing commit-by-commit, I tried to make the changes fairly mechanical and easy to understand at each step. With those refactorings, it becomes easier to reuse the code with Flight too, so that when we're generating `FlightData` for a `RecordBatch`, we get the `FlightData` for the batch's dictionaries too. This will help in getting the Flight integration tests that include dictionaries to pass... I'm also having some unrelated protocol problems over in [this PR I'm working on](#8641) but this work will help there. Closes #8826 from carols10cents/refactor-file-stream-writer Authored-by: Carol (Nichols || Goulding) <carol.nichols@gmail.com> Signed-off-by: Neville Dipale <nevilledips@gmail.com>

…ies with that of a RecordBatch This PR contains a series of refactorings to extract code from the IPC `FileWriter` and `StreamWriter` that generates `EncodedData` instances. I highly recommend reviewing commit-by-commit, I tried to make the changes fairly mechanical and easy to understand at each step. With those refactorings, it becomes easier to reuse the code with Flight too, so that when we're generating `FlightData` for a `RecordBatch`, we get the `FlightData` for the batch's dictionaries too. This will help in getting the Flight integration tests that include dictionaries to pass... I'm also having some unrelated protocol problems over in [this PR I'm working on](apache/arrow#8641) but this work will help there. Closes #8826 from carols10cents/refactor-file-stream-writer Authored-by: Carol (Nichols || Goulding) <carol.nichols@gmail.com> Signed-off-by: Neville Dipale <nevilledips@gmail.com>

github-actions bot added the Component: Rust label Nov 11, 2020

nevi-me approved these changes Nov 11, 2020

View reviewed changes

carols10cents force-pushed the flight-integration-tests branch from bf27024 to 087cedc Compare November 13, 2020 14:54

alamb reviewed Nov 14, 2020

View reviewed changes

alamb approved these changes Nov 14, 2020

View reviewed changes

jorgecarleitao approved these changes Nov 15, 2020

View reviewed changes

carols10cents force-pushed the flight-integration-tests branch from 429e6be to b62184e Compare November 19, 2020 14:17

carols10cents marked this pull request as draft November 19, 2020 14:19

github-actions bot added the needs-rebase A PR that needs to be rebased by the author label Nov 26, 2020

carols10cents and others added 17 commits December 3, 2020 11:28

Extract integration test json functions to the lib

9fd3f58

ARROW-8853: [Rust] [Integration Testing] Enable Flight tests

dd142d1

This adds Flight client and server implementations modeled after the behavior of the C++ Flight integration tests, and enables the Flight tests in the integration tests.

Don't send ACK for initial DoPut request from server

b078a65

Don't send or check for app_metadata in the DoPut request

cb54a90

Remove tokio spawn; a new thread isn't actually needed here

bfdfc78

Preload first batch in channel before starting DoPut request

355008e

rando debugging

7c182f9

tracing output

908c0a5

[upstream anyway] more generic

12d3138

[supahax] it gets to the next failure

aeb8539

Redo sending batches in client, needs cleaned up

0fb62e5

cargo fmt

b5b0414

Progress on understanding flight

6112e8a

more progress

4e79769

[Rust] Only return dict_id/dict_is_ordered values for Dictionary types

a3894c0

I had submitted this before but I think it got lost in a rebase somewhere. I think this is more correct and informative.

Actually really really read dictionaries

83e2dcd

set initial len of dictionaries

e8bcb79

carols10cents added 10 commits December 3, 2020 11:31

welp nope

2f97515

i know nothing

3af0c24

Extract generating schema EncodedData to a new struct's method

3cff161

Move record_batch_to_bytes to the new object

462d330

Extract dictionary_batch_to_bytes to the new struct

7366dd1

Move EncodedData generation out of write_message

3893361

Remove the now-unused intermediate Message enum

51be846

Extract a DictionaryTracker for logic of whether replacement is an error

71b9d8c

Extract shared dictionary code to the new struct

ab7ccc3

Always create FlightData for dictionaries when given a RecordBatch

dca9d09

carols10cents mentioned this pull request Dec 3, 2020

ARROW-10801: [Rust] [Flight] Support sending FlightData for Dictionaries with that of a RecordBatch #8826

Closed

carols10cents force-pushed the flight-integration-tests branch from c8bce76 to 3af0c24 Compare December 4, 2020 13:57

github-actions bot removed the needs-rebase A PR that needs to be rebased by the author label Dec 4, 2020

shepmaster and others added 10 commits December 4, 2020 09:13

cleaner

6eb0614

empty fix

5d3b333

kindawork

0b7b241

[upstream] print both values

d436206

Merge branch 'pr/8826' into flight-integration-tests

5fed5fd

reduce logging

87b3678

preload the authentication handshake

dd91f4f

Actually use the dictionaries instead of throwing them away :P

987fd60

Clear up what's being returned from converting a record batch to flig…

2574ce5

…ht data

send_all

46f76d7

carols10cents closed this Dec 11, 2020

asfimport mentioned this pull request Jan 10, 2021

[Rust] [Integration Testing] Enable Flight tests #24992

Closed

ARROW-8853: [Rust] [Integration Testing] Enable Flight tests #8641

ARROW-8853: [Rust] [Integration Testing] Enable Flight tests #8641

Uh oh!

Conversation

carols10cents commented Nov 11, 2020

Uh oh!

github-actions bot commented Nov 11, 2020

Uh oh!

nevi-me left a comment

Choose a reason for hiding this comment

Uh oh!

nevi-me Nov 11, 2020

Choose a reason for hiding this comment

Uh oh!

carols10cents Nov 11, 2020

Choose a reason for hiding this comment

Uh oh!

alamb Nov 14, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

carols10cents commented Nov 11, 2020

Uh oh!

carols10cents commented Nov 13, 2020

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alamb Nov 14, 2020

Choose a reason for hiding this comment

Uh oh!

alamb commented Nov 14, 2020

Uh oh!

jorgecarleitao left a comment

Choose a reason for hiding this comment

Uh oh!

carols10cents commented Dec 11, 2020

Uh oh!

jorgecarleitao commented Dec 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants