You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "BryanEmond (via GitHub)" <gi...@apache.org> on 2023/06/19 15:38:35 UTC
[GitHub] [arrow-datafusion] BryanEmond opened a new pull request, #6723: Panic date trunc
BryanEmond opened a new pull request, #6723:
URL: https://github.com/apache/arrow-datafusion/pull/6723
# Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #6701.
# Rationale for this change
see #6701
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
# What changes are included in this PR?
Changed unwrap() for unwrap_or_default().
I've tried to use nano.map(|nano| truncate(nano)) like discussed in the issue. But I'm not sure to understand how I'm suppose to use it.
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
# Are these changes tested?
yes
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
# Are there any user-facing changes?
no
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] viirya commented on pull request #6723: Return null for date_trunc(null) instead of panic
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #6723:
URL: https://github.com/apache/arrow-datafusion/pull/6723#issuecomment-1599600747
Just a format issue:
```rust
if nano.is_none() {
- return Ok(ColumnarValue::Scalar(ScalarValue::TimestampNanosecond(None, tz_opt.clone())));
+ return Ok(ColumnarValue::Scalar(ScalarValue::TimestampNanosecond(
+ None,
+ tz_opt.clone(),
+ )));
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] viirya commented on a diff in pull request #6723: Return null for date_trunc(null) instead of panic
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #6723:
URL: https://github.com/apache/arrow-datafusion/pull/6723#discussion_r1235808701
##########
datafusion/physical-expr/src/datetime_expressions.rs:
##########
@@ -283,6 +283,11 @@ pub fn date_trunc(args: &[ColumnarValue]) -> Result<ColumnarValue> {
Ok(match array {
ColumnarValue::Scalar(ScalarValue::TimestampNanosecond(v, tz_opt)) => {
let nano = (f)(*v)?;
+
+ if nano.is_none() {
+ return Ok(ColumnarValue::Scalar(ScalarValue::Null));
Review Comment:
The issue is because you use `ScalarValue::Null` here.
You can take a look of `enum ScalarValue`. `ScalarValue::Null` is for `DataType::Null`. For other types, they can take a `Some(value)` or a `None` which represents a null value for the type.
For `TimestampNanosecond` type, you just need to put `ScalarValue::TimestampNanosecond` with `None` here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] viirya merged pull request #6723: Return null for date_trunc(null) instead of panic
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya merged PR #6723:
URL: https://github.com/apache/arrow-datafusion/pull/6723
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] viirya commented on a diff in pull request #6723: Panic date trunc
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #6723:
URL: https://github.com/apache/arrow-datafusion/pull/6723#discussion_r1234479074
##########
datafusion/physical-expr/src/datetime_expressions.rs:
##########
@@ -287,31 +287,31 @@ pub fn date_trunc(args: &[ColumnarValue]) -> Result<ColumnarValue> {
"minute" => {
// trunc to minute
let second = ScalarValue::TimestampNanosecond(
- Some(nano.unwrap() / 1_000_000_000 * 1_000_000_000),
+ Some(nano.unwrap_or_default() / 1_000_000_000 * 1_000_000_000),
Review Comment:
Currently in `date_trunc`, `nano` is assumed to be `Some`. To return null for null input instead of `unwrap` on it, we can simply return a null timestamp scalar if `nano` is None.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] viirya commented on a diff in pull request #6723: Panic date trunc
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #6723:
URL: https://github.com/apache/arrow-datafusion/pull/6723#discussion_r1234458349
##########
datafusion/core/tests/sqllogictests/test_files/timestamps.slt:
##########
@@ -837,6 +837,16 @@ SELECT DATE_TRUNC('YEAR', TIMESTAMP '2022-08-03 14:38:50Z');
----
2022-01-01T00:00:00
+query P
+SELECT DATE_TRUNC('year', NULL);
+----
+1970-01-01T00:00:00
Review Comment:
Hmm, I think @alamb added the expected behavior in #6701, which is `null should be returned` instead of this default value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] BryanEmond commented on pull request #6723: Return null for date_trunc(null) instead of panic
Posted by "BryanEmond (via GitHub)" <gi...@apache.org>.
BryanEmond commented on PR #6723:
URL: https://github.com/apache/arrow-datafusion/pull/6723#issuecomment-1599189261
I know this push doesn't work, but I really don't get it. Now the unwrap() error is gone but I get this error.
```
Running "timestamps.slt"
External error: query failed: DataFusion error: simplify_expressions
caused by
Internal error: Optimizer rule 'simplify_expressions' failed, due to generate a different schema,
original schema:
DFSchema { fields: [DFField { qualifier: None, field: Field { name: "date_trunc(Utf8(\"year\"),NULL)", data_type: Timestamp(Nanosecond, None), nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} } }], metadata: {} },
new schema:
DFSchema { fields: [DFField { qualifier: None, field: Field { name: "date_trunc(Utf8(\"year\"),NULL)", data_type: Null, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} } }], metadata: {} }.
This was likely caused by a bug in DataFusion's code and we would welcome that you file an bug report in our issue tracker
[SQL] SELECT DATE_TRUNC('year', NULL);
at tests/sqllogictests/test_files/timestamps.slt:840
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] viirya commented on pull request #6723: Return null for date_trunc(null) instead of panic
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #6723:
URL: https://github.com/apache/arrow-datafusion/pull/6723#issuecomment-1599473011
You are pretty close to finish this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] viirya commented on a diff in pull request #6723: Return null for date_trunc(null) instead of panic
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #6723:
URL: https://github.com/apache/arrow-datafusion/pull/6723#discussion_r1235810333
##########
datafusion/physical-expr/src/datetime_expressions.rs:
##########
@@ -283,6 +283,11 @@ pub fn date_trunc(args: &[ColumnarValue]) -> Result<ColumnarValue> {
Ok(match array {
ColumnarValue::Scalar(ScalarValue::TimestampNanosecond(v, tz_opt)) => {
let nano = (f)(*v)?;
+
+ if nano.is_none() {
+ return Ok(ColumnarValue::Scalar(ScalarValue::Null));
Review Comment:
This is how `TimestampNanosecond` is defined in `enum ScalarValue`:
```
/// Timestamp Nanoseconds
TimestampNanosecond(Option<i64>, Option<Arc<str>>),
```
The first argument is `Option<i64>`. If it is a `None`, meaning that is a null `TimestampNanosecond` scalar value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] viirya commented on pull request #6723: Return null for date_trunc(null) instead of panic
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #6723:
URL: https://github.com/apache/arrow-datafusion/pull/6723#issuecomment-1601072731
Thank you @BryanEmond
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] viirya commented on a diff in pull request #6723: Return null for date_trunc(null) instead of panic
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #6723:
URL: https://github.com/apache/arrow-datafusion/pull/6723#discussion_r1235808701
##########
datafusion/physical-expr/src/datetime_expressions.rs:
##########
@@ -283,6 +283,11 @@ pub fn date_trunc(args: &[ColumnarValue]) -> Result<ColumnarValue> {
Ok(match array {
ColumnarValue::Scalar(ScalarValue::TimestampNanosecond(v, tz_opt)) => {
let nano = (f)(*v)?;
+
+ if nano.is_none() {
+ return Ok(ColumnarValue::Scalar(ScalarValue::Null));
Review Comment:
The issue is because you use `ScalarValue::Null` here.
You can take a look of `enum ScalarValue`. `ScalarValue::Null` is for `DataType::Null`. That's why there is an error for `a different schema`.
For other types, they can take a `Some(value)` or a `None` which represents a null value for the type.
For `TimestampNanosecond` type, you just need to put `ScalarValue::TimestampNanosecond` with `None` here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] viirya commented on pull request #6723: Return null for date_trunc(null) instead of panic
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #6723:
URL: https://github.com/apache/arrow-datafusion/pull/6723#issuecomment-1599601604
You can run cargo fmt locally or use it in your editor plugin so the code will be formatted automatically.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] BryanEmond commented on pull request #6723: Return null for date_trunc(null) instead of panic
Posted by "BryanEmond (via GitHub)" <gi...@apache.org>.
BryanEmond commented on PR #6723:
URL: https://github.com/apache/arrow-datafusion/pull/6723#issuecomment-1599608540
Thank you for your help, I really appreciate it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org