Skip to content

Decimal type support for to_timestamp #15486

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.

Sign up for GitHub

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

Jump to bottom
Merged
alamb merged 5 commits into from
Mar 31, 2025
Merged

Decimal type support for to_timestamp #15486

alamb merged 5 commits into from
Mar 31, 2025

Conversation

@jatin510
Copy link
Contributor

@jatin510 jatin510 commented Mar 29, 2025 •
edited
Loading

Which issue does this PR close?

  • Closes #.

Rationale for this change

While working on this:
#14612

I found out that, when we enable parse_float_as_decimal as true.
to_timestamp and as timestamp are converted into Decimal128 type.

So, creating a separate PR, which can help us support Decimal type for timestamp as well.

What changes are included in this PR?

Adding support for Decimal128 for timestamps

> SELECT to_timestamp(1.1) as c1, cast(1.1 as timestamp) as c2, 1.1::timestamp as c3;
+-------------------------+-------------------------+-------------------------+
| c1                      | c2                      | c3                      |
+-------------------------+-------------------------+-------------------------+
| 1970-01-01T00:00:01.100 | 1970-01-01T00:00:01.100 | 1970-01-01T00:00:01.100 |
+-------------------------+-------------------------+-------------------------+
1 row(s) fetched. 
Elapsed 0.022 seconds.

>  SELECT 
    to_timestamp(arrow_cast(1.1, 'Decimal128(2,1)')) as c1,
    cast(arrow_cast(1.1, 'Decimal128(2,1)') as timestamp) as c2,
    arrow_cast(1.1, 'Decimal128(2,1)')::timestamp as c3;
+-------------------------+-------------------------+-------------------------+
| c1                      | c2                      | c3                      |
+-------------------------+-------------------------+-------------------------+
| 1970-01-01T00:00:01.100 | 1970-01-01T00:00:01.100 | 1970-01-01T00:00:01.100 |
+-------------------------+-------------------------+-------------------------+
1 row(s) fetched. 
Elapsed 0.009 seconds.

Are these changes tested?

Yes, Updated test in timestamps.slt

Are there any user-facing changes?

No

@github-actions github-actions bot added the functions Changes to functions implementation label Mar 29, 2025
@github-actions github-actions bot added sqllogictest SQL Logic Tests (.slt) common Related to common crate labels Mar 29, 2025
@jatin510 jatin510 marked this pull request as ready for review March 29, 2025 19:13
@jatin510 jatin510 changed the title wip: decimal type support for to_timestamp Decimal type support for to_timestamp Mar 29, 2025
alamb
alamb reviewed Mar 30, 2025
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thanks @jatin510 -- very cool.

I have a question about the tests but otherwise this one looks good to go to me

datafusion/sqllogictest/test_files/timestamps.slt
1970-01-01T00:00:01.100 1970-01-01T00:00:01.100 1970-01-01T00:00:01.100

query PPP
SELECT to_timestamp(arrow_cast(-1.1, 'Decimal128(2,1)')) as c1, cast(arrow_cast(-1.1, 'Decimal128(2,1)') as timestamp) as c2, arrow_cast(-1.1, 'Decimal128(2,1)')::timestamp as c3;
Copy link
Contributor

@alamb alamb Mar 30, 2025

Choose a reason for hiding this comment

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

what is the value of computing the same value 3 times?

Perhaps it would make more sense to use different types with the same value to show the timestamp that comes back is the same and consistent?

Suggested change
SELECT to_timestamp(arrow_cast(-1.1, 'Decimal128(2,1)')) as c1, cast(arrow_cast(-1.1, 'Decimal128(2,1)') as timestamp) as c2, arrow_cast(-1.1, 'Decimal128(2,1)')::timestamp as c3;
SELECT to_timestamp(arrow_cast(-1.1, 'Int64')) as c1, cast(arrow_cast(-1.1, 'Decimal128(2,1)') as timestamp) as c2, arrow_cast(-1.1, 'Decimal128(9,8)')::timestamp as c3;

(and similarly for the other tests)

Copy link
Contributor Author

@jatin510 jatin510 Mar 30, 2025

Choose a reason for hiding this comment

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

I took reference from other tests, and what i can check is that it shows different way of executing to_timestamp

  • to_timestamp(value) uses the UDF (User Defined Function) implementation in to_timestamp.rs
  • CAST(value AS TIMESTAMP) or equivalently value AS TIMESTAMP uses DataFusion's casting system which relies on Arrow's cast kernels
  • value::TIMESTAMP is SQL syntax sugar for CAST, and uses the same casting route as option 2

alamb
alamb approved these changes Mar 30, 2025
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @jatin510

@alamb alamb merged commit a81ab3a into apache:main Mar 31, 2025
27 checks passed
nirnayroy pushed a commit to nirnayroy/datafusion that referenced this pull request May 2, 2025
@jatin510 @cipherstakes
Decimal type support for to_timestamp (apache#15486)
75989d2 
* wip: decimal type support for to_timestamp

* updated timestamp handling for cast operator

* fixed clippy error

* fixed fmt

* updated timestamps.slt file for decimal128 type support
@alamb alamb mentioned this pull request Jun 27, 2025
Merged
@findepi
Copy link
Member

findepi commented Jun 27, 2025

Any reason to_timestamp was implemented only for scalars, ie constant-foldable values, but not for the actual runtime?

> SELECT to_timestamp('1.1'::decimal(17,1));
+---------------------------+
| to_timestamp(Utf8("1.1")) |
+---------------------------+
| 1970-01-01T00:00:01.100   |
+---------------------------+
1 row(s) fetched.

> SELECT to_timestamp(v::decimal(17,1)) FROM (VALUES ('1.1'), ('2.1')) AS t(v);
Execution error: Invalid decimal value

datafusion/datafusion/functions/src/datetime/to_timestamp.rs

Lines 333 to 352 in 00132da

ColumnarValue::Scalar(ScalarValue::Decimal128(
Some(value),
_,
scale,
)) => {
// Convert decimal to seconds and nanoseconds
let scale_factor = 10_i128.pow(*scale as u32);
let seconds = value / scale_factor;
let fraction = value % scale_factor;
let nanos = (fraction * 1_000_000_000) / scale_factor;
let timestamp_nanos = seconds * 1_000_000_000 + nanos;
Ok(ColumnarValue::Scalar(ScalarValue::TimestampNanosecond(
Some(timestamp_nanos as i64),
None,
)))
}
_ => exec_err!("Invalid decimal value"),

@findepi findepi mentioned this pull request Jun 27, 2025
Open
@Omega359 Omega359 mentioned this pull request Dec 5, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alamb alamb alamb approved these changes

Assignees

No one assigned

Labels

common Related to common crate functions Changes to functions implementation sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jatin510 @findepi @alamb