Skip to content

Add SortBuilder for SortOptions#9436

Draft
Banyc wants to merge 1 commit intoapache:mainfrom
Banyc:master
Draft

Add SortBuilder for SortOptions#9436
Banyc wants to merge 1 commit intoapache:mainfrom
Banyc:master

Conversation

@Banyc
Copy link
Copy Markdown

@Banyc Banyc commented Feb 18, 2026

Which issue does this PR close?

Rationale for this change

want to solve the obscure API problem described in apache/datafusion#20227

What changes are included in this PR?

the change added a new builder for the existing SortOptions

Are these changes tested?

yes and everything passes

Are there any user-facing changes?

yes.

there are new struct SortBuilder and its pub methods introduced

@github-actions github-actions bot added the arrow Changes to the arrow crate label Feb 18, 2026
@Banyc
Copy link
Copy Markdown
Author

Banyc commented Feb 18, 2026

Risk

should we add a builder pattern if we already have the

SortOptions { a: true, b: false }

construtor pattern?

@brunal
Copy link
Copy Markdown
Contributor

brunal commented Feb 20, 2026

How about a typestate pattern? I think that would:

  • prevent the need for a () struct item
  • Introduce a single struct instead of several
  • Allow specifying the 2 parameters in any order

Copy link
Copy Markdown
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thanks @Banyc

This PR needs:

  1. Test coverage (maybe doc example)
  2. Some explanation why this doesn't follow standard rust builder patterns (e.g. why do we need an intermediate state for an object with 2 fields)

Also, given that the SortOptions struct itself already has builder like methods, such as https://docs.rs/arrow/latest/arrow/compute/struct.SortOptions.html#method.asc can you please explain the value a separate builder adds?

Comment thread arrow-schema/src/lib.rs
/// Please refer to the docs for details
#[derive(Clone, Hash, Copy, Debug, Eq, PartialEq, Ord, PartialOrd)]
pub struct SortBuilder {
_force_construct_with_new: (),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What is the purpose of this field? I am not familiar witht his pattern

@alamb
Copy link
Copy Markdown
Contributor

alamb commented Mar 18, 2026

Thank you for the review @brunal

@alamb alamb marked this pull request as draft March 18, 2026 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

arrow Changes to the arrow crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants