Skip to content

feat: expose centroids in approx_percentile_cont fluent api - #11878

Merged
alamb merged 3 commits into
apache:mainfrom
Michael-J-Ward:expose-centroids
Aug 8, 2024
Merged

feat: expose centroids in approx_percentile_cont fluent api#11878
alamb merged 3 commits into
apache:mainfrom
Michael-J-Ward:expose-centroids

Conversation

@Michael-J-Ward

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Closes#11877.

Rationale for this change

The UDAF approx_percentile_cont has a third optional argument that the fluent api does not currently expose.

This PR add the 3rd optional argument, consistent with how array_slice and regexp_* fluent APIs expose optional arguments.

What changes are included in this PR?

Add centroids: Option<Expr> to the fluent API.

Are these changes tested?

Yes.

Are there any user-facing changes?

Yes. Any user of the approx_percentile_cont fluent API will need to add None for the optional argument.

@github-actionsgithub-actionsBot added the core Core DataFusion crate label Aug 7, 2024

@jayzhan211jayzhan211 left a comment

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.

👍

Comment threaddatafusion/functions-aggregate/src/approx_percentile_cont.rs Outdated
assert_batches_eq!(expected, &batches);

// with number of centroids set
let expr = approx_percentile_cont(col("b"), lit(0.5), Some(lit(2)));

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

NOTE: I don't have SQL Server, so I did not verify that it produces this result.

I chose a choice for centroids that produced a different result than the default.

@Michael-J-WardMichael-J-Ward changed the title feat: expose centroids in approx_percentile_count fluent apifeat: expose centroids in approx_percentile_cont fluent apiAug 8, 2024
@alambalamb added the api change Changes the API exposed to users of the crate label Aug 8, 2024
@alamb

alamb commented Aug 8, 2024

Copy link
Copy Markdown
Contributor

I think technically speaking this is an API change as there is a new parameter, so I marked the PR thusly

@alamb
alamb merged commit 56f8e35 into apache:mainAug 8, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api changeChanges the API exposed to users of the cratecoreCore DataFusion crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Expose optional num_centroids argument in approx_percentile_cont's fluent API

3 participants

@Michael-J-Ward@alamb@jayzhan211