Skip to content

Conversation

@rok
Copy link
Member

@rok rok commented Jan 13, 2022

@github-actions
Copy link

@rok
Copy link
Member Author

rok commented Jan 13, 2022

@lidavidm similar question about units as in #12124 - should we only cover matching time units (e.g. subtract(timestamp[s], duration[s]) -> timestamp[s]) or allow non-matching units too (e.g. subtract(timestamp[s], duration[ms]) -> timestamp[s])?

@jorisvandenbossche
Copy link
Member

jorisvandenbossche commented Jan 13, 2022

I would expect that for non-matching resolution in the input, we cast to the most detailed resolution. But, this doesn't necessarily need to have kernels for all combinations of resolutions. I think the casting step before calling the kernel ("DispatchBest") should take care of that (like add(int64, int32) -> int64 is not a registered kernel (only add(int64, int64) is), but still works).

The same is true for subtract(timestamp, timestamp) -> duration, which is for now only implemented for matching resolutions, and also the automatic casting doesn't seem to work yet for timestamps.

@rok
Copy link
Member Author

rok commented Jan 19, 2022

@jorisvandenbossche heterogeneous time resolutions are now supported.

@jorisvandenbossche
Copy link
Member

This is now a test only change?

@rok
Copy link
Member Author

rok commented Jan 27, 2022

@jorisvandenbossche It would appear test + doc only yes. ARROW-14095 added the subtract(timestamp, duration) kernel and dispatch best path to get from subtract(date, duration) -> subtract(timestamp, duration).

| sign | Unary | Numeric | Int8/Float32/Float64 | \(2) |
+------------------+--------+----------------------------+----------------------------+-------+
| subtract | Binary | Numeric/Date/Duration | Numeric/Date/Duration | \(1) |
| subtract | Binary | Numeric/Timestamp/Duration | Numeric/Timestamp/Duration | \(1) |
Copy link
Member

Choose a reason for hiding this comment

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

Temporal instead of Timestamp?

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed Numeric/Timestamp/Duration -> Numeric/Temporal

auto seconds_3_tz = ArrayFromJSON(timestamp(TimeUnit::SECOND, "UTC"), R"([3, null])");
auto milliseconds_1k_tz =
ArrayFromJSON(timestamp(TimeUnit::MILLI, "UTC"), R"([1000, null])");
CheckScalarBinary(op, seconds_3, milliseconds_2k, milliseconds_1k);
Copy link
Member

Choose a reason for hiding this comment

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

nor here

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@rok rok requested a review from lidavidm January 28, 2022 14:55
@lidavidm lidavidm closed this in d747326 Jan 31, 2022
@ursabot
Copy link

ursabot commented Jan 31, 2022

Benchmark runs are scheduled for baseline = d8f8c09 and contender = d747326. d747326 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.73% ⬆️0.73%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants