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.

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

Initial Mid-level FlightClient #3378

Merged
merged 8 commits into from
Dec 23, 2022
Merged

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Dec 20, 2022

Which issue does this PR close?

Towards #3371

Rationale for this change

I am building a FlightSQL client. There are many lower level details such as RecordBatch reconstruction that are not FlightSQL specific. Not only are they easier to test as a separate layer, the mid level client is useful on its own (for writing clients for other Arrow Flight based APIs) so I am contributing it here.

More details are on #3371

If this basic approach is accepted, I plan:

  1. PRs with tests for do_get (this PR is already quite large)
  2. PR to update the FlightSQL client to use FlightClient
  3. File follow on tickets for the remaining APIS (e.g. do_action). I will likely not have time to implement them

What changes are included in this PR?

  1. Initial FlightClient, FlightError modules, originally written for IOx in https://github.com/influxdata/influxdb_iox/pull/6427
  2. Documentation
  3. Integration test

Are there any user-facing changes?

These are new features

cc @tustvold @Dandandan @avantgardnerio

@alamb alamb changed the title Start to Implement Mid-level FlightClient Initial Mid-level FlightClient Dec 20, 2022
@github-actions github-actions bot added the arrow-flight Changes to the arrow-flight crate label Dec 20, 2022
@alamb alamb requested a review from tustvold December 20, 2022 17:09
/// calling [`Self::into_inner`] and using the [`FlightDataStream`]
/// directly.
#[derive(Debug)]
pub struct FlightRecordBatchStream {
Copy link
Contributor Author

@alamb alamb Dec 20, 2022

Choose a reason for hiding this comment

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

This code is not tested yet (it is covered by tests in the IOx repo). I plan to add tests for it as a follow on PR

@alamb alamb marked this pull request as draft December 20, 2022 17:17
@alamb
Copy link
Contributor Author

alamb commented Dec 20, 2022

Marking as draft as I work out conflicts with #3359

Copy link
Contributor

@tustvold tustvold left a comment

Choose a reason for hiding this comment

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

Looks good to me

arrow-flight/tests/common/server.rs Outdated Show resolved Hide resolved
arrow-flight/src/client.rs Outdated Show resolved Hide resolved
arrow-flight/src/client.rs Outdated Show resolved Hide resolved
arrow-flight/src/client.rs Outdated Show resolved Hide resolved
arrow-flight/src/client.rs Outdated Show resolved Hide resolved
arrow-flight/src/client.rs Outdated Show resolved Hide resolved
@alamb alamb marked this pull request as ready for review December 20, 2022 18:03
Co-authored-by: Raphael Taylor-Davies <[email protected]>
)
})?;

arrow_ipc::reader::read_dictionary(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe there was some discussion about improving the efficiency of IPC reads, etc. This is now the place to do so.

// specific language governing permissions and limitations
// under the License.

/// Errors for the Apache Arrow Flight crate
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This follows the same model as ArrowError as I did not want to innovate a new Error paradigm (e.g. thiserror as part of this PR)


//! Integration test for "mid level" Client

mod common {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

here are the "missing" tests for arrow-flight -- I think this framework will let us bang out the tests for the flight / flight sql functionality

arrow-flight/tests/common/server.rs Outdated Show resolved Hide resolved
@alamb alamb merged commit 17b3210 into apache:master Dec 23, 2022
@ursabot
Copy link

ursabot commented Dec 23, 2022

Benchmark runs are scheduled for baseline = af07998 and contender = 17b3210. 17b3210 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
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

@alamb alamb deleted the alamb/flght_client2 branch December 25, 2022 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow-flight Changes to the arrow-flight crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants