-
Notifications
You must be signed in to change notification settings - Fork 839
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
Conversation
FlightClient
FlightClient
/// calling [`Self::into_inner`] and using the [`FlightDataStream`] | ||
/// directly. | ||
#[derive(Debug)] | ||
pub struct FlightRecordBatchStream { |
There was a problem hiding this comment.
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
Marking as draft as I work out conflicts with #3359 |
There was a problem hiding this 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
Co-authored-by: Raphael Taylor-Davies <[email protected]>
) | ||
})?; | ||
|
||
arrow_ipc::reader::read_dictionary( |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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
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. |
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:
FlightClient
What changes are included in this PR?
FlightClient
,FlightError
modules, originally written for IOx in https://github.com/influxdata/influxdb_iox/pull/6427Are there any user-facing changes?
These are new features
cc @tustvold @Dandandan @avantgardnerio