Skip to content

Conversation

@mjgarton
Copy link
Contributor

Instead of assuming we want Time64Microsecond when converting from NaiveType, convert based on the interenced type.

Update the get_time*_value functions to return NaiveTime instead of NaiveDateTime. Previously they were returning None, because that's what as_datetime always returns for Time32 & Time64 values.

@mjgarton
Copy link
Contributor Author

I am not sure whether this logic is worth adding tests for. I hope to consider adding tests some time later this week.

Instead of assuming we want `Time64Microsecond` when converting from
NaiveType, convert based on the interenced type.

Update the `get_time*_value` functions to return `NaiveTime` instead of
`NaiveDateTime`.  Previously they were returning None, because that's
what `as_datetime` always returns for Time32 & Time64 values.
Add tests for the `get_time*_value` functions.
Add test for deserialising time parameters, and fix an implementation
bug found by the test.
@mjgarton
Copy link
Contributor Author

Tests added now & fixed a bug in my initial changes. I think this is ready to merge now.

Copy link
Member

@sunng87 sunng87 left a comment

Choose a reason for hiding this comment

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

Thank you! @mjgarton

@sunng87 sunng87 merged commit 819bdae into datafusion-contrib:master Nov 20, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants