Add support for adding intervals to dates #2031

avantgardnerio · 2022-07-08T17:46:18Z

Rationale for this change

Support for adding scalar intervals to scalar dates was added in datafusion, but in order to support adding columns to columns, we need to move that logic down into an arrow kernel.

What changes are included in this PR?

Support for adding interval types to date types
A change to add_dyn to allow it to accept lambda functions with two differing parameter types

Are there any user-facing changes?

Adding intervals to dates should work now.

arrow/src/compute/kernels/arithmetic.rs

andygrove

LGTM. Thanks @avantgardnerio. It would be good to get a review from @alamb as well.

alamb · 2022-07-08T19:40:22Z

Looks like there is a minor clippy error: https://github.com/apache/arrow-rs/runs/7256871633?check_suite_focus=true

avantgardnerio · 2022-07-08T19:57:29Z

Looks like there is a minor clippy error: https://github.com/apache/arrow-rs/runs/7256871633?check_suite_focus=true

I had to re-install ubuntu to get clippy to work, but the next iteration should pass.

alamb

Thank you @avantgardnerio -- I really like where this is headed.

cc @tustvold @viirya and @HaoYang670

arrow/Cargo.toml

arrow/src/compute/kernels/arithmetic.rs

arrow/src/datatypes/types.rs

arrow/src/compute/kernels/arithmetic.rs

viirya · 2022-07-08T21:26:23Z

arrow/src/datatypes/delta.rs

+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+// SOFTWARE.
+
+// Copied from chronoutil crate


Why copying the code, instead of using it as dependency?

There was a conversation in the equivalent arrow-rs commit: apache/datafusion#2797 (comment)

I would vote to make it a dependency, but am fine either way.

Sorry for the contradictory feedback -- I defer to @viirya in terms of using dependency or inlining. Given how little actual code it is, I kind of like the avoidance of a new dependency but I agree this is an opinion rather than anything driven by other considerations.

The reason I personally prefer to avoid dependencies are they are really hard to remove (e.g. #1882) and there are many of these rust crates which seem to have an initial spurt of brilliance and then basically become unmaintained as the authors move on to something else. Though to be honest chrono itself kind of falls into that category too 🤔

The Chrono folks re-opened my PR, so I don't think this will be a concern for too long - it looks like they are interested in building a add_months() function into chrono, we're just debating where and how.

I agree with @alamb. The code copied is little so not a strong concern from me. I think we can copy the code now and maybe consider to remove it by adding chrono as dependency if it shows stable maintenance in the future.

codecov-commenter · 2022-07-08T21:26:56Z

Codecov Report

Merging #2031 (5ef4a1f) into master (ca1bfb8) will increase coverage by 0.07%.
The diff coverage is 95.06%.

❗ Current head 5ef4a1f differs from pull request most recent head 911b2db. Consider uploading reports for the commit 911b2db to get more accurate results

@@            Coverage Diff             @@
##           master    #2031      +/-   ##
==========================================
+ Coverage   83.54%   83.61%   +0.07%     
==========================================
  Files         222      223       +1     
  Lines       58178    58460     +282     
==========================================
+ Hits        48604    48884     +280     
- Misses       9574     9576       +2

Impacted Files	Coverage Δ
arrow/src/datatypes/mod.rs	`99.24% <ø> (ø)`
parquet/src/arrow/array_reader/list_array.rs	`92.69% <0.00%> (ø)`
arrow/src/ffi.rs	`87.52% <72.72%> (+0.34%)`	⬆️
parquet/src/arrow/record_reader/mod.rs	`89.17% <79.59%> (-0.19%)`	⬇️
arrow/src/datatypes/ffi.rs	`76.56% <92.85%> (+3.83%)`	⬆️
arrow/src/compute/kernels/arithmetic.rs	`93.75% <95.18%> (+0.12%)`	⬆️
arrow/src/datatypes/types.rs	`97.46% <98.57%> (+8.57%)`	⬆️
arrow/src/array/builder/generic_binary_builder.rs	`83.78% <100.00%> (+26.64%)`	⬆️
arrow/src/array/builder/generic_list_builder.rs	`95.09% <100.00%> (-1.61%)`	⬇️
arrow/src/array/builder/generic_string_builder.rs	`92.13% <100.00%> (+10.08%)`	⬆️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ca1bfb8...911b2db. Read the comment docs.

viirya · 2022-07-08T21:28:07Z

arrow/src/datatypes/types.rs

@@ -191,3 +194,166 @@ impl ArrowTimestampType for TimestampNanosecondType {
        TimeUnit::Nanosecond
    }
 }
+
+impl IntervalYearMonthType {
+    /// Creates a IntervalYearMonthType


Hm, looks like new returns the native value, not IntervalYearMonthType type?

Yes, I struggled a lot with this. It's very confusing the distinction between these. It technically returns a <Date64Type as IntervalYearMonthType>::Native which is just an i32. I question where to even put these methods, or if they belong on this impl, or what to call them in the first place. I think the awkwardness comes from arrow not really having a row, so heretofor there was nothing that operated on an individual value, but also thus some bad tests that did not properly parse/populate these, so I think some standard way to convert would be very helpful, hence the addition.

I think the functionality for manipulating values on to this impl makes sense

Perhaps @viirya was referring to the rust convention that Type::new() returns an instance of Type -- perhaps if we renamed this method make_value or something like that it would be less surprising for other rust developers.

We could do it as a follow on PR as well (before releasing arrow 19.x) too

Yea, makes sense to me.

arrow/src/datatypes/types.rs

avantgardnerio · 2022-07-12T17:00:25Z

Could anyone help me with reproducing this locally, or understanding what went wrong?

==========================================================
Testing file duration
==========================================================
Traceback (most recent call last):
  File "/arrow/dev/archery/archery/integration/runner.py", line 246, in _run_ipc_test_case
    run_binaries(producer, consumer, test_case)
  File "/arrow/dev/archery/archery/integration/runner.py", line 286, in _produce_consume
    consumer.stream_to_file(producer_stream_path, consumer_file_path)
  File "/arrow/dev/archery/archery/integration/tester_csharp.py", line 63, in stream_to_file
    self.run_shell_command(cmd)
  File "/arrow/dev/archery/archery/integration/tester.py", line 49, in run_shell_command
    subprocess.check_call(cmd, shell=True)
  File "/opt/conda/envs/arrow/lib/python3.10/subprocess.py", line 369, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '/arrow/csharp/artifacts/Apache.Arrow.IntegrationTest/Debug/net6.0/Apache.Arrow.IntegrationTest --mode stream-to-file -a /tmp/tmpzg7b26id/29338ab4_generated_datetime.consumer_stream_as_file < /tmp/tmpzg7b26id/29338ab4_generated_datetime.producer_file_as_stream' returned non-zero exit status 1.
################# FAILURES #################
Traceback (most recent call last):
FAILED TEST: datetime Java producing,  C# consuming
  File "/arrow/dev/archery/archery/integration/runner.py", line 246, in _run_ipc_test_case
    run_binaries(producer, consumer, test_case)
  File "/arrow/dev/archery/archery/integration/runner.py", line 286, in _produce_consume
    consumer.stream_to_file(producer_stream_path, consumer_file_path)
  File "/arrow/dev/archery/archery/integration/tester_csharp.py", line 63, in stream_to_file
    self.run_shell_command(cmd)
  File "/arrow/dev/archery/archery/integration/tester.py", line 49, in run_shell_command
    subprocess.check_call(cmd, shell=True)
  File "/opt/conda/envs/arrow/lib/python3.10/subprocess.py", line 369, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '/arrow/csharp/artifacts/Apache.Arrow.IntegrationTest/Debug/net6.0/Apache.Arrow.IntegrationTest --mode stream-to-file -a /tmp/tmpzg7b26id/4b457ee3_generated_datetime.consumer_stream_as_file < /tmp/tmpzg7b26id/4b457ee3_generated_datetime.producer_file_as_stream' returned non-zero exit status 1.
FAILED TEST: datetime Rust producing,  C# consuming

Thanks!

tustvold · 2022-07-12T17:01:43Z

@avantgardnerio It's a flaky test #1931 that should have now been fixed apache/arrow#13573. I'll rerun the CI job and it should clear

avantgardnerio · 2022-07-12T17:37:22Z

@avantgardnerio It's a flaky test #1931 that should have now been fixed apache/arrow#13573. I'll rerun the CI job and it should clear

Looks great, thanks!

alamb

I think this looks great -- thank you @avantgardnerio for figuring out a pattern and starting to implement this arithmetic. ❤️

I left one more comment but I also think this PR could be merged as is.

@viirya or @tustvold would you like to review again prior to merging?

alamb · 2022-07-13T10:36:59Z

arrow/src/compute/kernels/arithmetic.rs

+            NaiveDate::from_ymd(2000, 1, 1),
+        )]);
+        let b = IntervalYearMonthArray::from(vec![IntervalYearMonthType::new(1, 2)]);
+        let c = add_dyn(&a, &b).unwrap();


it is so great to see add_dyn used like this ❤️

alamb · 2022-07-13T10:40:01Z

arrow/src/datatypes/types.rs

@@ -191,3 +194,166 @@ impl ArrowTimestampType for TimestampNanosecondType {
        TimeUnit::Nanosecond
    }
 }
+
+impl IntervalYearMonthType {
+    /// Creates a IntervalYearMonthType


I think the functionality for manipulating values on to this impl makes sense

Perhaps @viirya was referring to the rust convention that Type::new() returns an instance of Type -- perhaps if we renamed this method make_value or something like that it would be less surprising for other rust developers.

We could do it as a follow on PR as well (before releasing arrow 19.x) too

avantgardnerio · 2022-07-13T12:36:47Z

renamed this method make_value

I think that is both valid, and non-trivial. I've renamed and pushed 🙂

ursabot · 2022-07-15T15:31:27Z

Benchmark runs are scheduled for baseline = 9d8f0c9 and contender = cb7e5b0. cb7e5b0 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

alamb · 2022-07-15T15:34:58Z

🎉

alamb · 2022-08-12T10:24:31Z

@avantgardnerio I wonder if you are tracking the "add support for adding interval columns to dates/timestamps" in datafusion somewhere?

I ask because apache/datafusion#3110 by @JasonLi-cn is starting to add support for timestamps

avantgardnerio · 2022-08-12T15:27:07Z

if you are tracking

I was not, but am now, ty!

Can drop this after rebase on commit cb7e5b0 "Add support for adding intervals to dates (apache#2031)", first released in 19.0.0

github-actions bot added the arrow Changes to the arrow crate label Jul 8, 2022

avantgardnerio added 3 commits July 8, 2022 11:50

Adding dates and stuff

959a374

Use shift_months to not re-invent wheels

f1c812c

Add Date64

97cb277

andygrove reviewed Jul 8, 2022

View reviewed changes

arrow/src/compute/kernels/arithmetic.rs Outdated Show resolved Hide resolved

andygrove reviewed Jul 8, 2022

View reviewed changes

arrow/src/compute/kernels/arithmetic.rs Outdated Show resolved Hide resolved

Test values and formatting

ef88801

avantgardnerio force-pushed the bg_date_math branch from 3093692 to ef88801 Compare July 8, 2022 18:02

avantgardnerio requested a review from andygrove July 8, 2022 18:21

andygrove approved these changes Jul 8, 2022

View reviewed changes

Fix clippy feedback

e1bbe56

avantgardnerio requested a review from andygrove July 8, 2022 19:57

alamb reviewed Jul 8, 2022

View reviewed changes

avantgardnerio added 2 commits July 8, 2022 14:24

Verbosity, clippy errors, no std

c8a1404

PR feedback

2068071

avantgardnerio requested a review from alamb July 8, 2022 21:01