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

feat: Cater for ExtractEpochSeconds overflowing integer #1397

Merged

Conversation

nj1973
Copy link
Contributor

@nj1973 nj1973 commented Jan 14, 2025

This PR fixes ExtractEpochSeconds for PostgreSQL, Teradata and SQL Server.

It also adds tests for large ranges of values for ExtractEpochSeconds to have to deal with.

@nj1973 nj1973 marked this pull request as ready for review January 14, 2025 21:56
@nj1973 nj1973 requested a review from a team as a code owner January 14, 2025 21:56
Copy link
Collaborator

@sundar-mudupalli-work sundar-mudupalli-work left a comment

Choose a reason for hiding this comment

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

Could you please add tests for MySQL max timestamp.

third_party/ibis/ibis_addon/operations.py Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would have loved to have some test data with the max MySQL Timestamp - i.e. 3001-01-19 03:14:07.999999 - one way I suggest is to put it in its own row and filter it out using --fliter for those engines that it does not make sense I am wondering - might we have an issue like 1396, where the combiner has a fit.

Thank you.

Sundar Mudupalli

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, you were absolutely right that MySQL falls into the same issue as other engines for min/max. I made a mistake when excluding the low/high columns, the limitation only applies to SUM.

I've rectified this mistake. Good spot.

@nj1973
Copy link
Contributor Author

nj1973 commented Jan 21, 2025

/gcbrun

@renzokuken renzokuken self-requested a review January 22, 2025 17:13
Copy link
Collaborator

@renzokuken renzokuken 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 for re-organizing the addon library as well

@nj1973 nj1973 dismissed sundar-mudupalli-work’s stale review January 22, 2025 18:02

Mike reviewed during vacation

@nj1973
Copy link
Contributor Author

nj1973 commented Jan 22, 2025

/gcbrun

@nj1973 nj1973 merged commit c175146 into develop Jan 22, 2025
5 checks passed
@nj1973 nj1973 deleted the 1390-validate-column-postgresql-epoch-seconds-exception branch January 22, 2025 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

validate column: PostgreSQL exception with --wildcard-include-timestamp and old timestamps
4 participants