Skip to content

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

Open
egolearner wants to merge 3 commits into
apache:mainfrom
egolearner:batch_reader_source_order
Open

GH-47481: [C++][Acero] record_batch_reader_source specify Ordering::Implicit to support select * limit k#47482
egolearner wants to merge 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants