ARROW-10420: [C++] Refactor io and filesystem APIs to take an IOContext #9474

pitrou · 2021-02-11T18:07:41Z

The io::IOContext class allows passing various settings such as the MemoryPool used for allocation and the Executor for async methods.

github-actions · 2021-02-11T18:08:16Z

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

pitrou · 2021-02-11T18:08:55Z

A couple things to discuss:

API choices: should we take a const IOContext&, a const IOContext*, a IOContext*?
I've added an owned IOContext to the base FileSystem class. Should we do that on RandomAccessFile or InputStream as well?

@westonpace @bkietz input welcome.

pitrou · 2021-02-11T18:11:26Z

cpp/src/arrow/dataset/file_base.cc

Note: explicitly closing output files is preferrable, especially with remote filesystems where this might plausibly fail.

(and MockFileSystem will deliberately not write anything out if you don't close it explicitly)

pitrou · 2021-02-11T18:12:22Z

cpp/src/arrow/filesystem/filesystem.cc

Same comment here about explicitly closing files.

pitrou · 2021-02-11T18:13:01Z

cpp/src/arrow/filesystem/filesystem.cc

Note that this was scheduling file copies on the CPU thread pool.

westonpace · 2021-02-11T20:35:37Z

I've added an owned IOContext to the base FileSystem class. Should we do that on RandomAccessFile or InputStream as well?

My gut says no but I could be convinced otherwise. For example, the S3 filesystem (if I understand correctly) would plug the executor into the AWS client configuration and the random access files / etc. would rely on that and not the Executor directly. At best it would just be for convenience right? A way to provide implementors easy access to the context instead of having to take care of passing it around?

emkornfield · 2021-02-12T14:59:07Z

naming nit: IoContext?

pitrou · 2021-02-16T12:53:30Z

@westonpace I think I misphrased my question. This PR adds an owned IOContext to filesystem instances. My question was whether stream objects should also have an owned IOContext; currently they only keep a const-ref, meaning it has to be kept alive elsewhere.

@emkornfield Hmm, I'm not sure what the convention should be. We currently have Status::IOError, though.

emkornfield · 2021-02-17T04:15:44Z

@emkornfield Hmm, I'm not sure what the convention should be. We currently have Status::IOError, though.

According to the style guide IoContext:

For the purposes of the naming rules below, a "word" is anything that you would write in English without internal spaces. This includes abbreviations, such as acronyms and initialisms. For names written in mixed case (also sometimes referred to as "camel case" or "Pascal case"), in which the first letter of each word is capitalized, prefer to capitalize abbreviations as single words, e.g., StartRpc() rather than StartRPC().

However, if we want to keep consistency I'm OK with IO.

bkietz · 2021-02-18T18:21:08Z

Since IOContext only wraps pointers and an id integer, semantically it represents a reference. Therefore I'd recommend never producing references to them; it's redundant and the structure is tiny and trivially copyable anyway.

pitrou · 2021-02-25T14:11:31Z

cpp/src/arrow/csv/reader.cc

Note that the CPU and IO executors were passed in the wrong order here.

pitrou · 2021-02-25T14:15:55Z

@ursabot please benchmark

bkietz

a few minor comments

cpp/src/arrow/io/interfaces.h

r/src/filesystem.cpp

cpp/src/arrow/filesystem/filesystem.h

cpp/src/arrow/io/interfaces.h

The `io::IOContext` class allows passing various settings such as the MemoryPool used for allocation and the Executor for async methods.

pitrou · 2021-02-25T19:15:22Z

@bkietz Do you want to give this another look?

bkietz

LGTM, will merge when CI completes

bkietz · 2021-02-25T20:07:38Z

r/src/csv.cpp

-  return ValueOrStop(
-      arrow::csv::TableReader::Make(gc_memory_pool(), arrow::io::AsyncContext(), input,
-                                    *read_options, *parse_options, *convert_options));
+  return ValueOrStop(arrow::csv::TableReader::Make(arrow::io::IOContext(gc_memory_pool()),


Oh, good catch

ursabot · 2021-02-25T20:23:32Z

Benchmark runs are scheduled for baseline = 9a9baf6 and contender = da3ece9. Results will be available as each benchmark for each run completes:
[Finished] ursa-dgx1: https://conbench.ursa.dev/compare/runs/d21bb0de-dc67-4bd3-8e0e-00cee85a296d...60e9bcc3-0bf6-4386-8b6c-983b398ae676/
[Finished] ursa-i9-9960x: https://conbench.ursa.dev/compare/runs/9dda2d10-03ed-4b58-8b57-6cac360134cc...885e0359-8e9d-4302-b464-0ba20e50e8f7/
If you have Ursa Computing Inc's Buildkite access, you can also view benchmark runs logs using these links:
[Finished] https://buildkite.com/ursa-computing/arrow-run-benchmarks-dgx/builds/401
[Finished] https://buildkite.com/ursa-computing/arrow-run-benchmarks/builds/904
[Finished] https://buildkite.com/ursa-computing/arrow-run-benchmarks/builds/903
[Finished] https://buildkite.com/ursa-computing/arrow-run-benchmarks-dgx/builds/400

bkietz · 2021-02-26T02:16:37Z

CI failure is ARROW-11717. Merging

github-actions bot added Component: C++ Component: Parquet labels Feb 11, 2021

pitrou commented Feb 11, 2021

View reviewed changes

cpp/src/arrow/filesystem/filesystem.cc Outdated

Copy link

Member Author

pitrou Feb 11, 2021

Choose a reason for hiding this comment

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

Same comment here about explicitly closing files.

pitrou commented Feb 11, 2021

View reviewed changes

cpp/src/arrow/filesystem/filesystem.cc Outdated

Copy link

Member Author

pitrou Feb 11, 2021

Choose a reason for hiding this comment

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

Note that this was scheduling file copies on the CPU thread pool.

pitrou force-pushed the ARROW-10420-io-context branch 2 times, most recently from 443569d to bb09c58 Compare February 11, 2021 19:11

github-actions bot added the Component: R label Feb 11, 2021

jorgecarleitao force-pushed the master branch from d4608a9 to 356c300 Compare February 14, 2021 12:09

pitrou force-pushed the ARROW-10420-io-context branch from bb09c58 to 37373b4 Compare February 25, 2021 13:59

pitrou marked this pull request as ready for review February 25, 2021 13:59

pitrou commented Feb 25, 2021

View reviewed changes

cpp/src/arrow/csv/reader.cc Outdated

Copy link

Member Author

pitrou Feb 25, 2021

Choose a reason for hiding this comment

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

Note that the CPU and IO executors were passed in the wrong order here.

pitrou requested a review from bkietz February 25, 2021 14:12

bkietz requested changes Feb 25, 2021

View reviewed changes

cpp/src/arrow/io/interfaces.h Outdated Show resolved Hide resolved

r/src/filesystem.cpp Outdated Show resolved Hide resolved

cpp/src/arrow/filesystem/filesystem.h Outdated Show resolved Hide resolved

cpp/src/arrow/io/interfaces.h Outdated Show resolved Hide resolved

pitrou added 2 commits February 25, 2021 19:38

ARROW-10420: [C++] Refactor io and filesystem APIs to take an IOContext

1baede8

The `io::IOContext` class allows passing various settings such as the MemoryPool used for allocation and the Executor for async methods.

Fix reversed CPU and IO executor in CSV reader

d750f4c

pitrou force-pushed the ARROW-10420-io-context branch from f16a80c to a463936 Compare February 25, 2021 19:15

bkietz approved these changes Feb 25, 2021

View reviewed changes

Address review comments

da3ece9

pitrou force-pushed the ARROW-10420-io-context branch from a463936 to da3ece9 Compare February 25, 2021 19:36

bkietz reviewed Feb 25, 2021

View reviewed changes

github-actions bot added Component: GLib Component: Python labels Feb 25, 2021

bkietz closed this in 59f9d20 Feb 26, 2021

pitrou deleted the ARROW-10420-io-context branch February 26, 2021 10:44

asfimport mentioned this pull request Feb 26, 2021

[C++] FileSystem::OpenInput{File,Stream} should accept a MemoryPool #26400

Closed

ARROW-10420: [C++] Refactor io and filesystem APIs to take an IOContext #9474

ARROW-10420: [C++] Refactor io and filesystem APIs to take an IOContext #9474

Uh oh!

Conversation

pitrou commented Feb 11, 2021

Uh oh!

github-actions bot commented Feb 11, 2021

Uh oh!

pitrou commented Feb 11, 2021

Uh oh!

pitrou Feb 11, 2021

Choose a reason for hiding this comment

Uh oh!

pitrou Feb 11, 2021

Choose a reason for hiding this comment

Uh oh!

pitrou Feb 11, 2021

Choose a reason for hiding this comment

Uh oh!

pitrou Feb 11, 2021

Choose a reason for hiding this comment

Uh oh!

westonpace commented Feb 11, 2021

Uh oh!

emkornfield commented Feb 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pitrou commented Feb 16, 2021

Uh oh!

emkornfield commented Feb 17, 2021

Uh oh!

bkietz commented Feb 18, 2021

Uh oh!

pitrou Feb 25, 2021

Choose a reason for hiding this comment

Uh oh!

pitrou commented Feb 25, 2021

Uh oh!

bkietz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pitrou commented Feb 25, 2021

Uh oh!

bkietz left a comment

Choose a reason for hiding this comment

Uh oh!

bkietz Feb 25, 2021

Choose a reason for hiding this comment

Uh oh!

ursabot commented Feb 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bkietz commented Feb 26, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

emkornfield commented Feb 12, 2021 •

edited

Loading

ursabot commented Feb 25, 2021 •

edited

Loading