You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/04/16 15:38:02 UTC
[GitHub] [arrow] lidavidm opened a new pull request #10070: ARROW-12231: [C++][Python][Dataset] Differentiate one-shot datasets
lidavidm opened a new pull request #10070:
URL: https://github.com/apache/arrow/pull/10070
As suggested, this makes a new subclass for datasets that wrap iterators/readers. The difference is not exposed on the Python side. Later, when ARROW-12289 lands, StreamingDataset can return a subclass of Fragment which uses an I/O thread to scan the reader.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on pull request #10070: ARROW-12231: [C++][Python][Dataset] Differentiate one-shot datasets
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #10070:
URL: https://github.com/apache/arrow/pull/10070#issuecomment-832638716
I think you could argue that Fragment is just `Iterable<RecordBatch>` while Scanner is `Iterator<RecordBatch>`. While usually Scanner is a rewindable (but not random-access) iterator, it only guarantees ForwardIterator. Furthermore it's pretty simple to implement a one-shot scanner by having it wrap a (non-public-API) one-shot fragment (like the one implemented here).
Or put another way, if we limit the one-shotness to the Scanner, then we can hide the odd nonconforming Dataset/Fragment from the public API.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #10070: ARROW-12231: [C++][Python][Dataset] Differentiate one-shot datasets
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #10070:
URL: https://github.com/apache/arrow/pull/10070#discussion_r625999228
##########
File path: cpp/src/arrow/dataset/dataset.cc
##########
@@ -244,6 +222,81 @@ Result<FragmentIterator> InMemoryDataset::GetFragmentsImpl(compute::Expression)
return MakeMaybeMapIterator(std::move(create_fragment), std::move(batches_it));
}
+struct ReaderRecordBatchGenerator : InMemoryDataset::RecordBatchGenerator {
+ explicit ReaderRecordBatchGenerator(std::shared_ptr<RecordBatchReader> reader)
+ : reader_(std::move(reader)), consumed_(false) {}
+
+ RecordBatchIterator Get() const final {
+ if (consumed_) {
+ return MakeErrorIterator<std::shared_ptr<RecordBatch>>(
+ Status::Invalid("OneShotDataset was already consumed"));
+ }
+ consumed_ = true;
+ auto reader = reader_;
+ return MakeFunctionIterator([reader] { return reader->Next(); });
Review comment:
```suggestion
return MakeIteratorFromReader(reader_);
```
##########
File path: cpp/src/arrow/dataset/dataset.h
##########
@@ -219,6 +218,22 @@ class ARROW_DS_EXPORT InMemoryDataset : public Dataset {
std::shared_ptr<RecordBatchGenerator> get_batches_;
};
+/// \brief A Source which yields fragments wrapping a one-shot stream
+/// of record batches.
+///
+/// Unlike other datasets, this can be scanned only once. This is
+/// intended to support writing data from streaming sources or other
+/// sources that can be iterated only once.
+class ARROW_DS_EXPORT OneShotDataset : public InMemoryDataset {
Review comment:
Nit: this is no longer necessarily in memory
```suggestion
class ARROW_DS_EXPORT OneShotDataset : public Dataset {
```
##########
File path: cpp/src/arrow/dataset/scanner.cc
##########
@@ -673,6 +673,62 @@ ScannerBuilder::ScannerBuilder(std::shared_ptr<Schema> schema,
DCHECK_OK(Filter(scan_options_->filter));
}
+class ARROW_DS_EXPORT OneShotScanTask : public ScanTask {
Review comment:
Instead of exporting these, I think we can keep them in an anonymous namespace
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on pull request #10070: ARROW-12231: [C++][Python][Dataset] Differentiate one-shot datasets
Posted by GitBox <gi...@apache.org>.
bkietz commented on pull request #10070:
URL: https://github.com/apache/arrow/pull/10070#issuecomment-832149506
To me, this seems less like a subclass of dataset and more like a subclass of Scanner: IMHO it's not intuitive that a dataset would ever be single-shot. Instead, I think it'd make more sense to add `Scanner::MakeFromRecordBatchReader` or so, and (probably) add single-shot-ness to the contract of Scanner.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on pull request #10070: ARROW-12231: [C++][Python][Dataset] Differentiate one-shot datasets
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #10070:
URL: https://github.com/apache/arrow/pull/10070#issuecomment-824060446
Rebased to pick up ARROW-12289; now OneShotDataset uses its own Fragment implementation so that ScanBatchesAsync uses a background thread, to avoid blocking in the async scanner.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on pull request #10070: ARROW-12231: [C++][Python][Dataset] Differentiate one-shot datasets
Posted by GitBox <gi...@apache.org>.
westonpace commented on pull request #10070:
URL: https://github.com/apache/arrow/pull/10070#issuecomment-821420585
Hmm...maybe streaming isn't the most intuitive name then. Technically all the file-based datasets are "streaming". If a user was copying a dataset for example we would stream the data a few batches at a time. Should we just use SingleShotDataset?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on pull request #10070: ARROW-12231: [C++][Python][Dataset] Differentiate one-shot datasets
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #10070:
URL: https://github.com/apache/arrow/pull/10070#issuecomment-832885553
Right (though the implementation would just be a SyncScanner wrapping a OneShotFragment).
In fact, Joris already refactored the Python side to have `write` take a Scanner. So in all cases, a Scanner gets passed to C++ (batches/etc. get turned into an InMemoryDataset and scanned; iterators get turned into a scanner directly). I'll rebase again to make sure everything still fits together.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on pull request #10070: ARROW-12231: [C++][Python][Dataset] Differentiate one-shot datasets
Posted by GitBox <gi...@apache.org>.
westonpace commented on pull request #10070:
URL: https://github.com/apache/arrow/pull/10070#issuecomment-821393195
It's a pity, if we were not crossing languages I think we could require as input an iterable (something that can be invoked to generate a (potentially one-shot) iterator) instead of an iterator and then wouldn't have to have this distinction. This looks good but do we want to open a second JIRA for the post-12289 follow-up or just wait and do that work in this JIRA later?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on pull request #10070: ARROW-12231: [C++][Python][Dataset] Differentiate one-shot datasets
Posted by GitBox <gi...@apache.org>.
westonpace commented on pull request #10070:
URL: https://github.com/apache/arrow/pull/10070#issuecomment-832175816
> To me, this seems less like a subclass of dataset and more like a subclass of Scanner: IMHO it's not intuitive that a dataset would ever be single-shot. Instead, I think it'd make more sense to add Scanner::MakeFromRecordBatchReader or so, and (probably) add single-shot-ness to the contract of Scanner.
I'm not sure I agree. I agree with "it's not intuitive that a dataset would ever be single-shot". I don't agree that it makes any more sense for Scanner to be single-shot. I think the core non-intuitive piece is the concept of a "one-shot iterable".
In my mental model:
```
Dataset -> Iterable<Fragment>
Fragment -> Map<Fragment, Iterable<RecordBatch>>
Scanner -> Map<Dataset, Iterable<RecordBatch>>
```
So Scanner is just a "map" function which is generally (Python being the exception) reusable.
Perhaps I will revisit my original suggestion of having the input to dataset be an iterable (`InMemoryDataset::RecordBatchGenerator` is already sort of an "iterable" interface) and the in-memory variants are one-shot iterables. The user facing python API could remain as-is. list of batches, tables, or iterable of batches, tables would be converted into a `RecordBatchReader` and a one-shot implementation of `InMemoryDataset::RecordBatchGenerator` would consume the reader and then return an invalid status the next time `InMemoryDataset::RecordBatchGenerator::Get` is called.
Although that takes us back pretty close to where we started :grimacing:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #10070: ARROW-12231: [C++][Python][Dataset] Differentiate one-shot datasets
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10070:
URL: https://github.com/apache/arrow/pull/10070#issuecomment-821263146
https://issues.apache.org/jira/browse/ARROW-12231
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz closed pull request #10070: ARROW-12231: [C++][Python][Dataset] Isolate one-shot data to scanner
Posted by GitBox <gi...@apache.org>.
bkietz closed pull request #10070:
URL: https://github.com/apache/arrow/pull/10070
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on pull request #10070: ARROW-12231: [C++][Python][Dataset] Differentiate one-shot datasets
Posted by GitBox <gi...@apache.org>.
westonpace commented on pull request #10070:
URL: https://github.com/apache/arrow/pull/10070#issuecomment-832883714
So this would be a scanner that doesn't use fragments or datasets at all? Then would the python API change? Right now they pass batches/tables to the `dataset` function and then later get a scanner with `scan`.
So this change would be creating a scanner directly from a table/batches and bypassing the creation of a "dataset" entirely?
I think that makes a lot of sense.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jorisvandenbossche commented on pull request #10070: ARROW-12231: [C++][Python][Dataset] Isolate one-shot data to scanner
Posted by GitBox <gi...@apache.org>.
jorisvandenbossche commented on pull request #10070:
URL: https://github.com/apache/arrow/pull/10070#issuecomment-833325695
LGTM, the `Scanner.from_batches` instead of a single-shot dataset seems like a nice solution
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on pull request #10070: ARROW-12231: [C++][Python][Dataset] Differentiate one-shot datasets
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #10070:
URL: https://github.com/apache/arrow/pull/10070#issuecomment-821262718
CC @westonpace
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on pull request #10070: ARROW-12231: [C++][Python][Dataset] Differentiate one-shot datasets
Posted by GitBox <gi...@apache.org>.
westonpace commented on pull request #10070:
URL: https://github.com/apache/arrow/pull/10070#issuecomment-821404602
How would a flight stream even work with the datasets API? What would fragments be? How would it know when the file is ended? I think I need to fit this into my mental model.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on pull request #10070: ARROW-12231: [C++][Python][Dataset] Differentiate one-shot datasets
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #10070:
URL: https://github.com/apache/arrow/pull/10070#issuecomment-821422249
Sounds good to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on pull request #10070: ARROW-12231: [C++][Python][Dataset] Differentiate one-shot datasets
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #10070:
URL: https://github.com/apache/arrow/pull/10070#issuecomment-821396874
If the other side is something like a Flight stream, then I think we'd still have the distinction, unfortunately.
I am happy to wait until we have AsyncScanner merged and then I can update this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on pull request #10070: ARROW-12231: [C++][Python][Dataset] Differentiate one-shot datasets
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #10070:
URL: https://github.com/apache/arrow/pull/10070#issuecomment-827814625
@westonpace do you want to look over the ScanBatchesAsync implementation here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on pull request #10070: ARROW-12231: [C++][Python][Dataset] Differentiate one-shot datasets
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #10070:
URL: https://github.com/apache/arrow/pull/10070#issuecomment-821409962
Sorry, so this was originally added to support _writing_ data from a generator - which could be something like a Flight stream (=record batch reader). But writing data in Datasets consumes a scanner, so you end up having to support one-shot datasets. I agree supporting _reading_ data from Flight is an entirely different manner and would be modeled differently (presumably, as an iterable, as you suggest, corresponding to an RPC with a fixed set of parameters).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on pull request #10070: ARROW-12231: [C++][Python][Dataset] Differentiate one-shot datasets
Posted by GitBox <gi...@apache.org>.
westonpace commented on pull request #10070:
URL: https://github.com/apache/arrow/pull/10070#issuecomment-821414995
Ah, that's right. And for the writing from memory case we want to free up the memory after we write it so an iterable would be out of the question.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org