Skip to content

Conversation

@amoeba
Copy link
Member

@amoeba amoeba commented Jun 6, 2025

Rationale for this change

Our docs say you can construct a Dataset from a RecordBatchReader but you can't. While we can't pass the actual RecordBatchReader to the Dataset as a source (AFAIK), we can at least consume the reader immediately and create an InMemoryDataset from the batches.

What changes are included in this PR?

  • Tweaked type checks so this now works (both from ds.dataset and ds.InMemoryDataset)
  • Test case extended to cover the new behavior
  • Tweaked error message just to use proper case

Are these changes tested?

Yes.

Are there any user-facing changes?

No.

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting review Awaiting review labels Jun 6, 2025
@amoeba amoeba merged commit 1969ae3 into apache:main Jun 6, 2025
13 of 15 checks passed
@amoeba amoeba removed the awaiting merge Awaiting merge label Jun 6, 2025
@amoeba
Copy link
Member Author

amoeba commented Jun 6, 2025

Thanks for quick reviews.

@conbench-apache-arrow
Copy link

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 1969ae3.

There were 66 benchmark results with an error:

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them.

alinaliBQ pushed a commit to Bit-Quill/arrow that referenced this pull request Jun 17, 2025
…rdBatchReader (apache#46731)

### Rationale for this change

Our docs say you can construct a Dataset from a RecordBatchReader but you can't. While we can't pass the actual RecordBatchReader to the Dataset as a source (AFAIK), we can at least consume the reader immediately and create an InMemoryDataset from the batches.

### What changes are included in this PR?

- Tweaked type checks so this now works (both from ds.dataset and ds.InMemoryDataset)
- Test case extended to cover the new behavior
- Tweaked error message just to use proper case

### Are these changes tested?

Yes.

### Are there any user-facing changes?

No.
* GitHub Issue: apache#46729

Authored-by: Bryce Mecum <[email protected]>
Signed-off-by: Bryce Mecum <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants