Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correctly handle mysql timestamp() and datetime() types #96

Merged

Conversation

Sevenannn
Copy link
Contributor

@Sevenannn Sevenannn commented Sep 12, 2024

This PR includes the changes to correctly handle MySQL timestamp() and datetime() types when converting to arrow types.

  • MySQL timestamp(1-6) and timestamp() are all MYSQL_TYPE_TIMESTAMP
  • MySQL datetime(1-6) and datetime() are all MYSQL_TYPE_DATETIME
  • Use arrow TimestampMicrosecond type for mapping MYSQL_TYPE_TIMESTAMP and MYSQL_TYPE_DATETIME types, to handle up to microsecond precision for timestamp & datetime types
  • Simplify the logic for convert MYSQL_TYPE_TIMESTAMP and MYSQL_TYPE_DATETIME values to arrow TimestampMicrosecond value

Copy link
Contributor

@hozan23 hozan23 left a comment

Choose a reason for hiding this comment

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

Thanks @Sevenannn, nice work.

I wonder if we could add some tests here. Just to cover all ranging precision from 0-6 with the TIMESTAMP type.

A DATETIME or TIMESTAMP value can include a trailing fractional seconds part in up to microseconds (6 digits) precision. In particular, any fractional part in a value inserted into a DATETIME or TIMESTAMP column is stored rather than discarded.

https://dev.mysql.com/doc/refman/8.4/en/datetime.html

@Sevenannn
Copy link
Contributor Author

Thanks for the feedback! @hozan23

I added tests for MySQL TIMESTAMP and DATETIME types, and they are now running as part of the PR test.

@phillipleblanc phillipleblanc merged commit c8ff753 into datafusion-contrib:spiceai Sep 14, 2024
3 checks passed
@hozan23
Copy link
Contributor

hozan23 commented Sep 14, 2024

@phillipleblanc any reason not to merge this into the main branch?

@phillipleblanc
Copy link
Collaborator

It should be merged it into main. Once we (Spice AI) migrate to main from our spiceai branch (TBD), it will eventually be merged in. If you want to bring it in faster, I'm happy to review the PR.

This pull request was closed.
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.

3 participants