You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "tustvold (via GitHub)" <gi...@apache.org> on 2023/07/03 18:05:37 UTC

[GitHub] [arrow-datafusion] tustvold opened a new pull request, #6832: POC: Datum arithmetic

tustvold opened a new pull request, #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832

   _Draft as upstream kernels still need some work_
   
   # 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 #.
   
   # Rationale for this change
   
   <!--
    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.  
   -->
   
   Use new kernels added in https://github.com/apache/arrow-rs/pull/4393
   
   # What changes are included in this PR?
   
   <!--
   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?
   
   <!--
   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?
   
   <!--
   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 a diff in pull request #6832: Update Arrow 45.0.0 And Datum Arithmetic

Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832#discussion_r1278621222


##########
datafusion/physical-expr/src/expressions/binary.rs:
##########
@@ -2609,14 +2441,14 @@ mod tests {
             Operator::Divide,
             create_decimal_array(
                 &[
-                    Some(99193548387), // 0.99193548387

Review Comment:
   I think that it the rules for precision of decimal division are changed. I won't say previous rules are incorrect.



-- 
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] alamb commented on a diff in pull request #6832: Datum arithmetic

Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832#discussion_r1263034062


##########
datafusion/physical-expr/src/expressions/negative.rs:
##########
@@ -236,9 +179,6 @@ mod tests {
         test_array_negative_op!(Int64, 23456i64, 12345i64);
         test_array_negative_op!(Float32, 2345.0f32, 1234.0f32);
         test_array_negative_op!(Float64, 23456.0f64, 12345.0f64);
-        test_array_negative_op_intervals!(YearMonth, 2345i32, 1234i32);
-        test_array_negative_op_intervals!(DayTime, 23456i64, 12345i64);
-        test_array_negative_op_intervals!(MonthDayNano, 234567i128, 123456i128);

Review Comment:
   I think you can do something like this
   
   ```
   select arrow_cast(interval '30 minutes', 'Interval(MonthDayNano)');
   ```



-- 
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] tustvold commented on a diff in pull request #6832: POC: Datum arithmetic

Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832#discussion_r1252178385


##########
datafusion/core/tests/sqllogictests/test_files/timestamps.slt:
##########
@@ -1182,10 +1182,10 @@ create table bar (val int, i1 interval, i2 interval) as values
 query I?
 SELECT val, ts1 - ts2 FROM foo ORDER BY ts2 - ts1;
 ----
-4 0 years 0 mons -15250 days -13 hours -28 mins -44.999876545 secs
-3 0 years 0 mons 15952 days 23 hours 22 mins 12.667123455 secs
-2 0 years 0 mons 8406 days 1 hours 1 mins 54.877123455 secs
-1 0 years 0 mons 53 days 16 hours 0 mins 20.000000024 secs
+3 P15952DT84132.667123455S

Review Comment:
   This does highlight another fun quirk of intervals - they sort strangely



-- 
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] alamb merged pull request #6832: Update Arrow 45.0.0 And Datum Arithmetic, change Decimal Division semantics

Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832


-- 
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] alamb commented on pull request #6832: Update Arrow 45.0.0 And Datum Arithmetic, change Decimal Division semantics

Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832#issuecomment-1669973698

   Our tests passed. 🚀  Onward


-- 
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] tustvold commented on pull request #6832: Update Arrow 45.0.0 And Datum Arithmetic, change Decimal Division semantics

Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832#issuecomment-1670314868

   https://github.com/apache/arrow-rs/issues/4664 tracks further upstream work related to decimal arithmetic


-- 
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] alamb commented on pull request #6832: Update Arrow 45.0.0 And Datum Arithmetic, change Decimal Division semantics

Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832#issuecomment-1680512787

   I filed https://github.com/apache/arrow-datafusion/issues/7301 to track possibly adding a spark decimal compatibility mode if anyone needs 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

[GitHub] [arrow-datafusion] tustvold commented on a diff in pull request #6832: POC: Datum arithmetic

Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832#discussion_r1252177822


##########
datafusion/common/src/scalar.rs:
##########
@@ -5912,285 +5859,6 @@ mod tests {
         check_array(array);
     }
 
-    fn get_timestamp_test_data(

Review Comment:
   This test is removed as timestamp arithmetic no longer returns intervals (which avoids this whole mess of edge cases)



-- 
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] tustvold commented on pull request #6832: Update Arrow 45.0.0 And Datum Arithmetic, change Decimal Division semantics

Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832#issuecomment-1680113761

   https://github.com/apache/arrow-rs/issues/4664#issuecomment-1676091528 summarises my thoughts on the issue having spent significant time on this. TLDR is we already support a higher precision than most commercial systems, and given this and the fact I personally have no use-cases for decimals, find it hard to justify spending more of my time on this. Ultimately if someone really wants true arbitrary precision decimal arithmetic a la postgres or spark, arrow is probably the wrong tool.


-- 
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] alamb commented on pull request #6832: Datum arithmetic

Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832#issuecomment-1627432766

   🤯 
   <img width="198" alt="Screenshot 2023-07-08 at 1 22 32 PM" src="https://github.com/apache/arrow-datafusion/assets/490673/f7d63ea5-f3a3-4cb3-a68c-b6350c18b0b8">
   


-- 
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] tustvold commented on pull request #6832: Update Arrow 44.0.0 And Datum Arithmetic

Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832#issuecomment-1646247771

   https://github.com/apache/arrow-rs/pull/4557 proposes changing the formatting of durations upstream, PTAL


-- 
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] alamb commented on pull request #6832: Update Arrow 45.0.0 And Datum Arithmetic, change Decimal Division semantics

Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832#issuecomment-1669422649

   I am going to update this PR and resolve conflicts


-- 
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] alamb commented on pull request #6832: Update Arrow 45.0.0 And Datum Arithmetic, change Decimal Division semantics

Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832#issuecomment-1663898998

   I have posted an announcement about this PR to the mailing list: https://lists.apache.org/thread/837ghhjh9gd1kvg0qqnmxqs0rm62x0xt
   
   I will plan to merge it in Monday Aug 7 2023 unless I hear otherwise


-- 
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 #6832: Update Arrow 45.0.0 And Datum Arithmetic

Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832#discussion_r1278622152


##########
datafusion/core/tests/sqllogictests/test_files/decimal.slt:
##########
@@ -365,7 +365,7 @@ select c1*c5 from decimal_simple;
 query T
 select arrow_typeof(c1/cast(0.00001 as decimal(5,5))) from decimal_simple limit 1;
 ----
-Decimal128(21, 12)
+Decimal128(19, 10)

Review Comment:
   I won't say this is not correct, this is more like a design decision to me. But this actually changes the precision of decimal arithmetic operations in DataFusion. For some, this might be a different change and not something could be implicitly accepted. I'd suggest there will be some documents on these changes with new release including 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 #6832: Update Arrow 45.0.0 And Datum Arithmetic

Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832#discussion_r1278621222


##########
datafusion/physical-expr/src/expressions/binary.rs:
##########
@@ -2609,14 +2441,14 @@ mod tests {
             Operator::Divide,
             create_decimal_array(
                 &[
-                    Some(99193548387), // 0.99193548387

Review Comment:
   I think that the rules for precision of decimal division are changed. I won't say previous rules are incorrect.
   
   And, obviously it is precision loss 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] tustvold commented on a diff in pull request #6832: Update Arrow 45.0.0 And Datum Arithmetic

Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832#discussion_r1278622708


##########
datafusion/core/tests/sqllogictests/test_files/decimal.slt:
##########
@@ -365,7 +365,7 @@ select c1*c5 from decimal_simple;
 query T
 select arrow_typeof(c1/cast(0.00001 as decimal(5,5))) from decimal_simple limit 1;
 ----
-Decimal128(21, 12)
+Decimal128(19, 10)

Review Comment:
   > But this actually changes the precision of decimal arithmetic operations in DataFusion
   
   Correct, it changes the semantics of decimal division to match that of postgres, mysql and Hive. I've not done an exhaustive survey of other DBMS products, but the Hive semantics were decided based on a survey of the systems at the time. Other decimal operations are unaffected. This should be definitely communicated, especially if Spark does something different (possibly as a misreading of the Hive specification).



-- 
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] tustvold commented on a diff in pull request #6832: Update Arrow 45.0.0 And Datum Arithmetic

Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832#discussion_r1278588947


##########
datafusion/physical-expr/src/format.rs:
##########
@@ -0,0 +1,22 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use arrow::util::display::{DurationFormat, FormatOptions};
+
+/// The default [`FormatOptions`] to use within DataFusion
+pub const DEFAULT_FORMAT_OPTIONS: FormatOptions<'static> =

Review Comment:
   I wasn't really sure where to put this, I may move this into core so that it can be used by the assert_batches_eq macro :thinking: 
   
   



-- 
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] berkaysynnada commented on pull request #6832: Update Arrow 44.0.0 And Datum Arithmetic

Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on PR #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832#issuecomment-1642232791

   > 
   
   AFAIK the user should not experience two different types as duration and interval. However, in this case, the timestamp - timestamp difference causes a different representation, and the operations between the intervals cause a different representation. This situation can be solved with implicit cast, but it seems like conversion between units is required while doing this (years and days are represented in hours in your example).


-- 
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] tustvold commented on a diff in pull request #6832: Update Arrow 44.0.0 And Datum Arithmetic

Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832#discussion_r1268522941


##########
datafusion/core/tests/sql/timestamp.rs:
##########
@@ -1035,13 +1035,13 @@ async fn timestamp_sub_with_tz() -> Result<()> {
     let sql = "SELECT val, ts1 - ts2 AS ts_diff FROM table_a ORDER BY ts2 - ts1";
     let actual = execute_to_batches(&ctx, sql).await;
     let expected = vec![
-        "+-----+---------------------------------------------------+",
-        "| val | ts_diff                                           |",
-        "+-----+---------------------------------------------------+",
-        "| 3   | 0 years 0 mons 0 days 10 hours 0 mins 30.000 secs |",
-        "| 1   | 0 years 0 mons 0 days 10 hours 0 mins 20.000 secs |",
-        "| 2   | 0 years 0 mons 0 days 10 hours 0 mins 10.000 secs |",
-        "+-----+---------------------------------------------------+",
+        "+-----+---------+",
+        "| val | ts_diff |",
+        "+-----+---------+",
+        "| 3   | PT30S   |",

Review Comment:
   I think we should probably change duration output to be a number of the duration unit, i.e. seconds, milliseconds, etc... to avoid potential ambiguity over what the duration represents. It might be confusing if both intervals and durations report some number of days, but arithmetic involving them has different semantics across DST boundaries. I will file an upstream ticket and work on 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

[GitHub] [arrow-datafusion] tustvold commented on a diff in pull request #6832: Update Arrow 44.0.0 And Datum Arithmetic

Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832#discussion_r1267282191


##########
datafusion/physical-expr/src/expressions/binary.rs:
##########
@@ -2609,14 +2441,14 @@ mod tests {
             Operator::Divide,
             create_decimal_array(
                 &[
-                    Some(99193548387), // 0.99193548387

Review Comment:
   Because the rules for precision of decimal division are now correct #6828 



-- 
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 #6832: Update Arrow 45.0.0 And Datum Arithmetic

Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832#discussion_r1278622152


##########
datafusion/core/tests/sqllogictests/test_files/decimal.slt:
##########
@@ -365,7 +365,7 @@ select c1*c5 from decimal_simple;
 query T
 select arrow_typeof(c1/cast(0.00001 as decimal(5,5))) from decimal_simple limit 1;
 ----
-Decimal128(21, 12)
+Decimal128(19, 10)

Review Comment:
   I won't say this is not correct, this is more like a design decision to me. But this actually changes the precision of decimal arithmetic operations in DataFusion. For some, this might be an important change that could change at downstream and not something could be implicitly accepted. I'd suggest there will be some documents on these changes with new release including 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] alamb commented on a diff in pull request #6832: Update Arrow 45.0.0 And Datum Arithmetic

Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832#discussion_r1279154756


##########
datafusion/physical-expr/src/format.rs:
##########
@@ -0,0 +1,22 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use arrow::util::display::{DurationFormat, FormatOptions};
+
+/// The default [`FormatOptions`] to use within DataFusion
+pub const DEFAULT_FORMAT_OPTIONS: FormatOptions<'static> =

Review Comment:
   How about `datafusion_common` as it seems like common functionality across moduels?



-- 
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] alamb commented on pull request #6832: Update Arrow 45.0.0 And Datum Arithmetic, change Decimal Division semantics

Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832#issuecomment-1669682259

   I believe we have resolved all concerns on this PR and it represents a significant step forward in terms of correctness I plan to merge it in once CI has passed. 


-- 
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] tustvold commented on a diff in pull request #6832: Update Arrow 44.0.0 And Datum Arithmetic

Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832#discussion_r1267280451


##########
datafusion/core/tests/sql/timestamp.rs:
##########
@@ -1035,13 +1035,13 @@ async fn timestamp_sub_with_tz() -> Result<()> {
     let sql = "SELECT val, ts1 - ts2 AS ts_diff FROM table_a ORDER BY ts2 - ts1";
     let actual = execute_to_batches(&ctx, sql).await;
     let expected = vec![
-        "+-----+---------------------------------------------------+",
-        "| val | ts_diff                                           |",
-        "+-----+---------------------------------------------------+",
-        "| 3   | 0 years 0 mons 0 days 10 hours 0 mins 30.000 secs |",
-        "| 1   | 0 years 0 mons 0 days 10 hours 0 mins 20.000 secs |",
-        "| 2   | 0 years 0 mons 0 days 10 hours 0 mins 10.000 secs |",
-        "+-----+---------------------------------------------------+",
+        "+-----+---------+",
+        "| val | ts_diff |",
+        "+-----+---------+",
+        "| 3   | PT30S   |",

Review Comment:
   I would be happy to change the formatting upstream if there is consensus on what it should be. We are currently just doing what chrono does, which appears to be [ISO 8601](https://en.wikipedia.org/wiki/ISO_8601#Time_intervals)



-- 
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] alamb commented on a diff in pull request #6832: Update Arrow 44.0.0 And Datum Arithmetic

Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832#discussion_r1272516823


##########
datafusion/core/tests/sql/timestamp.rs:
##########
@@ -1035,13 +1035,13 @@ async fn timestamp_sub_with_tz() -> Result<()> {
     let sql = "SELECT val, ts1 - ts2 AS ts_diff FROM table_a ORDER BY ts2 - ts1";
     let actual = execute_to_batches(&ctx, sql).await;
     let expected = vec![
-        "+-----+---------------------------------------------------+",
-        "| val | ts_diff                                           |",
-        "+-----+---------------------------------------------------+",
-        "| 3   | 0 years 0 mons 0 days 10 hours 0 mins 30.000 secs |",
-        "| 1   | 0 years 0 mons 0 days 10 hours 0 mins 20.000 secs |",
-        "| 2   | 0 years 0 mons 0 days 10 hours 0 mins 10.000 secs |",
-        "+-----+---------------------------------------------------+",
+        "+-----+---------+",
+        "| val | ts_diff |",
+        "+-----+---------+",
+        "| 3   | PT30S   |",

Review Comment:
   I have filed two tickets of interest: 
   * https://github.com/apache/arrow-datafusion/issues/7068 which tries to describes the change in semantics this PR does
   * https://github.com/apache/arrow-datafusion/issues/7070 that proposes special casing the display of Duration, based on @waitingkuo 's feedback above



-- 
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] tustvold commented on a diff in pull request #6832: Update Arrow 44.0.0 And Datum Arithmetic

Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832#discussion_r1267181751


##########
datafusion/physical-expr/src/expressions/negative.rs:
##########
@@ -236,9 +179,6 @@ mod tests {
         test_array_negative_op!(Int64, 23456i64, 12345i64);
         test_array_negative_op!(Float32, 2345.0f32, 1234.0f32);
         test_array_negative_op!(Float64, 23456.0f64, 12345.0f64);
-        test_array_negative_op_intervals!(YearMonth, 2345i32, 1234i32);
-        test_array_negative_op_intervals!(DayTime, 23456i64, 12345i64);
-        test_array_negative_op_intervals!(MonthDayNano, 234567i128, 123456i128);

Review Comment:
   Arrow cast doesn't appear to support intervals in my experiments



-- 
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] waitingkuo commented on a diff in pull request #6832: Update Arrow 44.0.0 And Datum Arithmetic

Posted by "waitingkuo (via GitHub)" <gi...@apache.org>.
waitingkuo commented on code in PR #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832#discussion_r1268498518


##########
datafusion/core/tests/sql/timestamp.rs:
##########
@@ -1035,13 +1035,13 @@ async fn timestamp_sub_with_tz() -> Result<()> {
     let sql = "SELECT val, ts1 - ts2 AS ts_diff FROM table_a ORDER BY ts2 - ts1";
     let actual = execute_to_batches(&ctx, sql).await;
     let expected = vec![
-        "+-----+---------------------------------------------------+",
-        "| val | ts_diff                                           |",
-        "+-----+---------------------------------------------------+",
-        "| 3   | 0 years 0 mons 0 days 10 hours 0 mins 30.000 secs |",
-        "| 1   | 0 years 0 mons 0 days 10 hours 0 mins 20.000 secs |",
-        "| 2   | 0 years 0 mons 0 days 10 hours 0 mins 10.000 secs |",
-        "+-----+---------------------------------------------------+",
+        "+-----+---------+",
+        "| val | ts_diff |",
+        "+-----+---------+",
+        "| 3   | PT30S   |",

Review Comment:
   i think the coercion rules might not work well. 
   ```bash
   ❯ select ('2023-01-02'::timestamp - '2023-01-01'::timestamp)::interval;
   +--------------------------------------------------------+
   | Utf8("2023-01-02") - Utf8("2023-01-01")                |
   +--------------------------------------------------------+
   | 0 years 0 mons 0 days 24 hours 0 mins 0.000000000 secs |
   +--------------------------------------------------------+
   1 row in set. Query took 0.002 seconds.
   ```
   it's converted to (0 month, 0 day, 86400000000000 nanos) instead of (0 month, 1 day, 0 nanos)
   
   which isn't consist with 
   ```bash
   ❯ select ('2023-01-02'::timestamp - '2023-01-01'::timestamp);
   +-----------------------------------------+
   | Utf8("2023-01-02") - Utf8("2023-01-01") |
   +-----------------------------------------+
   | P1D                                     |
   +-----------------------------------------+
   1 row in set. Query took 0.002 seconds.
   ``` 
   
   both formats are fine so we. in case we decide to align with interval format, we should discuss whether it outputs 
   `0 years 0 mons 1 days 0 hours 0 mins 0 secs` or 
   `0 years 0 mons 0 days 24 hours 0 mins 0 secs` in this case



##########
datafusion/core/tests/sql/timestamp.rs:
##########
@@ -1035,13 +1035,13 @@ async fn timestamp_sub_with_tz() -> Result<()> {
     let sql = "SELECT val, ts1 - ts2 AS ts_diff FROM table_a ORDER BY ts2 - ts1";
     let actual = execute_to_batches(&ctx, sql).await;
     let expected = vec![
-        "+-----+---------------------------------------------------+",
-        "| val | ts_diff                                           |",
-        "+-----+---------------------------------------------------+",
-        "| 3   | 0 years 0 mons 0 days 10 hours 0 mins 30.000 secs |",
-        "| 1   | 0 years 0 mons 0 days 10 hours 0 mins 20.000 secs |",
-        "| 2   | 0 years 0 mons 0 days 10 hours 0 mins 10.000 secs |",
-        "+-----+---------------------------------------------------+",
+        "+-----+---------+",
+        "| val | ts_diff |",
+        "+-----+---------+",
+        "| 3   | PT30S   |",

Review Comment:
   i think the coercion rules might not work well. 
   ```bash
   ❯ select ('2023-01-02'::timestamp - '2023-01-01'::timestamp)::interval;
   +--------------------------------------------------------+
   | Utf8("2023-01-02") - Utf8("2023-01-01")                |
   +--------------------------------------------------------+
   | 0 years 0 mons 0 days 24 hours 0 mins 0.000000000 secs |
   +--------------------------------------------------------+
   1 row in set. Query took 0.002 seconds.
   ```
   it's converted to (0 month, 0 day, 86400000000000 nanos) instead of (0 month, 1 day, 0 nanos)
   
   which isn't consist with 
   ```bash
   ❯ select ('2023-01-02'::timestamp - '2023-01-01'::timestamp);
   +-----------------------------------------+
   | Utf8("2023-01-02") - Utf8("2023-01-01") |
   +-----------------------------------------+
   | P1D                                     |
   +-----------------------------------------+
   1 row in set. Query took 0.002 seconds.
   ``` 
   
   both formats are fine so we. in case we decide to align with interval format, we should discuss whether it outputs 
   `0 years 0 mons 1 days 0 hours 0 mins 0 secs` or 
   `0 years 0 mons 0 days 24 hours 0 mins 0 secs` in this case



-- 
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 #6832: Update Arrow 45.0.0 And Datum Arithmetic

Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832#discussion_r1278622152


##########
datafusion/core/tests/sqllogictests/test_files/decimal.slt:
##########
@@ -365,7 +365,7 @@ select c1*c5 from decimal_simple;
 query T
 select arrow_typeof(c1/cast(0.00001 as decimal(5,5))) from decimal_simple limit 1;
 ----
-Decimal128(21, 12)
+Decimal128(19, 10)

Review Comment:
   I won't say this is not correct, this is more like a design decision to me. But this actually changes the precision of decimal arithmetic operations in DataFusion. For some, this might be an important change that could cause difference at downstream and not something could be implicitly accepted. I'd suggest there will be some documents on these changes with new release including 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] alamb commented on a diff in pull request #6832: Update Arrow 44.0.0 And Datum Arithmetic

Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832#discussion_r1267268245


##########
datafusion/core/tests/sqllogictests/test_files/decimal.slt:
##########
@@ -365,7 +365,7 @@ select c1*c5 from decimal_simple;
 query T
 select arrow_typeof(c1/cast(0.00001 as decimal(5,5))) from decimal_simple limit 1;
 ----
-Decimal128(21, 12)
+Decimal128(19, 10)

Review Comment:
   cc @liukun4515 and @viirya -- I assume you have reviewed the decimal changes upstream



##########
datafusion/core/tests/sqllogictests/test_files/arrow_typeof.slt:
##########
@@ -303,9 +303,11 @@ select arrow_cast('30 minutes', 'Interval(MonthDayNano)');
 
 ## Duration
 
-query error DataFusion error: Optimizer rule 'simplify_expressions' failed\ncaused by\nThis feature is not implemented: Can't create a scalar from array of type "Duration\(Second\)"
+query ?

Review Comment:
   🎉 



##########
datafusion/core/tests/sql/timestamp.rs:
##########
@@ -1035,13 +1035,13 @@ async fn timestamp_sub_with_tz() -> Result<()> {
     let sql = "SELECT val, ts1 - ts2 AS ts_diff FROM table_a ORDER BY ts2 - ts1";
     let actual = execute_to_batches(&ctx, sql).await;
     let expected = vec![
-        "+-----+---------------------------------------------------+",
-        "| val | ts_diff                                           |",
-        "+-----+---------------------------------------------------+",
-        "| 3   | 0 years 0 mons 0 days 10 hours 0 mins 30.000 secs |",
-        "| 1   | 0 years 0 mons 0 days 10 hours 0 mins 20.000 secs |",
-        "| 2   | 0 years 0 mons 0 days 10 hours 0 mins 10.000 secs |",
-        "+-----+---------------------------------------------------+",
+        "+-----+---------+",
+        "| val | ts_diff |",
+        "+-----+---------+",
+        "| 3   | PT30S   |",

Review Comment:
   I remember the `PT30S`  format is some sort of standard but I think it is a pretty terrible UX and very uncommon. 
   
   I know there is a technical difference between duration and interval in the arrow spec but as a user, I find this very confusing:
   
   ```
   ❯ select (now() - '2023-01-01'::timestamp);
   +----------------------------+
   | now() - Utf8("2023-01-01") |
   +----------------------------+
   | P198DT72932.972880S        |
   +----------------------------+
   ```
   
   On the other hand the original formatting is not consistent with Postgres either.
   
   
   ```
   postgres=# select interval '5 minutes 2  days';
       interval
   -----------------
    2 days 00:05:00
   (1 row)
   
   postgres=# select interval '5 minutes';
    interval
   ----------
    00:05:00
   (1 row)
   ```
   
   I played around with this locally and  we can still get an `interval` with an explicit cast 
   
   ```sql
   ❯ select (now() - '2023-01-01'::timestamp);
   +----------------------------+
   | now() - Utf8("2023-01-01") |
   +----------------------------+
   | P198DT72932.972880S        |
   +----------------------------+
   1 row in set. Query took 0.002 seconds.
   ❯ select (now() - '2023-01-01'::timestamp)::interval;
   +------------------------------------------------------------+
   | now() - Utf8("2023-01-01")                                 |
   +------------------------------------------------------------+
   | 0 years 0 mons 0 days 4772 hours 15 mins 35.571791000 secs |
   +------------------------------------------------------------+
   1 row in set. Query took 0.002 seconds.
   ```
   
   So we can perhaps add the appropriate coercion rules if necessary
   
   Or perhaps we can change the formatting of Durations in arrow-rs to be more interval like
   
   @waitingkuo do you have any thoughts
   



##########
datafusion/physical-expr/src/expressions/binary.rs:
##########
@@ -2609,14 +2441,14 @@ mod tests {
             Operator::Divide,
             create_decimal_array(
                 &[
-                    Some(99193548387), // 0.99193548387

Review Comment:
   why these changes?



##########
datafusion/physical-expr/src/expressions/datetime.rs:
##########


Review Comment:
   👏 



##########
datafusion/core/tests/sql/timestamp.rs:
##########
@@ -1035,13 +1035,13 @@ async fn timestamp_sub_with_tz() -> Result<()> {
     let sql = "SELECT val, ts1 - ts2 AS ts_diff FROM table_a ORDER BY ts2 - ts1";
     let actual = execute_to_batches(&ctx, sql).await;
     let expected = vec![
-        "+-----+---------------------------------------------------+",
-        "| val | ts_diff                                           |",
-        "+-----+---------------------------------------------------+",
-        "| 3   | 0 years 0 mons 0 days 10 hours 0 mins 30.000 secs |",
-        "| 1   | 0 years 0 mons 0 days 10 hours 0 mins 20.000 secs |",
-        "| 2   | 0 years 0 mons 0 days 10 hours 0 mins 10.000 secs |",
-        "+-----+---------------------------------------------------+",
+        "+-----+---------+",
+        "| val | ts_diff |",
+        "+-----+---------+",
+        "| 3   | PT30S   |",

Review Comment:
   I remember the `PT30S`  format is some sort of standard but I think it is a pretty terrible UX and very uncommon. 
   
   I know there is a technical difference between duration and interval in the arrow spec but as a user, I find this very confusing:
   
   ```
   ❯ select (now() - '2023-01-01'::timestamp);
   +----------------------------+
   | now() - Utf8("2023-01-01") |
   +----------------------------+
   | P198DT72932.972880S        |
   +----------------------------+
   ```
   
   On the other hand the original formatting is not consistent with Postgres either.
   
   
   ```
   postgres=# select interval '5 minutes 2  days';
       interval
   -----------------
    2 days 00:05:00
   (1 row)
   
   postgres=# select interval '5 minutes';
    interval
   ----------
    00:05:00
   (1 row)
   ```
   
   I played around with this locally and  we can still get an `interval` with an explicit cast 
   
   ```sql
   ❯ select (now() - '2023-01-01'::timestamp);
   +----------------------------+
   | now() - Utf8("2023-01-01") |
   +----------------------------+
   | P198DT72932.972880S        |
   +----------------------------+
   1 row in set. Query took 0.002 seconds.
   ❯ select (now() - '2023-01-01'::timestamp)::interval;
   +------------------------------------------------------------+
   | now() - Utf8("2023-01-01")                                 |
   +------------------------------------------------------------+
   | 0 years 0 mons 0 days 4772 hours 15 mins 35.571791000 secs |
   +------------------------------------------------------------+
   1 row in set. Query took 0.002 seconds.
   ```
   
   So we can perhaps add the appropriate coercion rules if necessary
   
   Or perhaps we can change the formatting of Durations in arrow-rs to be more interval like
   
   @waitingkuo do you have any thoughts
   



##########
datafusion/physical-expr/src/expressions/datetime.rs:
##########


Review Comment:
   👏 



##########
datafusion/core/tests/sqllogictests/test_files/interval.slt:
##########
@@ -430,15 +436,11 @@ select '1 month'::interval + '1980-01-01T12:00:00'::timestamp;
 ----
 1980-02-01T12:00:00
 
-query D
+query error DataFusion error: Error during planning: Cannot coerce arithmetic expression Interval\(MonthDayNano\) \- Date32 to valid types
 select '1 month'::interval - '1980-01-01'::date;
-----
-1979-12-01
 
-query P
+query error DataFusion error: Error during planning: Cannot coerce arithmetic expression Interval\(MonthDayNano\) \- Timestamp\(Nanosecond, None\) to valid types
 select '1 month'::interval - '1980-01-01T12:00:00'::timestamp;
-----

Review Comment:
   I agree an interval minus a timestamp doesn't make a lot of sense



##########
datafusion/core/tests/sqllogictests/test_files/binary.slt:
##########
@@ -110,14 +110,19 @@ ff0102 FixedSizeBinary(3)
 000102 FixedSizeBinary(3)
 
 # Comparison
-# https://github.com/apache/arrow-rs/pull/4492
-query error DataFusion error: Arrow error: Compute error: eq_dyn_binary_scalar only supports Binary or LargeBinary arrays

Review Comment:
   FYI @maxburke 



##########
datafusion/core/tests/sqllogictests/test_files/arrow_typeof.slt:
##########
@@ -303,9 +303,11 @@ select arrow_cast('30 minutes', 'Interval(MonthDayNano)');
 
 ## Duration
 
-query error DataFusion error: Optimizer rule 'simplify_expressions' failed\ncaused by\nThis feature is not implemented: Can't create a scalar from array of type "Duration\(Second\)"
+query ?

Review Comment:
   🎉 



-- 
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] tustvold commented on a diff in pull request #6832: Update Arrow 44.0.0 And Datum Arithmetic

Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832#discussion_r1267181751


##########
datafusion/physical-expr/src/expressions/negative.rs:
##########
@@ -236,9 +179,6 @@ mod tests {
         test_array_negative_op!(Int64, 23456i64, 12345i64);
         test_array_negative_op!(Float32, 2345.0f32, 1234.0f32);
         test_array_negative_op!(Float64, 23456.0f64, 12345.0f64);
-        test_array_negative_op_intervals!(YearMonth, 2345i32, 1234i32);
-        test_array_negative_op_intervals!(DayTime, 23456i64, 12345i64);
-        test_array_negative_op_intervals!(MonthDayNano, 234567i128, 123456i128);

Review Comment:
   Arrow cast doesn't appear to support intervals in my experiments



-- 
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] Dandandan commented on pull request #6832: Update Arrow 45.0.0 And Datum Arithmetic, change Decimal Division semantics

Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan commented on PR #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832#issuecomment-1663454733

   Does this also fix https://github.com/apache/arrow-datafusion/issues/6794 (see https://github.com/apache/arrow-datafusion/issues/5646#issuecomment-1663422469)


-- 
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 #6832: Update Arrow 45.0.0 And Datum Arithmetic, change Decimal Division semantics

Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832#issuecomment-1681100995

   > @tustvold @alamb @viirya spark has the config to control the `precision loss` : https://github.com/apache/spark/blob/2be20e54a2222f6cdf64e8486d1910133b43665f/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala#L246
   
   I know it. But so?
   
   


-- 
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] tustvold commented on a diff in pull request #6832: Datum arithmetic

Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832#discussion_r1261585761


##########
datafusion/physical-expr/src/expressions/negative.rs:
##########
@@ -236,9 +179,6 @@ mod tests {
         test_array_negative_op!(Int64, 23456i64, 12345i64);
         test_array_negative_op!(Float32, 2345.0f32, 1234.0f32);
         test_array_negative_op!(Float64, 23456.0f64, 12345.0f64);
-        test_array_negative_op_intervals!(YearMonth, 2345i32, 1234i32);
-        test_array_negative_op_intervals!(DayTime, 23456i64, 12345i64);
-        test_array_negative_op_intervals!(MonthDayNano, 234567i128, 123456i128);

Review Comment:
   This is moved into sqllogic, unfortunately I couldn't find a way to get a MonthDayNano type, but this is better than nothing



-- 
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] tustvold commented on pull request #6832: Update Arrow 44.0.0 And Datum Arithmetic

Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832#issuecomment-1656725527

   https://github.com/apache/arrow-rs/pull/4581 adds the ability to both configure how durations are displayed, and declare const FormatOptions and CastOptions. This will allow DataFusion to consistently override how durations are displayed


-- 
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] tustvold commented on a diff in pull request #6832: Update Arrow 44.0.0 And Datum Arithmetic

Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832#discussion_r1268522941


##########
datafusion/core/tests/sql/timestamp.rs:
##########
@@ -1035,13 +1035,13 @@ async fn timestamp_sub_with_tz() -> Result<()> {
     let sql = "SELECT val, ts1 - ts2 AS ts_diff FROM table_a ORDER BY ts2 - ts1";
     let actual = execute_to_batches(&ctx, sql).await;
     let expected = vec![
-        "+-----+---------------------------------------------------+",
-        "| val | ts_diff                                           |",
-        "+-----+---------------------------------------------------+",
-        "| 3   | 0 years 0 mons 0 days 10 hours 0 mins 30.000 secs |",
-        "| 1   | 0 years 0 mons 0 days 10 hours 0 mins 20.000 secs |",
-        "| 2   | 0 years 0 mons 0 days 10 hours 0 mins 10.000 secs |",
-        "+-----+---------------------------------------------------+",
+        "+-----+---------+",
+        "| val | ts_diff |",
+        "+-----+---------+",
+        "| 3   | PT30S   |",

Review Comment:
   I think we should probably change duration output to be a number of the duration unit, i.e. `5000 seconds`, or `2500 milliseconds`, etc... to avoid potential ambiguity over what the duration represents. It might be confusing if both intervals and durations report some number of days, but arithmetic involving them has different semantics across DST boundaries. I will file an upstream ticket and work on 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

[GitHub] [arrow-datafusion] tustvold commented on a diff in pull request #6832: Update Arrow 44.0.0 And Datum Arithmetic

Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832#discussion_r1268522941


##########
datafusion/core/tests/sql/timestamp.rs:
##########
@@ -1035,13 +1035,13 @@ async fn timestamp_sub_with_tz() -> Result<()> {
     let sql = "SELECT val, ts1 - ts2 AS ts_diff FROM table_a ORDER BY ts2 - ts1";
     let actual = execute_to_batches(&ctx, sql).await;
     let expected = vec![
-        "+-----+---------------------------------------------------+",
-        "| val | ts_diff                                           |",
-        "+-----+---------------------------------------------------+",
-        "| 3   | 0 years 0 mons 0 days 10 hours 0 mins 30.000 secs |",
-        "| 1   | 0 years 0 mons 0 days 10 hours 0 mins 20.000 secs |",
-        "| 2   | 0 years 0 mons 0 days 10 hours 0 mins 10.000 secs |",
-        "+-----+---------------------------------------------------+",
+        "+-----+---------+",
+        "| val | ts_diff |",
+        "+-----+---------+",
+        "| 3   | PT30S   |",

Review Comment:
   I think we should probably change duration output to be a number of the duration unit, i.e. seconds, milliseconds, etc... to avoid potential ambiguity over what the duration represents. It might be confusing if both intervals and durations report some number of days, but arithmetic involving them has different semantics across DST boundaries



-- 
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] tustvold commented on pull request #6832: Update Arrow 45.0.0 And Datum Arithmetic, change Decimal Division semantics

Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832#issuecomment-1664488998

   https://github.com/apache/arrow-rs/pull/4640 proposes a further tweak to the decimal arithmetic rules, in light of #6828 I'm not sure this should be a blocker to getting this in, but defer to the consensus. Worst case we could do a patch release for arrow-rs, but I would rather avoid this if possible 


-- 
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] tustvold commented on pull request #6832: Update Arrow 45.0.0 And Datum Arithmetic, change Decimal Division semantics

Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832#issuecomment-1663712120

   It should do, but I haven't verified 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] alamb commented on pull request #6832: Update Arrow 45.0.0 And Datum Arithmetic, change Decimal Division semantics

Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832#issuecomment-1669939519

   I am running this update against the IOx tests as one final check: https://github.com/influxdata/influxdb_iox/pull/8452


-- 
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 #6832: Update Arrow 45.0.0 And Datum Arithmetic, change Decimal Division semantics

Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832#issuecomment-1670301176

   > @viirya / @waitingkuo / @liukun4515 I wonder if you could verify this PR is acceptable (specifically the Decimal display differences as well as the change in decimal division semantics)
   
   I only concern the change of decimal division semantics could be an impact to some customers potentially if high precision decimal results are important to them. Ideally I'd like to get same precision results as Spark but under current design I think it might be tricky to have two different precision rules for decimal operations. It could possibly make DataFusion complicated on decimal operation too. So it may not be a good idea. Among existing systems, I also don't know if any provides alternative option regarding decimal precision rule.


-- 
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] tustvold commented on a diff in pull request #6832: POC: Datum arithmetic

Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832#discussion_r1252122244


##########
datafusion/core/tests/sqllogictests/test_files/interval.slt:
##########
@@ -430,15 +430,11 @@ select '1 month'::interval + '1980-01-01T12:00:00'::timestamp;
 ----
 1980-02-01T12:00:00
 
-query D
+query error DataFusion error: Error during planning: Cannot coerce arithmetic expression Interval\(MonthDayNano\) \- Date32 to valid types

Review Comment:
   This query was only supported following #6778 and was done so accidentally - https://github.com/apache/arrow-datafusion/pull/6778#discussion_r1243994954
   
   I tested some other systems, duckdb explicitly doesn't support this, and I couldn't work out how to get mysql to express this, so I think this is fine



-- 
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] liukun4515 commented on pull request #6832: Update Arrow 45.0.0 And Datum Arithmetic, change Decimal Division semantics

Posted by "liukun4515 (via GitHub)" <gi...@apache.org>.
liukun4515 commented on PR #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832#issuecomment-1680098056

   > > @viirya / @waitingkuo / @liukun4515 I wonder if you could verify this PR is acceptable (specifically the Decimal display differences as well as the change in decimal division semantics)
   > 
   > I only concern the change of decimal division semantics could be an impact to some customers potentially if high precision decimal results are important to them. Ideally I'd like to get same precision results as Spark but under current design I think it might be tricky to have two different precision rules for decimal operations. It could possibly make DataFusion complicated on decimal operation too. So it may not be a good idea. Among existing systems, I also don't know if any provides alternative option regarding decimal precision rule.
   
   @tustvold @alamb @viirya 
   spark has the config to control the `precision loss` : https://github.com/apache/spark/blob/2be20e54a2222f6cdf64e8486d1910133b43665f/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala#L246


-- 
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] alamb commented on a diff in pull request #6832: Update Arrow 44.0.0 And Datum Arithmetic

Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832#discussion_r1268065417


##########
datafusion/core/tests/sql/timestamp.rs:
##########
@@ -1035,13 +1035,13 @@ async fn timestamp_sub_with_tz() -> Result<()> {
     let sql = "SELECT val, ts1 - ts2 AS ts_diff FROM table_a ORDER BY ts2 - ts1";
     let actual = execute_to_batches(&ctx, sql).await;
     let expected = vec![
-        "+-----+---------------------------------------------------+",
-        "| val | ts_diff                                           |",
-        "+-----+---------------------------------------------------+",
-        "| 3   | 0 years 0 mons 0 days 10 hours 0 mins 30.000 secs |",
-        "| 1   | 0 years 0 mons 0 days 10 hours 0 mins 20.000 secs |",
-        "| 2   | 0 years 0 mons 0 days 10 hours 0 mins 10.000 secs |",
-        "+-----+---------------------------------------------------+",
+        "+-----+---------+",
+        "| val | ts_diff |",
+        "+-----+---------+",
+        "| 3   | PT30S   |",

Review Comment:
   Update here is that @tustvold  and I spoke, and the next action is I will file a ticket explaining the behavior change and trying to build some consensus on what to do 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] tustvold commented on pull request #6832: Update Arrow 44.0.0 And Datum Arithmetic

Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832#issuecomment-1642604829

   Days aren't necessarily 24 hours, e.g. at a DST boundary


-- 
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] waitingkuo commented on pull request #6832: Update Arrow 44.0.0 And Datum Arithmetic

Posted by "waitingkuo (via GitHub)" <gi...@apache.org>.
waitingkuo commented on PR #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832#issuecomment-1642595688

   @berkaysynnada @tustvold @alamb 
   
   currently we couldn't covert interval back to duration if it contains mons or days
   
   ```bash
   ❯ select arrow_cast(interval '1 day', 'Duration(Nanosecond)');
   Optimizer rule 'simplify_expressions' failed
   caused by
   Arrow error: Compute error: Cannot cast to Duration(Nanosecond). Overflowing on 18446744073709551616
   ```
   
   however pure nanos representation works
   
   ```bash
   ❯ select arrow_cast(interval '24 hours', 'Duration(Nanosecond)');
   +----------------------------------------+
   | IntervalMonthDayNano("86400000000000") |
   +----------------------------------------+
   | P1D                                    |
   +----------------------------------------+
   1 row in set. Query took 0.002 seconds.
   ```
   
   it make sense to raise error while converting month unit, but for days should we fix 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

[GitHub] [arrow-datafusion] viirya commented on a diff in pull request #6832: Update Arrow 45.0.0 And Datum Arithmetic

Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832#discussion_r1278622152


##########
datafusion/core/tests/sqllogictests/test_files/decimal.slt:
##########
@@ -365,7 +365,7 @@ select c1*c5 from decimal_simple;
 query T
 select arrow_typeof(c1/cast(0.00001 as decimal(5,5))) from decimal_simple limit 1;
 ----
-Decimal128(21, 12)
+Decimal128(19, 10)

Review Comment:
   I won't say this is not correct, this is more like a design decision to me. But this actually changes the precision of decimal arithmetic operations in DataFusion. For some, this might be a different change and not something could be implicitly accepted. I'd suggest there are some documents on these changes with new release including 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 #6832: Update Arrow 45.0.0 And Datum Arithmetic

Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832#discussion_r1278621222


##########
datafusion/physical-expr/src/expressions/binary.rs:
##########
@@ -2609,14 +2441,14 @@ mod tests {
             Operator::Divide,
             create_decimal_array(
                 &[
-                    Some(99193548387), // 0.99193548387

Review Comment:
   I think that the rules for precision of decimal division are changed. I won't say previous rules are incorrect.



-- 
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] alamb commented on pull request #6832: Update Arrow 45.0.0 And Datum Arithmetic, change Decimal Division semantics

Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832#issuecomment-1663899116

   I have posted an announcement about this PR to the mailing list: https://lists.apache.org/thread/837ghhjh9gd1kvg0qqnmxqs0rm62x0xt
   
   I will plan to merge it in Monday Aug 7 2023 unless I hear otherwise


-- 
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] alamb commented on a diff in pull request #6832: Update Arrow 45.0.0 And Datum Arithmetic

Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832#discussion_r1279155844


##########
datafusion/core/tests/sqllogictests/test_files/dates.slt:
##########
@@ -91,17 +91,19 @@ where d3_date > now() + '5 days';
 
 # DATE minus DATE
 # https://github.com/apache/arrow-rs/issues/4383
-query error DataFusion error: Optimizer rule 'simplify_expressions' failed\ncaused by\nArrow error: Cast error: Cannot perform arithmetic operation between array of type Date32 and array of type Date32
+query ?
 SELECT DATE '2023-04-09' - DATE '2023-04-02';
+----
+7 days 0 hours 0 mins 0 secs
 
 # DATE minus Timestamp
 query ?
 SELECT DATE '2023-04-09' - '2000-01-01T00:00:00'::timestamp;
 ----
-0 years 0 mons 8499 days 0 hours 0 mins 0.000000000 secs
+8499 days 0 hours 0 mins 0.000000000 secs

Review Comment:
   much better



##########
datafusion/core/tests/sqllogictests/test_files/decimal.slt:
##########
@@ -365,7 +365,7 @@ select c1*c5 from decimal_simple;
 query T
 select arrow_typeof(c1/cast(0.00001 as decimal(5,5))) from decimal_simple limit 1;
 ----
-Decimal128(21, 12)
+Decimal128(19, 10)

Review Comment:
   I think spark is of the other major targets / comparison system -- I think @liukun4515  and maybe @yjshen  perhaps would have more context / understanding of how potential changes to the decimal division semantics may affect downstream users
   
   If there is need for both semantics, maybe we could add a  "use_spark_decimal_division_rules" config option to DataFusion
    



-- 
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] alamb commented on pull request #6832: Update Arrow 45.0.0 And Datum Arithmetic, change Decimal Division semantics

Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832#issuecomment-1669982731

   One final shoutout for this cleanup in this PR:
   
   <img width="272" alt="Screenshot 2023-08-08 at 12 56 17 PM" src="https://github.com/apache/arrow-datafusion/assets/490673/d4cf3751-9cd3-43ba-a39e-e3c4589b9973">
   


-- 
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] tustvold commented on a diff in pull request #6832: POC: Datum arithmetic

Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #6832:
URL: https://github.com/apache/arrow-datafusion/pull/6832#discussion_r1252123045


##########
datafusion/core/tests/sql/timestamp.rs:
##########
@@ -1035,13 +1035,13 @@ async fn timestamp_sub_with_tz() -> Result<()> {
     let sql = "SELECT val, ts1 - ts2 AS ts_diff FROM table_a ORDER BY ts2 - ts1";
     let actual = execute_to_batches(&ctx, sql).await;
     let expected = vec![
-        "+-----+---------------------------------------------------+",
-        "| val | ts_diff                                           |",
-        "+-----+---------------------------------------------------+",
-        "| 3   | 0 years 0 mons 0 days 10 hours 0 mins 30.000 secs |",
-        "| 1   | 0 years 0 mons 0 days 10 hours 0 mins 20.000 secs |",
-        "| 2   | 0 years 0 mons 0 days 10 hours 0 mins 10.000 secs |",
-        "+-----+---------------------------------------------------+",
+        "+-----+---------+",
+        "| val | ts_diff |",
+        "+-----+---------+",
+        "| 3   | PT30S   |",

Review Comment:
   Timestamp arithmetic now returns the appropriate duration type



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