Skip to content

Conversation

@Omega359
Copy link
Contributor

@Omega359 Omega359 commented Dec 7, 2025

Which issue does this PR close?

Rationale for this change

There wasn't a good way to make a time from component parts.

What changes are included in this PR?

Code, test, docs

Are these changes tested?

Yes

Are there any user-facing changes?

New function.

@github-actions github-actions bot added documentation Improvements or additions to documentation sqllogictest SQL Logic Tests (.slt) functions Changes to functions implementation labels Dec 7, 2025
@Omega359 Omega359 marked this pull request as ready for review December 7, 2025 18:42
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.

Thanks @Omega359 the PR is LGTM, 1 nit on datatype for time constituents which can be unsigned ints.

Copy link
Contributor

@Jefffrey Jefffrey left a comment

Choose a reason for hiding this comment

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

Had a quick shot at some potential refactors to see if we can simplify some of the logic here a bit; feel free to disregard, especially as they are still rough and affect some of the tests here, but I think it's worth looking into (if not now then potentially in the future).

@Omega359
Copy link
Contributor Author

Omega359 commented Dec 9, 2025

Not sure what to do about the cargo fmt error - the PR output is what cargo fmt results in locally.

bruce@devbox:/opt/dev/datafusion$ rustup show
Default host: x86_64-unknown-linux-gnu
rustup home:  /home/bruce/.rustup

installed toolchains
--------------------
stable-x86_64-unknown-linux-gnu (default)
1.89.0-x86_64-unknown-linux-gnu
1.90.0-x86_64-unknown-linux-gnu
1.91.0-x86_64-unknown-linux-gnu (active)

active toolchain
----------------
name: 1.91.0-x86_64-unknown-linux-gnu
active because: overridden by '/opt/dev/datafusion/rust-toolchain.toml'
installed targets:
  x86_64-unknown-linux-gnu

Seems very odd.

@comphead
Copy link
Contributor

comphead commented Dec 9, 2025

Not sure what to do about the cargo fmt error - the PR output is what cargo fmt results in locally.

bruce@devbox:/opt/dev/datafusion$ rustup show
Default host: x86_64-unknown-linux-gnu
rustup home:  /home/bruce/.rustup

installed toolchains
--------------------
stable-x86_64-unknown-linux-gnu (default)
1.89.0-x86_64-unknown-linux-gnu
1.90.0-x86_64-unknown-linux-gnu
1.91.0-x86_64-unknown-linux-gnu (active)

active toolchain
----------------
name: 1.91.0-x86_64-unknown-linux-gnu
active because: overridden by '/opt/dev/datafusion/rust-toolchain.toml'
installed targets:
  x86_64-unknown-linux-gnu

Seems very odd.

This is so weird issue, sometime cargo clean helps, otherwise rust total reinstall what helped me

@Omega359
Copy link
Contributor Author

This is so weird issue, sometime cargo clean helps, otherwise rust total reinstall what helped me

I've noticed cargo fmt bouncing around the ordering of imports in 1.91 in my other projects. It's rather annoying honestly and feels like needless bikeshedding.

@Omega359
Copy link
Contributor Author

Omega359 commented Dec 10, 2025

More info: the only way I could get rust fmt to output the use statements in the same order as ci was to force the datafusion-functions crate to use the 2024 edition (vs workspace now which is edition 2021). This feels like a cargo bug ...

More info: https://doc.rust-lang.org/edition-guide/rust-2024/rustfmt-version-sorting.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation functions Changes to functions implementation sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants