[C++][Dataset] Allow setting FragmentReadahead to 0 in ScannerBuilder

[asfimport]

I have an application where I need to set fragment readahead to 0. But, looks like for some reason the ScannerBuilder does not allow setting the fragment readahead to 0 [1]. It would be very helpful to know why it is that way and if a PR lifting that restriction would be accepted because a docstring mentions that users can set fragment readahead to 0 if they want [2].

[1]https://github.com/apache/arrow/blob/master/cpp/src/arrow/dataset/scanner.cc#L864
[2]

arrow/cpp/src/arrow/dataset/scanner.h

Line 93 in 998a2a1

/// Set to 0 to disable fragment readahead

Reporter: Jayjeet Chakraborty / @JayjeetAtGithub

Related issues:

• [C++][Datasets] Improve memory usage of datasets (is depended upon by)

_{Note: This issue was originally created as ARROW-13161. Please see the migration documentation for further details.}

[asfimport]

David Li / @lidavidm:
@westonpace can confirm but it's because places of the code use it as the number of files to read, total, i.e. readahead == 0 would read no fragments. So that docstring is a little misleading.

However: what is your use case, so we can better understand it/why does fragment_readahead = 1 not suffice?

[asfimport]

Jayjeet Chakraborty / @JayjeetAtGithub:
Thanks a lot for the quick response. We are using the DisposableScannerAdaptor in the JNI bridge for integrating Arrow with Spark. We use the Dataset API call to read a single file (not a collection of files). We want to map Sparks iterator with an iterator that the Dataset API provides via JNI. In the Arrow layer, the DisposalbleScannerAdaptor::Create calls the ScanBatches method from SyncScanner. AFAIK, the ScanBatches method is supposed to be lazy and return an iterator and batches will be read out only when Next() is called on the iterator. But currently, since the fragment readahead is > 0, our only fragment (note we have a single file per dataset API call) is read out in the initialization phase and we don't want that since that breaks the iterator mapping to Spark. We just want ScanBatches to return a TaggedRecordBatchIterator. I hope that clears our requirement a little bit.

[asfimport]

David Li / @lidavidm:
Thanks for clarifying. The sync scanner will be going away at some point, but both the sync and async scanner have this issue. The sync scanner especially would need refactoring in order to fix this issue, from what I see.

There's a batch_readahead parameter that would probably control this better, but it's not actually hooked up to anything.

[asfimport]

Jayjeet Chakraborty / @JayjeetAtGithub:
I see. Looks like fragment readahead has to be min. 1 for scanning to start. Also, I looked into the batch readahead parameter. Our fragments (files) have single record batches. So, it is not of much use to us at this point. 

[asfimport]

Weston Pace / @westonpace:
David is right, batch readahead is largely ignored as most formats don't actually do readahead at the file level.  I think the exception is parquet and it does prebuffering so it is difficult to limit it and batch readahead is not hooked up.  It is primarily there as a limit to how much RAM you use.  As for fragment readahead I agree it sounds like what you are describing is a fragment readahead of 0.

AFAIK, the ScanBatches method is supposed to be lazy and return an iterator and batches will be read out only when Next() is called on the iterator.

No, that is not how it was designed, as you have observed.  The scanner is designed to keep scanning in the background.  Can you explain a bit more how this breaks the iterator mapping with Spark?

 

What happens if you use the async scanner but set use_threads to false?

 

I think it would not be a far reach to get "don't read any batches until ScanBatches is called at least once" but it would be harder to get "don't do any readahead at all".  Also, the scanner is currently being migrated to use an ExecPlan for filtering/projection and that will create its own pressure.  @bkietz

[github-actions]

This issue has been marked as stale because it has had no activity in the past 365 days. Please remove the stale label or comment below, or this issue will be closed in 14 days. If this improvement is still desired but has no current owner, please add the 'Status: needs champion' label.