Skip to content

GH-47481: [C++][Acero] record_batch_reader_source specify Ordering::Implicit to support select * limit k#47482

Merged
zanmato1984 merged 3 commits into
apache:mainfrom
egolearner:batch_reader_source_order
May 24, 2026
Merged

GH-47481: [C++][Acero] record_batch_reader_source specify Ordering::Implicit to support select * limit k#47482
zanmato1984 merged 3 commits into
apache:mainfrom
egolearner:batch_reader_source_order

Conversation

@egolearner
Copy link
Copy Markdown
Contributor

@egolearner egolearner commented Sep 3, 2025

Rationale for this change

record_batch_reader_source does not support select * limit k as described in #47481

What changes are included in this PR?

record_batch_reader_source specify Ordering::Implicit

Are these changes tested?

yes

Are there any user-facing changes?

no

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Sep 3, 2025

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

See also:

@egolearner egolearner changed the title [C++][Acero] record_batch_reader_source specify Ordering::Implicit to support select * limit k GH-47481: [C++][Acero] record_batch_reader_source specify Ordering::Implicit to support select * limit k Sep 3, 2025
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Sep 3, 2025

⚠️ GitHub issue #47481 has been automatically assigned in GitHub to PR creator.

@egolearner
Copy link
Copy Markdown
Contributor Author

Hi @westonpace , please take a look

1 similar comment
@egolearner
Copy link
Copy Markdown
Contributor Author

Hi @westonpace , please take a look

@egolearner
Copy link
Copy Markdown
Contributor Author

ping @westonpace

@egolearner
Copy link
Copy Markdown
Contributor Author

Since @westonpace seems unavailable — @jonkeane @kou @raulcd could you please review this PR?

@egolearner egolearner force-pushed the batch_reader_source_order branch from ca21ef4 to cf69c13 Compare May 15, 2026 16:39
@egolearner
Copy link
Copy Markdown
Contributor Author

ping @westonpace @jonkeane @kou @raulcd

@kou
Copy link
Copy Markdown
Member

kou commented May 22, 2026

Can we add a test for this case?

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses GH-47481 by making record_batch_reader_source report an implicit (meaningful) ordering, allowing Acero’s fetch node (used for LIMIT/OFFSET) to accept it without requiring an explicit order_by.

Changes:

  • Mark RecordBatchReaderSourceNode output ordering as Ordering::Implicit() so fetch no longer rejects it as non-deterministic.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread cpp/src/arrow/acero/source_node.cc
@kou kou requested a review from zanmato1984 May 22, 2026 02:44
@egolearner
Copy link
Copy Markdown
Contributor Author

Can we add a test for this case?

UT added. Thx @kou .

zanmato1984
zanmato1984 approved these changes May 23, 2026
Copy link
Copy Markdown
Contributor

@zanmato1984 zanmato1984 left a comment

Choose a reason for hiding this comment

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

+1. I think this change aligns the ordering declaration of record_batch_reader_source with other similar sources (e.g. SchemaSourceNode). Thanks for working on this.

@github-actions github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels May 23, 2026
@zanmato1984 zanmato1984 merged commit 7c9d880 into apache:main May 24, 2026
59 of 61 checks passed
@zanmato1984 zanmato1984 removed the awaiting committer review Awaiting committer review label May 24, 2026
@conbench-apache-arrow
Copy link
Copy Markdown

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

There weren't enough matching historic benchmark results to make a call on whether there were regressions.

The full Conbench report has more details.

@egolearner egolearner deleted the batch_reader_source_order branch May 24, 2026 13:25
Mottl pushed a commit to Mottl/arrow that referenced this pull request May 26, 2026
…ing::Implicit to support `select * limit k` (apache#47482)

### Rationale for this change
record_batch_reader_source does not support `select * limit k` as described in apache#47481 

### What changes are included in this PR?
record_batch_reader_source specify Ordering::Implicit

### Are these changes tested?
yes

### Are there any user-facing changes?
no

* GitHub Issue: apache#47481

Lead-authored-by: egolearner <lijiliang@outlook.com>
Co-authored-by: egolearner <45122959+egolearner@users.noreply.github.com>
Co-authored-by: Rossi Sun <zanmato1984@gmail.com>
Signed-off-by: Rossi Sun <zanmato1984@gmail.com>
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.

4 participants