ORC-262: [C++] Support async io prefetch for orc c++ lib #2048

taiyang-li · 2024-10-09T07:33:30Z

What changes were proposed in this pull request?

Support async io prefetch for orc c++ lib. Close https://issues.apache.org/jira/browse/ORC-262

Changes:

Added new interface InputStream::readAsync(default unimplemented). It reads io asynchronously within the specified range.
Added IO Cache implementation ReadRangeCache to cache async io results. This borrows from a similar design of Parquet Reader in https://github.com/apache/arrow
Added interface Reader::preBuffer to trigger io prefetch. In the specific implementation of ReaderImpl::preBuffer, the io ranges will be calculated according to the selected stripe and columns, and then these ranges will be merged and sorted, and ReadRangeCache::cache will be called to trigger the asynchronous io in the background, waiting for the use of the upper layer
Added the interface Reader::releaseBuffer, which is used to release all cached io ranges before an offset

Why are the changes needed?

Async io prefetch could hide io latency during reading orc files, which improves performance of scan operators in ClickHouse.

How was this patch tested?

Was this patch authored or co-authored using generative AI tooling?

c++/src/io/Cache.hh

c++/src/MemoryPool.cc

c++/src/Reader.cc

c++/src/io/Cache.hh

ffacs · 2024-10-11T03:27:46Z

Reader::preBuffer prefetch stripes as a unit which might be too large. For those users who don't want to prefetch entire file one-shot, they have to know the structure of the file. Do you think it is a good idea to make prefetch transparent to users and let the orc reader prefetch data(eg. 1MB for each column at a time) when it's proper.
What's more, we could make enable async IO a option and expose a cache interface for users so they can implement their eviction policy.

taiyang-li · 2024-10-11T04:36:28Z

Reader::preBuffer prefetch stripes as a unit which might be too large. For those users who don't want to prefetch entire file one-shot, they have to know the structure of the file. Do you think it is a good idea to make prefetch transparent to users and let the orc reader prefetch data(eg. 1MB for each column at a time) when it's proper. What's more, we could make enable async IO a option and expose a cache interface for users so they can implement their eviction policy.

It is totally decided by users to choose whether to prefetch the whole orc file or single/multiple columns in single stripe or single column in single/multiple stripes. Reader::preBuffer already supported all those options.

It is better letting user invoke Reader::preBuffer explicitly because only user knows which stripe/columns to read. Thus they could find the best change to prefetch to hide io latency sufficiently. e.g. the orc prefetch implementation in ClickHouse relying on current PR: ClickHouse/ClickHouse#70534 (speed up 1.47x). Besides, the parquet reader in apache arrow also has similar design.

taiyang-li · 2024-10-16T02:05:25Z

@ffacs @wgtmac any more comments ? Thanks!

wgtmac · 2024-10-16T15:29:22Z

Sorry that I'm a little bit overwhelmed these days. Will take a look when I get the chance.

BTW, @luffy-zh is implementing exposing RowIndex positions: #2005. Perhaps there is an opportunity to further prefetch io together with predicate pushdown.

taiyang-li · 2024-10-17T00:56:24Z

@wgtmac That's a great work. We could do more improvements on IO latency hiding after it is merged.

wgtmac

I have just finished the initial review. Thanks @taiyang-li! Please see my inline comments. My main concern is the usability that it requires user to call preBuffer instead of automatically prefetching required data.

c++/include/orc/OrcFile.hh

c++/include/orc/Reader.hh

c++/src/MemoryPool.cc

c++/src/Reader.hh

c++/src/StripeStream.hh

c++/src/io/Cache.hh

c++/include/orc/Reader.hh

c++/src/io/Cache.hh

c++/src/Reader.cc

dongjoon-hyun · 2024-11-07T20:09:09Z

Could you resolve the conflicts, @taiyang-li ?

taiyang-li · 2024-11-11T01:14:00Z

Could you resolve the conflicts, @taiyang-li ?

Done.

c++/src/Reader.cc

c++/include/orc/Reader.hh

c++/src/Reader.cc

wgtmac

I still have some concerns about the public API. Please see my inline comments.

BTW, I believe void preBuffer(const std::vector<int>& stripes, const std::list<uint64_t>& includeTypes) is a little bit coarse. We need to think about how to work together with selective read (e.g. when predicate pushdown is able to filter most rows).

c++/include/orc/Reader.hh

c++/include/orc/OrcFile.hh

wgtmac

Thanks for the update and benchmark! I think we should not add the readAsync function which returns a DataBuffer. Please see my inline comments.

c++/src/io/Cache.cc

c++/src/Reader.hh

c++/include/orc/Reader.hh

c++/src/io/Cache.hh

c++/include/orc/OrcFile.hh

taiyang-li · 2024-11-21T08:27:23Z

@wgtmac thanks for your advice. I had already finished the requested changes. Do you think the pr is ready to be merged ?

wgtmac

Thanks for the quick update! Now the public interfaces look good.

I haven't carefully reviewed Cache.hh/cc yet. Hopefully I can make it by the end of this week.

cc @ffacs @luffy-zh

c++/include/orc/OrcFile.hh

c++/include/orc/Reader.hh

wgtmac · 2024-11-21T08:51:32Z

c++/include/orc/Reader.hh

@@ -624,6 +647,21 @@ namespace orc {
     */
    virtual std::map<uint32_t, RowGroupIndex> getRowGroupIndex(
        uint32_t stripeIndex, const std::set<uint32_t>& included = {}) const = 0;
+
+    /**
+     * Trigger IO prefetch and cache the prefetched contents asynchronously.


Please add the expectation when it is called multiple times w/ or w/o overlapping ranges. Also it is good to mention that it is thread safe.

wgtmac · 2024-11-21T09:00:23Z

c++/src/StripeStream.cc

@@ -19,6 +19,7 @@
 #include "StripeStream.hh"
 #include "RLE.hh"
 #include "Reader.hh"
+#include "io/Cache.hh"


Please sort alphabetically.

wgtmac · 2024-11-21T09:02:37Z

c++/src/StripeStream.cc

@@ -37,7 +38,8 @@ namespace orc {
        stripeStart_(stripeStart),
        input_(input),
        writerTimezone_(writerTimezone),
-        readerTimezone_(readerTimezone) {
+        readerTimezone_(readerTimezone),
+        readCache_(reader.getReadCache()) {


You might directly call RowReaderImpl.getFileContents() to get readCache as suggested above.

c++/src/io/Cache.hh

wgtmac · 2024-11-21T09:07:08Z

c++/test/TestReader.cc

+    for (size_t i = 0; i < num_stripes; ++i) {
+      stripes.push_back(i);
+    }
+    reader->preBuffer(stripes, {0});


Please add test case where preBuffer is called for multiple times and with different stripe/column, etc.

Co-authored-by: Gang Wu <[email protected]>

luffy-zh · 2024-11-22T03:20:59Z

c++/src/io/Cache.cc

+    bool hit_cache = false;
+    if (it != entries_.end() && it->range.contains(range)) {
+      hit_cache = it->future.valid();
+      it->future.get();


If it->future.valid() returns false, we might encounter an exception here.

wgtmac · 2024-11-22T05:05:53Z

c++/src/io/Cache.hh

+    std::vector<ReadRange> coalesce(std::vector<ReadRange> ranges) const;
+  };
+
+  std::vector<ReadRange> coalesceReadRanges(std::vector<ReadRange> ranges, uint64_t holeSizeLimit,


What about moving coalesceReadRanges into struct ReadRangeCombiner as a static function? Actually I think a separate coalesceReadRanges function is redundant.

wgtmac · 2024-11-22T05:07:05Z

c++/src/io/Cache.hh

+  std::vector<ReadRange> coalesceReadRanges(std::vector<ReadRange> ranges, uint64_t holeSizeLimit,
+                                            uint64_t rangeSizeLimit);
+  struct RangeCacheEntry {
+    using BufferPtr = InputStream::BufferPtr;


We can directly use std::shared_ptr now.

wgtmac · 2024-11-22T05:07:38Z

c++/src/io/Cache.hh

+    BufferPtr buffer;
+    std::shared_future<void> future;
+
+    RangeCacheEntry() = default;


Should it be delete?

wgtmac · 2024-11-22T05:08:05Z

c++/src/io/Cache.hh

+    RangeCacheEntry(const ReadRange& range, BufferPtr buffer, std::future<void> future)
+        : range(range), buffer(std::move(buffer)), future(std::move(future).share()) {}
+
+    friend bool operator<(const RangeCacheEntry& left, const RangeCacheEntry& right) {


Why friend?

wgtmac · 2024-11-22T05:20:00Z

c++/src/io/Cache.hh

+    using Buffer = InputStream::Buffer;
+    using BufferPtr = InputStream::BufferPtr;
+
+    struct BufferSlice {


To be consistent, struct BufferSlice should not be a nest class as well.

wgtmac · 2024-11-22T05:32:58Z

c++/src/io/Cache.hh

+   private:
+    std::vector<RangeCacheEntry> makeCacheEntries(const std::vector<ReadRange>& ranges);
+
+    InputStream* stream_;


Please either remove blank lines or add a blank line between member variables to keep consistency.

wgtmac · 2024-11-22T05:33:46Z

c++/src/io/Cache.cc

+    entries_.erase(entries_.begin(), it);
+  }
+
+  std::vector<RangeCacheEntry> ReadRangeCache::makeCacheEntries(


nit: make it const or static

wgtmac · 2024-11-22T05:38:57Z

c++/src/io/Cache.cc

+
+    auto itr = ranges.begin();
+    // Ensure ranges is not empty.
+    assert(itr <= ranges.end());


this assert is unnecessary

wgtmac · 2024-11-22T05:42:04Z

c++/src/io/Cache.cc

+    uint64_t coalescedStart = itr->offset;
+    uint64_t coalescedEnd = coalescedStart + itr->length;
+
+    for (++itr; itr < ranges.end(); ++itr) {


We have iterated the ranges for three times: line 31, line 41 and here. It can be done in a single pass after sorting.

wgtmac · 2024-11-22T06:15:03Z

c++/src/io/Cache.cc

+    bool hit_cache = false;
+    if (it != entries_.end() && it->range.contains(range)) {
+      hit_cache = it->future.valid();
+      it->future.get();


Should we catch and rethrow an orc::Exception?
Should we use timeout here to fallback to direct read?

support async io prefetch for orc c++ lib

09ad258

github-actions bot added the CPP label Oct 9, 2024

taiyang-li changed the title ~~Support async io prefetch for orc c++ lib~~ ORC-262: [C++] Support async io prefetch for orc c++ lib Oct 9, 2024

taiyang-li added 4 commits October 9, 2024 16:14

fix failed ci

b0aeb9e

fix styles

bad04f8

add uts

8692901

fix failed uts

a6159e5

taiyang-li mentioned this pull request Oct 10, 2024

Support async io prefetch for NativeORCBlockInputFormat ClickHouse/ClickHouse#70534

Draft

21 tasks

fix building

1808c90

taiyang-li mentioned this pull request Oct 10, 2024

[WIP] ORC async prefetch ClickHouse/orc#18

Closed

taiyang-li added 2 commits October 10, 2024 10:49

fix format

4a3fe36

fix failed ci

2d8f77d

ffacs reviewed Oct 10, 2024

View reviewed changes

c++/src/io/Cache.hh Outdated Show resolved Hide resolved

taiyang-li added 3 commits October 10, 2024 18:39

fix bugs when ranges has the same offsets

90c3539

fix bugs when ranges has the same offsets

52e02a4

fix failed cit

a6691bc

zhanglistar reviewed Oct 11, 2024

View reviewed changes

Update Cache.hh

a2d6723

wgtmac requested changes Oct 24, 2024

View reviewed changes

ffacs reviewed Oct 24, 2024

View reviewed changes

c++/src/io/Cache.hh Outdated Show resolved Hide resolved

c++/src/io/Cache.hh Outdated Show resolved Hide resolved

c++/src/io/Cache.hh Outdated Show resolved Hide resolved

taiyang-li added 3 commits October 25, 2024 12:05

change as request

fc58f1f

change as request

07c803d

change as request

156f4eb

ffacs reviewed Oct 25, 2024

View reviewed changes

c++/src/Reader.cc Show resolved Hide resolved

fix format

bb5b3ae

fix style

3303fe9

fix conflicts

ac5a188

ffacs reviewed Nov 11, 2024

View reviewed changes

c++/src/Reader.cc Outdated Show resolved Hide resolved

c++/include/orc/Reader.hh Outdated Show resolved Hide resolved

c++/src/Reader.cc Outdated Show resolved Hide resolved

fix style

2046a01

wgtmac requested changes Nov 14, 2024

View reviewed changes

c++/include/orc/Reader.hh Outdated Show resolved Hide resolved

c++/include/orc/Reader.hh Outdated Show resolved Hide resolved

c++/include/orc/OrcFile.hh Outdated Show resolved Hide resolved

c++/include/orc/OrcFile.hh Outdated Show resolved Hide resolved

taiyang-li added 5 commits November 15, 2024 10:21

hide cacheoptions

48ddd50

protext read range cache from parallel accessing

f3d76f1

hide some private structures

64861ce

fix building

2bd47b0

fix code style

75dbf47

wgtmac requested changes Nov 20, 2024

View reviewed changes

change as requested

0e14b04

taiyang-li force-pushed the upstream_orc_prefetch branch from c8016c0 to 0e14b04 Compare November 21, 2024 03:38

taiyang-li added 3 commits November 21, 2024 11:40

change

e7ec8f9

add metrics

60ca6e9

revert files

5e7c4db

wgtmac requested changes Nov 21, 2024

View reviewed changes

taiyang-li and others added 2 commits November 21, 2024 17:38

Update c++/include/orc/OrcFile.hh

9c00e51

Co-authored-by: Gang Wu <[email protected]>

Update c++/include/orc/Reader.hh

5dc0266

Co-authored-by: Gang Wu <[email protected]>

taiyang-li marked this pull request as draft November 21, 2024 09:39

taiyang-li and others added 5 commits November 21, 2024 20:00

Update c++/src/io/Cache.hh

7e40819

Co-authored-by: Gang Wu <[email protected]>

Update c++/src/io/Cache.hh

bed31a5

Co-authored-by: Gang Wu <[email protected]>

Update c++/include/orc/Reader.hh

8dd4ca4

Co-authored-by: Gang Wu <[email protected]>

Update c++/include/orc/OrcFile.hh

ce3d455

Co-authored-by: Gang Wu <[email protected]>

Update c++/src/Options.hh

e20fc4e

Co-authored-by: Gang Wu <[email protected]>

luffy-zh reviewed Nov 22, 2024

View reviewed changes

wgtmac reviewed Nov 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ORC-262: [C++] Support async io prefetch for orc c++ lib #2048

ORC-262: [C++] Support async io prefetch for orc c++ lib #2048

taiyang-li commented Oct 9, 2024 •

edited

Loading

ffacs commented Oct 11, 2024

taiyang-li commented Oct 11, 2024 •

edited

Loading

taiyang-li commented Oct 16, 2024

wgtmac commented Oct 16, 2024

taiyang-li commented Oct 17, 2024

wgtmac left a comment

dongjoon-hyun commented Nov 7, 2024

taiyang-li commented Nov 11, 2024

wgtmac left a comment

wgtmac left a comment

taiyang-li commented Nov 21, 2024 •

edited

Loading

wgtmac left a comment

wgtmac Nov 21, 2024

wgtmac Nov 21, 2024

wgtmac Nov 21, 2024

wgtmac Nov 21, 2024

luffy-zh Nov 22, 2024

wgtmac Nov 22, 2024

wgtmac Nov 22, 2024

wgtmac Nov 22, 2024

wgtmac Nov 22, 2024

wgtmac Nov 22, 2024

wgtmac Nov 22, 2024

wgtmac Nov 22, 2024

wgtmac Nov 22, 2024

wgtmac Nov 22, 2024

wgtmac Nov 22, 2024

ORC-262: [C++] Support async io prefetch for orc c++ lib #2048

Are you sure you want to change the base?

ORC-262: [C++] Support async io prefetch for orc c++ lib #2048

Conversation

taiyang-li commented Oct 9, 2024 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

How was this patch tested?

Was this patch authored or co-authored using generative AI tooling?

ffacs commented Oct 11, 2024

taiyang-li commented Oct 11, 2024 • edited Loading

taiyang-li commented Oct 16, 2024

wgtmac commented Oct 16, 2024

taiyang-li commented Oct 17, 2024

wgtmac left a comment

Choose a reason for hiding this comment

dongjoon-hyun commented Nov 7, 2024

taiyang-li commented Nov 11, 2024

wgtmac left a comment

Choose a reason for hiding this comment

wgtmac left a comment

Choose a reason for hiding this comment

taiyang-li commented Nov 21, 2024 • edited Loading

wgtmac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taiyang-li commented Oct 9, 2024 •

edited

Loading

taiyang-li commented Oct 11, 2024 •

edited

Loading

taiyang-li commented Nov 21, 2024 •

edited

Loading