Skip to content

feat(spark): implement Spark width_bucket function#17331

Merged
comphead merged 11 commits into
apache:mainfrom
davidlghellin:feature/width_bucket
Sep 2, 2025
Merged

feat(spark): implement Spark width_bucket function#17331
comphead merged 11 commits into
apache:mainfrom
davidlghellin:feature/width_bucket

Conversation

@davidlghellin

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

part of #15914

Rationale for this change

Migrate spark functions from https://github.com/lakehq/sail/ to datafusion engine to unify codebase

What changes are included in this PR?

implement spark udf width_bucket
https://spark.apache.org/docs/latest/api/sql/index.html#width_bucket

Are these changes tested?

unit-tests and sqllogictests added

Are there any user-facing changes?

now can be called in queries

@github-actions github-actions Bot added sqllogictest SQL Logic Tests (.slt) spark labels Aug 26, 2025
}

fn output_ordering(&self, input: &[ExprProperties]) -> Result<SortProperties> {
// FIXME copy

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.

is it needed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this I am not sure if necesary output_ordering, for this I put this, only for these,

If its correct, drop de FIXME, thanks

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hi, I drop the FIXME

@comphead comphead 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.

Thanks @davidlghellin for intensive testing, checked the slt tests are in sync with expected results in https://spark.apache.org/docs/latest/api/sql/#width_bucket

Waiting for CI

@davidlghellin

Copy link
Copy Markdown
Contributor Author

Thanks @comphead , I'm checking this because I don't understand the errors I'm getting on my machine. I'm not sure if it's something related to my setup or if I missed something in the configuration.

5

query I
SELECT width_bucket(INTERVAL '0' DAY, INTERVAL '0' DAY, INTERVAL '10' DAY, 10)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

check interval

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'll take a look to verify these errors

@davidlghellin
davidlghellin requested a review from comphead August 28, 2025 14:46
return Ok(vec![Float64, Float64, Float64, Int32]);
}

let all_duration = matches!(v, Duration(_))

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.

its probably can be rewritten using pattern matching?

6

query I
SELECT width_bucket(10.0, 0.0, 0.0, 5)

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.

Can the test with null/nan be added? to use nan please check snippet

isnan(-'NaN'::FLOAT)
or
select greatest(arrow_cast('NAN','Float64'), arrow_cast('NAN','Float64'))

@davidlghellin
davidlghellin requested a review from comphead August 30, 2025 09:58

@comphead comphead 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.

Thanks @davidlghellin I think this PR is a good to go!

@comphead
comphead merged commit 3f70c9a into apache:main Sep 2, 2025
28 checks passed
@davidlghellin
davidlghellin deleted the feature/width_bucket branch September 22, 2025 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

spark sqllogictest SQL Logic Tests (.slt)

2 participants