Fixing issues with for timestamp literals #8193

comphead · 2023-11-15T17:05:45Z

Which issue does this PR close?

Closes partially #7958

Rationale for this change

During #7958 investigation it was revealed that timestamp conversion has bunch of inconsistencies with PG and DF itself

What changes are included in this PR?

This PR is intended to fix:

to_timestamp, ::timestamp, cast(n as timestamp) should be consistent
functions above should work for negative integer values
fuctions above should treat integers input as seconds, to be in sync with PG

Are these changes tested?

Oh yes

Are there any user-facing changes?

comphead · 2023-11-15T19:22:07Z

@waitingkuo cc

datafusion/sql/src/expr/mod.rs

alamb

Thank you for working on this item @comphead

It seems to me there is a potential problem with treating cast specially from other places with timestamps. I think it would be better if we treated all timestamps the same (either switch them to Timestamp::Seconds or Timestamp::Nanoseconds` for consisteny

datafusion/common/src/scalar.rs

datafusion/sql/src/expr/mod.rs

comphead · 2023-11-17T23:17:20Z

Thanks @alamb I agree any kind of cast to timestamp is supposed to have consistent signature. Currently its not.
Major engines have timestamps with microsecond precision, I can try to do it within current PR, however tbh it might be difficult to review.

comphead · 2023-11-20T19:06:10Z

not sure, why clippy is complaining.... locally its okay

comphead · 2023-11-20T19:22:05Z

@alamb I have aligned signatures, the solution is still suboptimal(using 2 casts).

In followup PR I would get rid of 2 casts and we need to support extreme low and high integer values, currently it overflows because Timestamp backed by i64 which is supposed to be microsecond rather than nanosecond.

I see 2 options to handle this:

Have a default precision as a microsecond(now its nanosecond), imho its preferred as PG or other major engines have a microsecond precision
Modify underlying value type for Timestamp from i64 to i128

@alamb let me know your thoughts.

PS. Not sure why clippy failed...

comphead · 2023-11-20T22:05:44Z

datafusion/sqllogictest/test_files/timestamps.slt

+Timestamp(Nanosecond, None) Timestamp(Nanosecond, None) Timestamp(Nanosecond, None)
+
+
+# verify extreme values (expects default precision to be microsecond instead of nanoseconds. Work pending)


Ive intentionally added this test to be uncommented once we figure out the overflow solution

comphead · 2023-11-21T01:24:43Z

@alamb please re review once you have some free time

alamb

Thank you @comphead -- I am worried about this PR potentially messing up downstream users of datafusion (e.g. IOx) and makes the timestamp type inconsistent within datafusion (I realize currently the timestamp type is inconsitent with what is done with postgres, etc).

If the goal is to be able to read data from Parquet that is stored as INT96 correctly as a timestamp, I am not sure why we are changing timetamp handling in DataFusion)

Perhaps a better approach would be to investigate updating the arrow-rs reader to read whatever spark generates as a proper timestamp type (@avantgardnerio hints at that in #7958 (comment)) 🤔

datafusion/expr/src/built_in_function.rs

alamb · 2023-11-21T19:55:06Z

datafusion/sql/tests/sql_integration.rs

-    let expected = "Projection: person.id, person.first_name, person.last_name\
-            \n  Filter: person.state = Utf8(\"CO\") AND person.age >= Int64(21) AND person.age <= Int64(65)\
-            \n    TableScan: person";
+    let expected = "Projection: person.id, person.first_name, person.last_name\n  Filter: person.state = Utf8(\"CO\") AND person.age >= Int64(21) AND person.age <= Int64(65)\n    TableScan: person";


are these tests actually different? They look the same to me except the formatting changed

I think it would be good to revert these changes or reformat them if there is a difference, to make the diff clearer

datafusion/sql/src/expr/mod.rs

comphead · 2023-11-21T20:55:51Z

Thanks @alamb this is my fault, I referenced too much problems and brought up the confusion. This particular PR has nothing to do with INT96 and Parquet, it relates to timestamp literals only.

It fixes next problems

to handle integer inputs the same way as PG, DuckDB, Spark, etc
negative ops extended for timestamps
align all timestamp syntaxes to return consistent result and same datatypes.

I have returned the signature to be NanoSecond like it was before, and now everything timestamp related in DF is consistent.
We still have problems with overflows on extreme high/low values which can be fixed with either:

change default timestamp precision to microsecond instead of nanosecond, this might be huge work involved, as all timestamp arithmetics refers to NanoSeconds
change Timestamp underlying storage to i128 instead of i64

alamb · 2023-11-21T21:57:22Z

Seems reasonable to me -- thank you @comphead. Does this revert the behavior added to close #2979 (to align with postgres behavior?) Or does it just change to_timestamp to return a type of TimestampNanos (though the argument is still interpreted as seconds)?

-- @waitingkuo do you have a few moments to give this PR a look?

comphead · 2023-11-22T01:05:38Z

Seems reasonable to me -- thank you @comphead. Does this revert the behavior added to close #2979 (to align with postgres behavior?) Or does it just change to_timestamp to return a type of TimestampNanos (though the argument is still interpreted as seconds)?

-- @waitingkuo do you have a few moments to give this PR a look?

It doesn't revert the behavior, the input still treated as seconds, but the return type is nanoseconds to keep DataFusion consistent with all timestamp arithmetics and functions.

alamb · 2023-11-22T11:34:19Z

It doesn't revert the behavior, the input still treated as seconds, but the return type is nanoseconds to keep DataFusion consistent with all timestamp arithmetics and functions.

Ah, sorry for my confusion

alamb

Thank you @comphead for bearing with me. I took another look and pass through this PR and it makes sense to me. Thank you for your focus on detail

I left some suggestions for improvements, but I don't think they are necessary prior to merging this pR

datafusion/sql/src/expr/mod.rs

datafusion/sqllogictest/test_files/timestamps.slt

alamb · 2023-11-22T11:41:30Z

datafusion/sql/tests/sql_integration.rs

-    let expected = "Projection: person.id, person.first_name, person.last_name\
-            \n  Filter: person.state = Utf8(\"CO\") AND person.age >= Int64(21) AND person.age <= Int64(65)\
-            \n    TableScan: person";
+    let expected = "Projection: person.id, person.first_name, person.last_name\n  Filter: person.state = Utf8(\"CO\") AND person.age >= Int64(21) AND person.age <= Int64(65)\n    TableScan: person";


I think it would be good to revert these changes or reformat them if there is a difference, to make the diff clearer

comphead · 2023-11-22T22:45:58Z

Addressed all comments

comphead · 2023-11-23T22:15:36Z

Should I merge this PR @alamb ?

alamb · 2023-11-26T11:58:03Z

Should I merge this PR @alamb ?

Yes, please do so!

It is one of my goals for the year to grow the capacity for review / merging PRs (so as to avoid single-person bottlenecks, ahem...)

As long as the guidelines described here are followed any committer should feel free to merge approved PRs:
https://arrow.apache.org/datafusion/contributor-guide/index.html#pull-request-overview

alamb · 2023-11-27T12:04:09Z

🎉

comphead marked this pull request as draft November 15, 2023 17:05

github-actions bot added sql physical-expr Physical Expressions sqllogictest labels Nov 15, 2023

comphead changed the title ~~timestamp fixes~~ Fixing issues with timestamp cast Nov 15, 2023

comphead marked this pull request as ready for review November 15, 2023 19:21

comphead requested review from viirya and alamb November 15, 2023 19:21

Weijun-H reviewed Nov 15, 2023

View reviewed changes

datafusion/sql/src/expr/mod.rs Outdated Show resolved Hide resolved

This was referenced Nov 16, 2023

fix: Timestamp with timezone not considered join on #8150

Merged

Double type argument for to_timestamp function #8159

Merged

alamb reviewed Nov 17, 2023

View reviewed changes

datafusion/common/src/scalar.rs Show resolved Hide resolved

datafusion/sql/src/expr/mod.rs Outdated Show resolved Hide resolved

comphead marked this pull request as draft November 18, 2023 20:45

alamb mentioned this pull request Nov 20, 2023

DataFusion weekly project plan (Andrew Lamb) - Nov 20, 2023 #8274

Closed

7 tasks

github-actions bot added logical-expr Logical plan and expressions core Core datafusion crate labels Nov 20, 2023

comphead marked this pull request as ready for review November 20, 2023 19:17

comphead requested a review from alamb November 20, 2023 19:18

comphead commented Nov 20, 2023

View reviewed changes

comphead force-pushed the dev branch from 4afca6a to 8085383 Compare November 20, 2023 22:09

alamb reviewed Nov 21, 2023

View reviewed changes

comphead requested a review from alamb November 21, 2023 20:56

comphead changed the title ~~Fixing issues with timestamp cast~~ Fixing issues with for timestamp literals Nov 21, 2023

alamb approved these changes Nov 22, 2023

View reviewed changes

comphead added 9 commits November 26, 2023 13:49

timestamp fixes

728e9cf

fmt

ecb8625

fix test

5fc8524

fix test

a0f1c95

rm dbg

71a8bfc

sync signature

8813915

add descr

7fe27df

add descr

ccb276d

minors

bfea80c

comphead force-pushed the dev branch from 7ee9f9b to bfea80c Compare November 26, 2023 22:00

comphead merged commit 2071259 into apache:main Nov 26, 2023
22 checks passed

comphead mentioned this pull request Nov 30, 2023

Minor: small refactoring for ints/floats to timestamps conversions #8370

Closed

matthewgapp mentioned this pull request Jan 11, 2024

matt/feat/recursive ctes/config flag matthewgapp/arrow-datafusion#3

Closed

mhilton mentioned this pull request Feb 21, 2024

[cloud/v3] Casting BIGINT to TIMESTAMP doesn't seem to work anymore influxdata/influxdb#24581

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing issues with for timestamp literals #8193

Fixing issues with for timestamp literals #8193

comphead commented Nov 15, 2023 •

edited

comphead commented Nov 15, 2023

alamb left a comment •

edited

comphead commented Nov 17, 2023

comphead commented Nov 20, 2023

comphead commented Nov 20, 2023

comphead Nov 20, 2023

comphead commented Nov 21, 2023

alamb left a comment

alamb Nov 21, 2023

alamb Nov 22, 2023

comphead commented Nov 21, 2023 •

edited

alamb commented Nov 21, 2023

comphead commented Nov 22, 2023

alamb commented Nov 22, 2023

alamb left a comment

alamb Nov 22, 2023

comphead commented Nov 22, 2023

comphead commented Nov 23, 2023

alamb commented Nov 26, 2023

alamb commented Nov 27, 2023

		Timestamp(Nanosecond, None) Timestamp(Nanosecond, None) Timestamp(Nanosecond, None)


		# verify extreme values (expects default precision to be microsecond instead of nanoseconds. Work pending)

Fixing issues with for timestamp literals #8193

Fixing issues with for timestamp literals #8193

Conversation

comphead commented Nov 15, 2023 • edited

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

comphead commented Nov 15, 2023

alamb left a comment • edited

Choose a reason for hiding this comment

comphead commented Nov 17, 2023

comphead commented Nov 20, 2023

comphead commented Nov 20, 2023

comphead Nov 20, 2023

Choose a reason for hiding this comment

comphead commented Nov 21, 2023

alamb left a comment

Choose a reason for hiding this comment

alamb Nov 21, 2023

Choose a reason for hiding this comment

alamb Nov 22, 2023

Choose a reason for hiding this comment

comphead commented Nov 21, 2023 • edited

alamb commented Nov 21, 2023

comphead commented Nov 22, 2023

alamb commented Nov 22, 2023

alamb left a comment

Choose a reason for hiding this comment

alamb Nov 22, 2023

Choose a reason for hiding this comment

comphead commented Nov 22, 2023

comphead commented Nov 23, 2023

alamb commented Nov 26, 2023

alamb commented Nov 27, 2023

comphead commented Nov 15, 2023 •

edited

alamb left a comment •

edited

comphead commented Nov 21, 2023 •

edited