Skip to content

[ICU] Add casts from Timestamp_* to TimestampTZ#9539

Merged
Mytherin merged 30 commits intoduckdb:mainfrom
Tishj:timestamp_unit_to_tz
Feb 15, 2024
Merged

[ICU] Add casts from Timestamp_* to TimestampTZ#9539
Mytherin merged 30 commits intoduckdb:mainfrom
Tishj:timestamp_unit_to_tz

Conversation

@Tishj
Copy link
Contributor

@Tishj Tishj commented Nov 1, 2023

This PR fixes #9532

What we essentially do is just perform value::TIMESTAMP::TIMESTAMP_TZ where value::TIMESTAMP_TZ is requested.

@Tishj Tishj requested a review from hawkfish November 1, 2023 12:01
Copy link
Contributor

@hawkfish hawkfish left a comment

Choose a reason for hiding this comment

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

Code looks great! Can we have some pure SQL tests that don't require pandas?

@Tishj
Copy link
Contributor Author

Tishj commented Nov 1, 2023

Of course it couldn't be this simple 🙃

INTERNAL Error: Failed execute during verify: Statistics mismatch: value is smaller than min.
Statistics: [Min: 1990-12-21 00:00:00+00, Max: 1990-12-21 00:00:00+00][Has Null: false, Has No Null: true][Approx Unique: 1]
Vector: FLAT TIMESTAMP WITH TIME ZONE: 1 = [ 1990-12-20 23:00:00+00]

Because the conversion to TIMESTAMP_TZ is transformative, it fails to verify because the stats are out of date

	//! Expression statistics (if any) - ONLY USED FOR VERIFICATION
	unique_ptr<BaseStatistics> verification_stats;

🙃

Fixed it by just disabling the verification for the test

@Tishj
Copy link
Contributor Author

Tishj commented Nov 2, 2023

Interesting behavior?

D select make_timestamp(-9223372036854775806);
Error: Conversion Error: Date out of range in timestamp conversion

Replacing every value with a 0 until it succeeds:

┌──────────────────────────────────────┐
│ make_timestamp(-9223372000000000000) │
│              timestamp               │
├──────────────────────────────────────┤
│ 290309-12-22 (BC) 06:13:20           │
└──────────────────────────────────────┘

I assume this is the minimum then?

D select make_timestamp(-999999999999999999);
┌─────────────────────────────────────┐
│ make_timestamp(-999999999999999999) │
│              timestamp              │
├─────────────────────────────────────┤
│ 29720-04-05 (BC) 22:13:20.000001    │
└─────────────────────────────────────┘

@Tishj Tishj marked this pull request as ready for review November 2, 2023 11:04
@Tishj Tishj requested a review from hawkfish November 2, 2023 15:56
@github-actions github-actions bot marked this pull request as draft November 3, 2023 09:55
@Tishj Tishj marked this pull request as ready for review November 3, 2023 10:13
@github-actions github-actions bot marked this pull request as draft November 7, 2023 08:56
@Tishj Tishj marked this pull request as ready for review November 7, 2023 08:57
@github-actions github-actions bot marked this pull request as draft November 8, 2023 09:12
@Tishj Tishj marked this pull request as ready for review November 8, 2023 09:14
Copy link
Collaborator

@Mytherin Mytherin left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Looks good - some comments. Perhaps @hawkfish can also take another look.

@github-actions github-actions bot marked this pull request as draft November 10, 2023 09:52
Copy link
Contributor

@hawkfish hawkfish left a comment

Choose a reason for hiding this comment

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

The Try... is a good addition. I still think the extra IsFinite calls are unnecessary, although the assert is a good idea.

@Mytherin Mytherin marked this pull request as ready for review January 8, 2024 13:31
@Mytherin
Copy link
Collaborator

Mytherin commented Jan 8, 2024

Thanks! There seems to be some CI failures remaining - could you have a look?

@github-actions github-actions bot marked this pull request as draft January 10, 2024 10:01
@Tishj Tishj marked this pull request as ready for review February 14, 2024 09:53
@github-actions github-actions bot marked this pull request as draft February 14, 2024 19:26
@Tishj Tishj marked this pull request as ready for review February 14, 2024 19:26
@Tishj Tishj requested a review from Mytherin February 15, 2024 11:26
@Mytherin Mytherin merged commit f1532bf into duckdb:main Feb 15, 2024
@Mytherin
Copy link
Collaborator

Thanks! LGTM

krlmlr added a commit to duckdb/duckdb-r that referenced this pull request Mar 15, 2024
Merge pull request duckdb/duckdb#10658 from hannes/csvpathlength
Merge pull request duckdb/duckdb#10756 from Mytherin/preserveinsertionordermemory
Merge pull request duckdb/duckdb#10746 from samansmink/enable-azure-autoload
Merge pull request duckdb/duckdb#10747 from maiadegraaf/list_reverse_bug
Merge pull request duckdb/duckdb#10748 from taniabogatsch/capi-tests
Merge pull request duckdb/duckdb#10739 from peterboncz/pb/immmedate_mode_only_in_non_autocommit
Merge pull request duckdb/duckdb#10688 from Tmonster/union_exclude
Merge pull request duckdb/duckdb#10710 from samansmink/comment-on-column
Merge pull request duckdb/duckdb#10725 from hawkfish/fuzzer-preceding-frame
Merge pull request duckdb/duckdb#10723 from hawkfish/fuzzer-null-timestamp
Merge pull request duckdb/duckdb#10436 from taniabogatsch/map-fixes
Merge pull request duckdb/duckdb#10587 from kryonix/main
Merge pull request duckdb/duckdb#10738 from TinyTinni/fix-assert-in-iscntrl
Merge pull request duckdb/duckdb#10708 from carlopi/ci_fixes
Merge pull request duckdb/duckdb#10726 from hawkfish/fuzzer-to-weeks
Merge pull request duckdb/duckdb#10727 from hawkfish/fuzzer-window-bind
Merge pull request duckdb/duckdb#10733 from TinyTinni/remove-static-string
Merge pull request duckdb/duckdb#10715 from Tishj/python_tpch_regression_rework
Merge pull request duckdb/duckdb#10728 from hawkfish/fuzzer-argminmax-decimal
Merge pull request duckdb/duckdb#10717 from carlopi/fix_extension_deploy
Merge pull request duckdb/duckdb#10694 from Mytherin/castquerylocation
Merge pull request duckdb/duckdb#10448 from peteraisher/feature/use-assertThrows-for-jdbc-tests
Merge pull request duckdb/duckdb#10691 from Mytherin/issue10685
Merge pull request duckdb/duckdb#10684 from Mytherin/distincton
Merge pull request duckdb/duckdb#9539 from Tishj/timestamp_unit_to_tz
Merge pull request duckdb/duckdb#10341 from Tmonster/tpch_ingestion_benchmark
Merge pull request duckdb/duckdb#10689 from Mytherin/juliaversion
Merge pull request duckdb/duckdb#10669 from Mytherin/skippedtests
Merge pull request duckdb/duckdb#10679 from Tishj/reenable_window_rows_overflow
Merge pull request duckdb/duckdb#10672 from carlopi/wasm_extensions_ci
Merge pull request duckdb/duckdb#10660 from szarnyasg/update-storage-info-for-v0100
Merge pull request duckdb/duckdb#10643 from bleskes/duck_transaction_o11y
Merge pull request duckdb/duckdb#10654 from carlopi/fix_10548
Merge pull request duckdb/duckdb#10650 from hannes/noprintf
Merge pull request duckdb/duckdb#10649 from Mytherin/explicitenumnumbers
@Tishj Tishj deleted the timestamp_unit_to_tz branch November 7, 2025 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Can no longer cast Pandas Timestamps to timestamptz in 0.9

4 participants