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

Document timestamp input limits #8369

Merged
merged 5 commits into from
Dec 1, 2023
Merged

Document timestamp input limits #8369

merged 5 commits into from
Dec 1, 2023

Conversation

comphead
Copy link
Contributor

Which issue does this PR close?

Closes #8336
Closes #7958

Rationale for this change

Declared limits for to_timestamp, to_timestamp_nanos functions to prevent overflows. Its been agreed in #8336 (comment) to use to_timestamp_seconds function for values between 1677-09-21T00:12:44.0 and 2262-04-11T23:47:16.0

What changes are included in this PR?

Are these changes tested?

No need

Are there any user-facing changes?

No

@github-actions github-actions bot added the physical-expr Physical Expressions label Nov 29, 2023
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 @comphead

@@ -157,6 +161,10 @@ pub fn to_timestamp_micros(args: &[ColumnarValue]) -> Result<ColumnarValue> {
}

/// to_timestamp_nanos SQL function
///
/// Note: `to_timestamp_nanos` returns `Timestamp(Nanosecond)`. The supported range for integer input is between `-9223372037` and `9223372036`.
Copy link
Contributor

Choose a reason for hiding this comment

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

this doesn't seem right. I think the argument is treated as nanoseconds, so the range is really -i64::MIN to i64::MAX

For example, this works just fine

❯ select to_timestamp_nanos(92233720360000)
;
+-------------------------------------------+
| to_timestamp_nanos(Int64(92233720360000)) |
+-------------------------------------------+
| 1970-01-02T01:37:13.720360                |
+-------------------------------------------+
1 row in set. Query took 0.000 seconds.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, you are right

@@ -1497,6 +1500,9 @@ return the corresponding timestamp.
to_timestamp_nanos(expression)
```

Note: `to_timestamp_nanos` returns `Timestamp(Nanosecond)`. The supported range for integer input is between `-9223372037` and `9223372036`.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

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 @comphead

datafusion/physical-expr/src/datetime_expressions.rs Outdated Show resolved Hide resolved
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
@alamb alamb merged commit c19260d into apache:main Dec 1, 2023
23 checks passed
@alamb
Copy link
Contributor

alamb commented Dec 1, 2023

🚀

appletreeisyellow pushed a commit to appletreeisyellow/datafusion that referenced this pull request Dec 14, 2023
* document timestamp input limis

* fix text

* prettier

* remove doc for nanoseconds

* Update datafusion/physical-expr/src/datetime_expressions.rs

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

---------

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
physical-expr Physical Expressions
Projects
None yet
2 participants