support to_timestamp with optional chrono formats #8886

Omega359 · 2024-01-16T20:05:20Z

Which issue does this PR close?

Closes 5398.

Rationale for this change

Adding flexibility for parsing timestamp strings

What changes are included in this PR?

Code, tests and user guide updates.

Are these changes tested?

Tests were added and pass.

Are there any user-facing changes?

dataframe.to_timestamp(..) changed from to_timestamp(col("a")) to to_timestamp(vec![col("a")])

…ormats

…ation apache#5398

datafusion-examples/examples/dataframe_to_timestamp.rs

comphead

Thanks @Omega359 I'll go through it tomorrow if no one else beats me

…_formats

datafusion/sqllogictest/test_files/timestamps.slt

comphead · 2024-01-18T00:21:53Z

datafusion/sqllogictest/test_files/timestamps.slt

+
+# verify timestamp data with formatting options
+query PPPPPP
+SELECT to_timestamp(null, '%+'), to_timestamp(0, '%s'), to_timestamp(1926632005, '%s'), to_timestamp(1, '%+', '%s'), to_timestamp(-1, '%c', '%+', '%s'), to_timestamp(0-1, '%c', '%+', '%s')


@alamb as you are the ticket initiator, is it a valid query? I checked https://www.postgresql.org/docs/current/functions-formatting.html I dont see examples like that

These queries are just mirrors of the existing tests that were there for to_timestamp without formatting options.

I think the key observation here is that this PR implements different semantics than any existing to_timestamp (it isn't postgres format strings, nor is it spark format strings, it is something datafusion specific based on the rust chrono format strings)

alamb

Thank you @Omega359 -- I started going through this PR and it looks quite nice 👌

I haven't completed by review of datafusion/physical-expr/src/datetime_expressions.rs yet but I plan to do so later today or early tomorrow.

cc @waitingkuo @jhorstmann and @waynexia / @gruuya for your comments

alamb · 2024-01-18T14:07:53Z

datafusion-examples/examples/dataframe_to_timestamp.rs

+use datafusion::prelude::*;
+use datafusion_common::assert_contains;
+
+/// This example demonstrates how to use the to_timestamp function in the DataFrame API as well as via sql.


This is really nice 👌

datafusion/expr/src/expr_fn.rs

datafusion/expr/src/built_in_function.rs

datafusion/sqllogictest/test_files/timestamps.slt

alamb · 2024-01-18T14:16:13Z

datafusion/sqllogictest/test_files/timestamps.slt

+
+# verify timestamp data with formatting options
+query PPPPPP
+SELECT to_timestamp(null, '%+'), to_timestamp(0, '%s'), to_timestamp(1926632005, '%s'), to_timestamp(1, '%+', '%s'), to_timestamp(-1, '%c', '%+', '%s'), to_timestamp(0-1, '%c', '%+', '%s')


I think the key observation here is that this PR implements different semantics than any existing to_timestamp (it isn't postgres format strings, nor is it spark format strings, it is something datafusion specific based on the rust chrono format strings)

alamb

This is great -- thank you @Omega359 🙏

I think we just need some additional test coverage and this PR is ready to go from my perspective

datafusion/physical-expr/src/datetime_expressions.rs

gruuya

This looks extremely useful, thanks!

datafusion/physical-expr/src/datetime_expressions.rs

comphead

Thanks @alamb I would probably stick to PG standard as we declared. The PR implementation is really cool, and we may want to have a separate function for it? like from_chrono or something

datafusion/physical-expr/src/datetime_expressions.rs

Omega359 · 2024-01-18T21:03:33Z

Thanks @alamb I would probably stick to PG standard as we declared. The PR implementation is really cool, and we may want to have a separate function for it? like from_chrono or something

I had a thought for the future to allow for setting the format based on either config or based on dialect. I was thinking that might be best left to after the function refactor (#8045) is done then have separate implementations if desired. I just didn't want to write a parser for the PG syntax as it's pretty detailed and just wanted to get something out there. Someone else noted the complexity of implementing the PG syntax

Do you have happen to have such a parser available?

… without valid formats.

- Extracted out to_timestamp_impl method to reduce code duplication as per PR feedback. - Extracted out validate_to_timestamp_data_types to reduce code duplication as per PR feedback. - Added additional tests for argument validation and invalid arguments. - Removed unnecessary shim function 'string_to_timestamp_nanos_with_format_shim'

comphead · 2024-01-18T21:58:26Z

Thanks @alamb I would probably stick to PG standard as we declared. The PR implementation is really cool, and we may want to have a separate function for it? like from_chrono or something

I had a thought for the future to allow for setting the format based on either config or based on dialect. I was thinking that might be best left to after the function refactor (#8045) is done then have separate implementations if desired. I just didn't want to write a parser for the PG syntax as it's pretty detailed and just wanted to get something out there. Someone else noted the complexity of implementing the PG syntax

Do you have happen to have such a parser available?

I'm not sure if its available, but we prob use your approach to create an adapter to PG formatting? 🤔
So you map PG formatting rules into chrono formatting rules

Omega359 · 2024-01-19T04:21:48Z

It's possible to directly map most of the pg patterns to chrono patterns but not all. It's a pretty deep rabbit hole - pg supports Julian Day which chrono doesn't. PG also supports Era (AD/BC) whereas chrono only supports +- on year. Then there are escape characters of which backslash is allowed (but now discouraged <https://www.postgresql.org/docs/16/sql-syntax-lexical.html#SQL-SYNTAX-CONSTANTS> = S4.1.2.2), you can define your own in unicode strings, and we would possibly have to handle all of the c-style escape sequences in the format. And heaven forbid if you have to parse or display any day/month text in anything but English. Want day 1 in a week to be Sunday in Chrono (numerous Muslim countries, Israel) ? Nope, you get day 0 - 6 instead (PG does 1-7 correctly). Or we just keep it simple for now :)

…

On Thu, Jan 18, 2024 at 4:58 PM comphead ***@***.***> wrote: Thanks @alamb <https://github.com/alamb> I would probably stick to PG standard as we declared. The PR implementation is really cool, and we may want to have a separate function for it? like from_chrono or something I had a thought for the future to allow for setting the format based on either config or based on dialect. I was thinking that might be best left to after the function refactor (#8045 <#8045>) is done then have separate implementations if desired. I just didn't want to write a parser for the PG syntax as it's pretty detailed and just wanted to get something out there. Someone else noted <#5398 (comment)> the complexity of implementing the PG syntax Do you have happen to have such a parser available? I'm not sure if its available, but we prob use your approach to create an adapter to PG formatting? 🤔 So you map PG formatting rules into chrono formatting rules — Reply to this email directly, view it on GitHub <#8886 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABXHUU5XOZ6ME4SMDBNXJDYPGLI5AVCNFSM6AAAAABB5MLLU6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOJZGI4DQMRVGA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

comphead · 2024-01-19T17:17:26Z

I'll leave the final decision to @alamb although he already approved.
My point it would be unexpected to users to have to_timestamp behaving differently comparing to PG. I prefer a separate method for from_chrono conversions, but if we decide to go with current approach we need to provide more documentation and examples for users in scalar_functions.md. Ideally even to have a separate doc page with examples

…_formats # Conflicts: # datafusion/proto/src/logical_plan/from_proto.rs

…eam change

…ormats

alamb · 2024-01-19T21:07:55Z

I took the liberty of merging up from main and running prettier to get CI clean.

alamb · 2024-01-19T21:27:00Z

Thank you again @Omega359 @comphead and @gruuya

I'll leave the final decision to @alamb although he already approved. My point it would be unexpected to users to have to_timestamp behaving differently comparing to PG. I prefer a separate method for from_chrono conversions,

I agree in an ideal world, to_timestamp formatting would behave identically to an existing implementation (e.g. spark or postgres). However, as has been noted this is a non trivial amount of work. I filed #8915 to track this item

but if we decide to go with current approach we need to provide more documentation and examples for users in scalar_functions.md. Ideally even to have a separate doc page with examples

I agree this would be really nice to add

All in all I think this functionality is important enough to provide users ways to parse timestamps with alternate formatting, even if the supported format strings aren't the ideal form.

Omega359 · 2024-01-20T00:02:42Z

... if we decide to go with current approach we need to provide more documentation and examples for users in scalar_functions.md. Ideally even to have a separate doc page with examples

I did add a fairly extensive example sql however unless a user knows where to look they won't find see it. Speaking from experience adding more extensive documentation for functions would be very helpful for someone starting up with datafusion (& rust). I'll be happy to update the docs with a brief example and a link to the example sql - perhaps that could be a pattern for all functions?

alamb · 2024-01-20T10:28:20Z

I'll be happy to update the docs with a brief example and a link to the example sql - perhaps that could be a pattern for all functions?

That would be amazing @Omega359 -- and really helpful. I have found that when there are good existing patterns subsequent contributors follow them, but getting the initial pattern set is the tough part.

alamb · 2024-01-20T10:28:28Z

Thanks again @Omega359 and @comphead

Omega359 added 5 commits January 16, 2024 13:45

Support to_timestamp with chrono formatting apache#5398

c5e230a

Merge remote-tracking branch 'origin/main' into 5398-timestamp_with_f…

ef6723b

…ormats

Updated user guide's to_timestamp to include chrono formatting inform…

a48ab54

…ation apache#5398

Minor comment update.

3123539

Small documentation updates for to_timestamp functions.

573456d

github-actions bot added logical-expr Logical plan and expressions physical-expr Physical Expressions sqllogictest SQL Logic Tests (.slt) labels Jan 16, 2024

Omega359 changed the title ~~5398 timestamp with formats~~ suppor to_timestamp with optional chrono formats Jan 16, 2024

Omega359 changed the title ~~suppor to_timestamp with optional chrono formats~~ support to_timestamp with optional chrono formats Jan 16, 2024

comphead reviewed Jan 16, 2024

View reviewed changes

datafusion-examples/examples/dataframe_to_timestamp.rs Outdated Show resolved Hide resolved

comphead reviewed Jan 16, 2024

View reviewed changes

Omega359 added 5 commits January 16, 2024 17:01

Merge remote-tracking branch 'upstream/main' into 5398-timestamp_with…

66bf2e3

…_formats

Cargo fmt and clippy improvements.

a78a85a

Switched to assert and unwrap_err based on feedback

c06adcb

Fixed assert, code compiles and runs as expected now.

d14bb0b

Fix fmt (again).

a83b900

alamb mentioned this pull request Jan 17, 2024

DataFusion weekly project plan (Andrew Lamb) - Jan 15, 2024 #8864

Closed

9 tasks

Merge remote-tracking branch 'upstream/main' into 5398-timestamp_with…

0fb0dd2

…_formats

comphead reviewed Jan 18, 2024

View reviewed changes

datafusion/sqllogictest/test_files/timestamps.slt Show resolved Hide resolved

comphead reviewed Jan 18, 2024

View reviewed changes

alamb reviewed Jan 18, 2024

View reviewed changes

alamb approved these changes Jan 18, 2024

View reviewed changes

datafusion/physical-expr/src/datetime_expressions.rs Show resolved Hide resolved

datafusion/physical-expr/src/datetime_expressions.rs Show resolved Hide resolved

datafusion/physical-expr/src/datetime_expressions.rs Outdated Show resolved Hide resolved

gruuya reviewed Jan 18, 2024

View reviewed changes

datafusion/physical-expr/src/datetime_expressions.rs Outdated Show resolved Hide resolved

datafusion/physical-expr/src/datetime_expressions.rs Outdated Show resolved Hide resolved

comphead reviewed Jan 18, 2024

View reviewed changes

gruuya reviewed Jan 18, 2024

View reviewed changes

datafusion/physical-expr/src/datetime_expressions.rs Outdated Show resolved Hide resolved

Omega359 added 3 commits January 18, 2024 16:24

Add additional to_timestamp tests covering usage with tables with and…

db071cc

… without valid formats.

to_timestamp documentation fixes.

40d932c

Omega359 and others added 4 commits January 19, 2024 12:33

Merge remote-tracking branch 'upstream/main' into 5398-timestamp_with…

1b637ae

…_formats # Conflicts: # datafusion/proto/src/logical_plan/from_proto.rs

Resolved merge conflict, updated toStringXXX methods to reflect upstr…

951809a

…eam change

Merge remote-tracking branch 'apache/main' into 5398-timestamp_with_f…

4fcecb3

…ormats

prettier

406fd77

alamb mentioned this pull request Jan 19, 2024

Support "standard" / alternate format arguments for to_timestamp #8915

Open

Fix clippy

3c01f75

alamb merged commit e7c0482 into apache:main Jan 20, 2024
23 checks passed

Omega359 deleted the 5398-timestamp_with_formats branch January 24, 2024 03:35

This was referenced Jan 24, 2024

Improve documentation for new to_timestamp functionality #8980

Closed

Add a to_date function #8987

Closed

Omega359 mentioned this pull request Jan 31, 2024

Add perf tests for newly created/updated functions to_timestamp and make_date #9085

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support to_timestamp with optional chrono formats #8886

support to_timestamp with optional chrono formats #8886

Omega359 commented Jan 16, 2024

comphead left a comment

comphead Jan 18, 2024

Omega359 Jan 18, 2024

alamb Jan 18, 2024

alamb left a comment

alamb Jan 18, 2024

alamb Jan 18, 2024

alamb left a comment

gruuya left a comment

comphead left a comment

Omega359 commented Jan 18, 2024

comphead commented Jan 18, 2024

Omega359 commented Jan 19, 2024 via email •

edited

Loading

comphead commented Jan 19, 2024 •

edited

Loading

alamb commented Jan 19, 2024

alamb commented Jan 19, 2024

Omega359 commented Jan 20, 2024

alamb commented Jan 20, 2024

alamb commented Jan 20, 2024

support to_timestamp with optional chrono formats #8886

support to_timestamp with optional chrono formats #8886

Conversation

Omega359 commented Jan 16, 2024

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 left a comment

Choose a reason for hiding this comment

comphead Jan 18, 2024

Choose a reason for hiding this comment

Omega359 Jan 18, 2024

Choose a reason for hiding this comment

alamb Jan 18, 2024

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb Jan 18, 2024

Choose a reason for hiding this comment

alamb Jan 18, 2024

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

gruuya left a comment

Choose a reason for hiding this comment

comphead left a comment

Choose a reason for hiding this comment

Omega359 commented Jan 18, 2024

comphead commented Jan 18, 2024

Omega359 commented Jan 19, 2024 via email • edited Loading

comphead commented Jan 19, 2024 • edited Loading

alamb commented Jan 19, 2024

alamb commented Jan 19, 2024

Omega359 commented Jan 20, 2024

alamb commented Jan 20, 2024

alamb commented Jan 20, 2024

Omega359 commented Jan 19, 2024 via email •

edited

Loading

comphead commented Jan 19, 2024 •

edited

Loading