Skip to content

Conversation

chenkovsky
Copy link
Contributor

Which issue does this PR close?

  • Closes #.

Rationale for this change

What changes are included in this PR?

implement spark rint udf

Are these changes tested?

UT

Are there any user-facing changes?

No

@chenkovsky chenkovsky marked this pull request as ready for review July 26, 2025 12:19
@github-actions github-actions bot added sqllogictest SQL Logic Tests (.slt) spark labels Jul 26, 2025
Copy link
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.

Looks good to me

@shehabgamin and @comphead in case you would like a chance to review too

@alamb
Copy link
Contributor

alamb commented Jul 27, 2025

Thank you for driving this @chenkovsky

Copy link
Contributor

@shehabgamin shehabgamin left a comment

Choose a reason for hiding this comment

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

Just 1 comment. I think it's okay to merge as is and iterate further though!

pub fn new() -> Self {
Self {
signature: Signature::one_of(
vec![Exact(vec![Float64]), Exact(vec![Float32])],
Copy link
Contributor

Choose a reason for hiding this comment

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

According to these docs, the input type might be a little bit more flexible?
https://docs.databricks.com/aws/en/sql/language-manual/functions/rint#arguments

Copy link
Contributor Author

@chenkovsky chenkovsky Jul 29, 2025

Choose a reason for hiding this comment

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

I added more types. please review it again


fn output_ordering(&self, input: &[ExprProperties]) -> Result<SortProperties> {
// round preserves the order of the first argument
let value = &input[0];
Copy link
Contributor

Choose a reason for hiding this comment

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

it is good to check is not empy

Copy link
Contributor

@comphead comphead left a comment

Choose a reason for hiding this comment

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

lgtm thanks @chenkovsky, some minors

@alamb alamb merged commit 677f723 into apache:main Jul 30, 2025
27 checks passed
@alamb
Copy link
Contributor

alamb commented Jul 30, 2025

Thanks @chenkovsky and @comphead and @shehabgamin

Standing-Man pushed a commit to Standing-Man/datafusion that referenced this pull request Aug 4, 2025
* feat: spark rint

* support all numeric

* update

* check empty for output ordering
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants