You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/06/02 17:56:09 UTC
[GitHub] [arrow] iChauster opened a new pull request, #13302: ARROW-16741: [C++] Add Benchmarks for Binary Temporal Operations
iChauster opened a new pull request, #13302:
URL: https://github.com/apache/arrow/pull/13302
[JIRA](https://issues.apache.org/jira/browse/ARROW-16741)
--
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] rok commented on a diff in pull request #13302: ARROW-16741: [C++] Add Benchmarks for Binary Temporal Operations
Posted by GitBox <gi...@apache.org>.
rok commented on code in PR #13302:
URL: https://github.com/apache/arrow/pull/13302#discussion_r892603134
##########
cpp/src/arrow/compute/kernels/scalar_temporal_benchmark.cc:
##########
@@ -172,6 +194,10 @@ auto non_zoned = timestamp(TimeUnit::NANO);
#define DECLARE_TEMPORAL_BENCHMARKS_ZONED(OP) \
BENCHMARK_TEMPLATE(BenchmarkTemporal, OP, zoned)->Apply(SetArgs);
+#define DECLARE_TEMPORAL_BINARY_BENCHMARKS(OP) \
+ BENCHMARK_TEMPLATE(BenchmarkTemporalBinary, OP, non_zoned)->Apply(SetArgs); \
+ BENCHMARK_TEMPLATE(BenchmarkTemporalBinary, OP, zoned)->Apply(SetArgs);
Review Comment:
I assume you're referring to this part?
```
auto array = rand.Numeric<Int64Type>(array_size, kInt64Min, kInt64Max, args.null_proportion);
std::shared_ptr<DataType> timestamp_type = timestamp(TimeUnit::NANO, "Pacific/Marquesas");
EXPECT_OK_AND_ASSIGN(auto timestamp_array, array->View(timestamp_type));
```
So `int64_t` here represents an integer array. Before throwing it into the kernel we interpret it as a timestamp array which does not change the underlaying buffer. I believe 32 types use `Int32Type` and 64 use `Int64Type`. Perhaps you can just use [`RandomArrayGenerator`](https://github.com/apache/arrow/blob/master/cpp/src/arrow/testing/random.h) to generate needed arrays.
--
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] rok commented on pull request #13302: ARROW-16741: [C++] Add Benchmarks for Binary Temporal Operations
Posted by GitBox <gi...@apache.org>.
rok commented on PR #13302:
URL: https://github.com/apache/arrow/pull/13302#issuecomment-1151352636
@iChauster here are the [temporal benchmarks for this PR](https://conbench.ursa.dev/compare/runs/85289eeb3c794766948e9065016c4717...aabd4c1c4d934cfa81256b6f772453f1/) if you're curious.
--
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] iChauster commented on pull request #13302: ARROW-16741: [C++] Add Benchmarks for Binary Temporal Operations
Posted by GitBox <gi...@apache.org>.
iChauster commented on PR #13302:
URL: https://github.com/apache/arrow/pull/13302#issuecomment-1151104439
@rok the benchmarks run great locally and fall into a similar range as the `TemporalRounding` statistics.
Thanks @lidavidm for approving, I'm assuming someone with write access will merge eventually?
--
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] iChauster commented on pull request #13302: ARROW-16741: [C++] Add Benchmarks for Binary Temporal Operations
Posted by GitBox <gi...@apache.org>.
iChauster commented on PR #13302:
URL: https://github.com/apache/arrow/pull/13302#issuecomment-1150384413
@rok, excellent, I think the binary benchmarks now support different datatypes for their respective kernels. Is this ready to be merged now?
--
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] rok commented on a diff in pull request #13302: ARROW-16741: [C++] Add Benchmarks for Binary Temporal Operations
Posted by GitBox <gi...@apache.org>.
rok commented on code in PR #13302:
URL: https://github.com/apache/arrow/pull/13302#discussion_r892805514
##########
cpp/src/arrow/compute/kernels/scalar_temporal_benchmark.cc:
##########
@@ -172,6 +194,10 @@ auto non_zoned = timestamp(TimeUnit::NANO);
#define DECLARE_TEMPORAL_BENCHMARKS_ZONED(OP) \
BENCHMARK_TEMPLATE(BenchmarkTemporal, OP, zoned)->Apply(SetArgs);
+#define DECLARE_TEMPORAL_BINARY_BENCHMARKS(OP) \
+ BENCHMARK_TEMPLATE(BenchmarkTemporalBinary, OP, non_zoned)->Apply(SetArgs); \
+ BENCHMARK_TEMPLATE(BenchmarkTemporalBinary, OP, zoned)->Apply(SetArgs);
Review Comment:
Yeah, not all kernels will support all types.
--
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] rok commented on pull request #13302: ARROW-16741: [C++] Add Benchmarks for Binary Temporal Operations
Posted by GitBox <gi...@apache.org>.
rok commented on PR #13302:
URL: https://github.com/apache/arrow/pull/13302#issuecomment-1150411617
> @rok, excellent, I think the binary benchmarks now support different datatypes for their respective kernels. Is this ready to be merged now?
Looks good to me. Do benchmarks run ok for you locally?
We need a committer to merge this. @lidavidm probably knows these functions best.
--
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] iChauster commented on a diff in pull request #13302: ARROW-16741: [C++] Add Benchmarks for Binary Temporal Operations
Posted by GitBox <gi...@apache.org>.
iChauster commented on code in PR #13302:
URL: https://github.com/apache/arrow/pull/13302#discussion_r892779444
##########
cpp/src/arrow/compute/kernels/scalar_temporal_benchmark.cc:
##########
@@ -172,6 +194,10 @@ auto non_zoned = timestamp(TimeUnit::NANO);
#define DECLARE_TEMPORAL_BENCHMARKS_ZONED(OP) \
BENCHMARK_TEMPLATE(BenchmarkTemporal, OP, zoned)->Apply(SetArgs);
+#define DECLARE_TEMPORAL_BINARY_BENCHMARKS(OP) \
+ BENCHMARK_TEMPLATE(BenchmarkTemporalBinary, OP, non_zoned)->Apply(SetArgs); \
+ BENCHMARK_TEMPLATE(BenchmarkTemporalBinary, OP, zoned)->Apply(SetArgs);
Review Comment:
Not sure why, but it seems like `date64` is viable as an input type, but `time64` / `time32`, and `duration` are not.
--
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] rok commented on a diff in pull request #13302: ARROW-16741: [C++] Add Benchmarks for Binary Temporal Operations
Posted by GitBox <gi...@apache.org>.
rok commented on code in PR #13302:
URL: https://github.com/apache/arrow/pull/13302#discussion_r892593123
##########
cpp/src/arrow/compute/kernels/scalar_temporal_benchmark.cc:
##########
@@ -214,5 +240,19 @@ BENCHMARK_TEMPLATE(BenchmarkStrptime, non_zoned)->Apply(SetArgs);
BENCHMARK_TEMPLATE(BenchmarkStrptime, zoned)->Apply(SetArgs);
BENCHMARK(BenchmarkAssumeTimezone)->Apply(SetArgs);
+// binary temporal benchmarks
Review Comment:
Actually, maybe the codepaths are not that different. Feel free to ignore 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] iChauster commented on a diff in pull request #13302: ARROW-16741: [C++] Add Benchmarks for Binary Temporal Operations
Posted by GitBox <gi...@apache.org>.
iChauster commented on code in PR #13302:
URL: https://github.com/apache/arrow/pull/13302#discussion_r892773713
##########
cpp/src/arrow/compute/kernels/scalar_temporal_benchmark.cc:
##########
@@ -172,6 +194,10 @@ auto non_zoned = timestamp(TimeUnit::NANO);
#define DECLARE_TEMPORAL_BENCHMARKS_ZONED(OP) \
BENCHMARK_TEMPLATE(BenchmarkTemporal, OP, zoned)->Apply(SetArgs);
+#define DECLARE_TEMPORAL_BINARY_BENCHMARKS(OP) \
+ BENCHMARK_TEMPLATE(BenchmarkTemporalBinary, OP, non_zoned)->Apply(SetArgs); \
+ BENCHMARK_TEMPLATE(BenchmarkTemporalBinary, OP, zoned)->Apply(SetArgs);
Review Comment:
Hi @rok, thank you for the clarification!
I refactored the code to work for the other data types, however it seems like (many of) these temporal binary functions [currently do not support any other datatype](https://github.com/apache/arrow/blob/master/cpp/src/arrow/compute/kernels/scalar_temporal_binary.cc#L461) than `Int64`.
When running my code with something like `date32` or `time64` with `random.ArrayOf`, I get an `NotImplemented: Function years_between has no kernel matching input types (array[time64[ns]], array[time64[ns]])`,
Let me know if I've missed a detail!
**EDIT: Ah, I see my error now!**
--
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] iChauster commented on a diff in pull request #13302: ARROW-16741: [C++] Add Benchmarks for Binary Temporal Operations
Posted by GitBox <gi...@apache.org>.
iChauster commented on code in PR #13302:
URL: https://github.com/apache/arrow/pull/13302#discussion_r892773713
##########
cpp/src/arrow/compute/kernels/scalar_temporal_benchmark.cc:
##########
@@ -172,6 +194,10 @@ auto non_zoned = timestamp(TimeUnit::NANO);
#define DECLARE_TEMPORAL_BENCHMARKS_ZONED(OP) \
BENCHMARK_TEMPLATE(BenchmarkTemporal, OP, zoned)->Apply(SetArgs);
+#define DECLARE_TEMPORAL_BINARY_BENCHMARKS(OP) \
+ BENCHMARK_TEMPLATE(BenchmarkTemporalBinary, OP, non_zoned)->Apply(SetArgs); \
+ BENCHMARK_TEMPLATE(BenchmarkTemporalBinary, OP, zoned)->Apply(SetArgs);
Review Comment:
Hi @rok, thank you for the clarification!
I refactored the code to work for the other data types, however it seems like (many of) these temporal binary functions [currently do not support any other datatype](https://github.com/apache/arrow/blob/master/cpp/src/arrow/compute/kernels/scalar_temporal_binary.cc#L461) than `Int64`.
When running my code with something like `date32` or `time64` with `random.ArrayOf`, I get an `NotImplemented: Function years_between has no kernel matching input types (array[time64[ns]], array[time64[ns]])`,
Let me know if I've missed a detail!
--
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] iChauster commented on a diff in pull request #13302: ARROW-16741: [C++] Add Benchmarks for Binary Temporal Operations
Posted by GitBox <gi...@apache.org>.
iChauster commented on code in PR #13302:
URL: https://github.com/apache/arrow/pull/13302#discussion_r892773713
##########
cpp/src/arrow/compute/kernels/scalar_temporal_benchmark.cc:
##########
@@ -172,6 +194,10 @@ auto non_zoned = timestamp(TimeUnit::NANO);
#define DECLARE_TEMPORAL_BENCHMARKS_ZONED(OP) \
BENCHMARK_TEMPLATE(BenchmarkTemporal, OP, zoned)->Apply(SetArgs);
+#define DECLARE_TEMPORAL_BINARY_BENCHMARKS(OP) \
+ BENCHMARK_TEMPLATE(BenchmarkTemporalBinary, OP, non_zoned)->Apply(SetArgs); \
+ BENCHMARK_TEMPLATE(BenchmarkTemporalBinary, OP, zoned)->Apply(SetArgs);
Review Comment:
Hi @rok, thank you for the clarification!
I refactored the code to work for the other data types, however it seems like (many of) these temporal binary functions [currently do not support any other datatype](https://github.com/apache/arrow/blob/master/cpp/src/arrow/compute/kernels/scalar_temporal_binary.cc#L461) than `Int64`.
When running my code with something like `date32` or `time64` with `random.ArrayOf`, I get an `NotImplemented: Function years_between has no kernel matching input types (array[time64[ns]], array[time64[ns]])`
--
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] iChauster commented on a diff in pull request #13302: ARROW-16741: [C++] Add Benchmarks for Binary Temporal Operations
Posted by GitBox <gi...@apache.org>.
iChauster commented on code in PR #13302:
URL: https://github.com/apache/arrow/pull/13302#discussion_r892773713
##########
cpp/src/arrow/compute/kernels/scalar_temporal_benchmark.cc:
##########
@@ -172,6 +194,10 @@ auto non_zoned = timestamp(TimeUnit::NANO);
#define DECLARE_TEMPORAL_BENCHMARKS_ZONED(OP) \
BENCHMARK_TEMPLATE(BenchmarkTemporal, OP, zoned)->Apply(SetArgs);
+#define DECLARE_TEMPORAL_BINARY_BENCHMARKS(OP) \
+ BENCHMARK_TEMPLATE(BenchmarkTemporalBinary, OP, non_zoned)->Apply(SetArgs); \
+ BENCHMARK_TEMPLATE(BenchmarkTemporalBinary, OP, zoned)->Apply(SetArgs);
Review Comment:
Hi @rok, thank you for the clarification!
I refactored the code to work for the other data types, however it seems like these temporal binary functions [currently do not support any other datatype](https://github.com/apache/arrow/blob/master/cpp/src/arrow/compute/kernels/scalar_temporal_binary.cc#L461) than `Int64`.
When running my code with something like `date32` or `time64` with `random.ArrayOf`, I get an `NotImplemented: Function years_between has no kernel matching input types (array[time64[ns]], array[time64[ns]])`
--
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] github-actions[bot] commented on pull request #13302: ARROW-16741: [C++] Add Benchmarks for Binary Temporal Operations
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #13302:
URL: https://github.com/apache/arrow/pull/13302#issuecomment-1145158430
https://issues.apache.org/jira/browse/ARROW-16741
--
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] rok commented on pull request #13302: ARROW-16741: [C++] Add Benchmarks for Binary Temporal Operations
Posted by GitBox <gi...@apache.org>.
rok commented on PR #13302:
URL: https://github.com/apache/arrow/pull/13302#issuecomment-1150143539
Hey @iChauster sorry for the delay!
--
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] lidavidm commented on pull request #13302: ARROW-16741: [C++] Add Benchmarks for Binary Temporal Operations
Posted by GitBox <gi...@apache.org>.
lidavidm commented on PR #13302:
URL: https://github.com/apache/arrow/pull/13302#issuecomment-1151114097
I have write access yes was just waiting for CI to pass/it was the end of the day for me :)
--
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] rok commented on a diff in pull request #13302: ARROW-16741: [C++] Add Benchmarks for Binary Temporal Operations
Posted by GitBox <gi...@apache.org>.
rok commented on code in PR #13302:
URL: https://github.com/apache/arrow/pull/13302#discussion_r888253687
##########
cpp/src/arrow/compute/kernels/scalar_temporal_benchmark.cc:
##########
@@ -214,5 +240,19 @@ BENCHMARK_TEMPLATE(BenchmarkStrptime, non_zoned)->Apply(SetArgs);
BENCHMARK_TEMPLATE(BenchmarkStrptime, zoned)->Apply(SetArgs);
BENCHMARK(BenchmarkAssumeTimezone)->Apply(SetArgs);
+// binary temporal benchmarks
Review Comment:
Perhaps we could also add `add`, `add_checked`, `subtract`, `subtract_checked`, `multiply`, `multiply_checked`, `divide` and `divide_checked` here since they are binary?
These kernels are [defined here](https://github.com/apache/arrow/blob/master/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc#L1999). However they are a bit more heterogeneous type wise so they will require some care (e.g. `add(timestamp, timestamp)` is invalid, while `subtract(timestamp, timestamp)` isn't)
##########
cpp/src/arrow/compute/api_scalar.h:
##########
@@ -1446,6 +1446,163 @@ ARROW_EXPORT Result<Datum> AssumeTimezone(const Datum& values,
ARROW_EXPORT Result<Datum> IsDaylightSavings(const Datum& values,
ExecContext* ctx = NULLPTR);
+/// \brief Years Between finds the number of years between two values
+///
+/// \param[in] left input treated as the start time
+/// \param[in] right input treated as the end time
+/// \param[in] ctx the function execution context, optional
+/// \return the resulting datum
+///
+/// \since 8.0.0
+/// \note API not yet finalized
+ARROW_EXPORT Result<Datum> YearsBetween(const Datum& left, const Datum& right,
+ ExecContext* ctx = NULLPTR);
+
+/// \brief Quarters Between finds the number of quarters between two values
+///
+/// \param[in] left input treated as the start time
+/// \param[in] right input treated as the end time
+/// \param[in] ctx the function execution context, optional
+/// \return the resulting datum
+///
+/// \since 8.0.0
+/// \note API not yet finalized
+ARROW_EXPORT Result<Datum> QuartersBetween(const Datum& left, const Datum& right,
+ ExecContext* ctx = NULLPTR);
+
+/// \brief Months Between finds the number of month between two values
+///
+/// \param[in] left input treated as the start time
+/// \param[in] right input treated as the end time
+/// \param[in] ctx the function execution context, optional
+/// \return the resulting datum
+///
+/// \since 8.0.0
+/// \note API not yet finalized
+ARROW_EXPORT Result<Datum> MonthsBetween(const Datum& left, const Datum& right,
+ ExecContext* ctx = NULLPTR);
+
+/// \brief Weeks Between finds the number of weeks between two values
+///
+/// \param[in] left input treated as the start time
+/// \param[in] right input treated as the end time
+/// \param[in] ctx the function execution context, optional
+/// \return the resulting datum
+///
+/// \since 8.0.0
+/// \note API not yet finalized
+ARROW_EXPORT Result<Datum> WeeksBetween(const Datum& left, const Datum& right,
+ ExecContext* ctx = NULLPTR);
+
+/// \brief Month Day Nano Between finds the number of months, days, and nonaseconds
+/// between two values
+///
+/// \param[in] left input treated as the start time
+/// \param[in] right input treated as the end time
+/// \param[in] ctx the function execution context, optional
+/// \return the resulting datum
+///
+/// \since 8.0.0
+/// \note API not yet finalized
+ARROW_EXPORT Result<Datum> MonthDayNanoBetween(const Datum& left, const Datum& right,
+ ExecContext* ctx = NULLPTR);
+
+/// \brief DayTime Between finds the number of days and milliseconds between two values
+///
+/// \param[in] left input treated as the start time
+/// \param[in] right input treated as the end time
+/// \param[in] ctx the function execution context, optional
+/// \return the resulting datum
+///
+/// \since 8.0.0
+/// \note API not yet finalized
+ARROW_EXPORT Result<Datum> DayTimeBetween(const Datum& left, const Datum& right,
+ ExecContext* ctx = NULLPTR);
+
+/// \brief Days Between finds the number of days between two values
+///
+/// \param[in] left input treated as the start time
+/// \param[in] right input treated as the end time
+/// \param[in] ctx the function execution context, optional
+/// \return the resulting datum
+///
+/// \since 8.0.0
+/// \note API not yet finalized
+ARROW_EXPORT Result<Datum> DaysBetween(const Datum& left, const Datum& right,
+ ExecContext* ctx = NULLPTR);
+
+/// \brief Hours Between finds the number of hours between two values
+///
+/// \param[in] left input treated as the start time
+/// \param[in] right input treated as the end time
+/// \param[in] ctx the function execution context, optional
+/// \return the resulting datum
+///
+/// \since 8.0.0
+/// \note API not yet finalized
+ARROW_EXPORT Result<Datum> HoursBetween(const Datum& left, const Datum& right,
+ ExecContext* ctx = NULLPTR);
+
+/// \brief Minutes Between finds the number of minutes between two values
+///
+/// \param[in] left input treated as the start time
+/// \param[in] right input treated as the end time
+/// \param[in] ctx the function execution context, optional
+/// \return the resulting datum
+///
+/// \since 8.0.0
+/// \note API not yet finalized
+ARROW_EXPORT Result<Datum> MinutesBetween(const Datum& left, const Datum& right,
+ ExecContext* ctx = NULLPTR);
+
+/// \brief Seconds Between finds the number of hours between two values
+///
+/// \param[in] left input treated as the start time
+/// \param[in] right input treated as the end time
+/// \param[in] ctx the function execution context, optional
+/// \return the resulting datum
+///
+/// \since 8.0.0
+/// \note API not yet finalized
+ARROW_EXPORT Result<Datum> SecondsBetween(const Datum& left, const Datum& right,
+ ExecContext* ctx = NULLPTR);
+
+/// \brief Milliseconds Between finds the number of milliseconds between two values
+///
+/// \param[in] left input treated as the start time
+/// \param[in] right input treated as the end time
+/// \param[in] ctx the function execution context, optional
+/// \return the resulting datum
+///
+/// \since 8.0.0
+/// \note API not yet finalized
+ARROW_EXPORT Result<Datum> MillisecondsBetween(const Datum& left, const Datum& right,
+ ExecContext* ctx = NULLPTR);
+
+/// \brief Microseconds Between finds the number of microseconds between two values
+///
+/// \param[in] left input treated as the start time
+/// \param[in] right input treated as the end time
+/// \param[in] ctx the function execution context, optional
+/// \return the resulting datum
+///
+/// \since 8.0.0
+/// \note API not yet finalized
+ARROW_EXPORT Result<Datum> MicrosecondsBetween(const Datum& left, const Datum& right,
+ ExecContext* ctx = NULLPTR);
+
+/// \brief Nanoseconds Between finds the number of nanoseconds between two values
+///
+/// \param[in] left input treated as the start time
+/// \param[in] right input treated as the end time
+/// \param[in] ctx the function execution context, optional
+/// \return the resulting datum
+///
+/// \since 8.0.0
+/// \note API not yet finalized
+ARROW_EXPORT Result<Datum> NanoesecondsBetween(const Datum& left, const Datum& right,
+ ExecContext* ctx = NULLPTR);
Review Comment:
@lidavidm was this not mapped out for a reason?
##########
cpp/src/arrow/compute/kernels/scalar_temporal_benchmark.cc:
##########
@@ -172,6 +194,10 @@ auto non_zoned = timestamp(TimeUnit::NANO);
#define DECLARE_TEMPORAL_BENCHMARKS_ZONED(OP) \
BENCHMARK_TEMPLATE(BenchmarkTemporal, OP, zoned)->Apply(SetArgs);
+#define DECLARE_TEMPORAL_BINARY_BENCHMARKS(OP) \
+ BENCHMARK_TEMPLATE(BenchmarkTemporalBinary, OP, non_zoned)->Apply(SetArgs); \
+ BENCHMARK_TEMPLATE(BenchmarkTemporalBinary, OP, zoned)->Apply(SetArgs);
Review Comment:
For some cases you might want other time types here (date32 date64, time32, time64, duration).
--
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] iChauster commented on pull request #13302: ARROW-16741: [C++] Add Benchmarks for Binary Temporal Operations
Posted by GitBox <gi...@apache.org>.
iChauster commented on PR #13302:
URL: https://github.com/apache/arrow/pull/13302#issuecomment-1147592977
Hi @rok, unfortunately I can't re-request review as a first-time contributor but I was wondering if you could answer some of my clarification questions from a few days ago when you get the chance! Excited to get this merged in.
--
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] rok commented on pull request #13302: ARROW-16741: [C++] Add Benchmarks for Binary Temporal Operations
Posted by GitBox <gi...@apache.org>.
rok commented on PR #13302:
URL: https://github.com/apache/arrow/pull/13302#issuecomment-1151114505
@ursabot please benchmark command=cpp-micro --suite-filter=scalar-temporal
--
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] github-actions[bot] commented on pull request #13302: ARROW-16741: [C++] Add Benchmarks for Binary Temporal Operations
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #13302:
URL: https://github.com/apache/arrow/pull/13302#issuecomment-1145158468
:warning: Ticket **has not been started in JIRA**, please click 'Start Progress'.
--
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] lidavidm commented on a diff in pull request #13302: ARROW-16741: [C++] Add Benchmarks for Binary Temporal Operations
Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #13302:
URL: https://github.com/apache/arrow/pull/13302#discussion_r888279575
##########
cpp/src/arrow/compute/api_scalar.h:
##########
@@ -1446,6 +1446,163 @@ ARROW_EXPORT Result<Datum> AssumeTimezone(const Datum& values,
ARROW_EXPORT Result<Datum> IsDaylightSavings(const Datum& values,
ExecContext* ctx = NULLPTR);
+/// \brief Years Between finds the number of years between two values
+///
+/// \param[in] left input treated as the start time
+/// \param[in] right input treated as the end time
+/// \param[in] ctx the function execution context, optional
+/// \return the resulting datum
+///
+/// \since 8.0.0
+/// \note API not yet finalized
+ARROW_EXPORT Result<Datum> YearsBetween(const Datum& left, const Datum& right,
+ ExecContext* ctx = NULLPTR);
+
+/// \brief Quarters Between finds the number of quarters between two values
+///
+/// \param[in] left input treated as the start time
+/// \param[in] right input treated as the end time
+/// \param[in] ctx the function execution context, optional
+/// \return the resulting datum
+///
+/// \since 8.0.0
+/// \note API not yet finalized
+ARROW_EXPORT Result<Datum> QuartersBetween(const Datum& left, const Datum& right,
+ ExecContext* ctx = NULLPTR);
+
+/// \brief Months Between finds the number of month between two values
+///
+/// \param[in] left input treated as the start time
+/// \param[in] right input treated as the end time
+/// \param[in] ctx the function execution context, optional
+/// \return the resulting datum
+///
+/// \since 8.0.0
+/// \note API not yet finalized
+ARROW_EXPORT Result<Datum> MonthsBetween(const Datum& left, const Datum& right,
+ ExecContext* ctx = NULLPTR);
+
+/// \brief Weeks Between finds the number of weeks between two values
+///
+/// \param[in] left input treated as the start time
+/// \param[in] right input treated as the end time
+/// \param[in] ctx the function execution context, optional
+/// \return the resulting datum
+///
+/// \since 8.0.0
+/// \note API not yet finalized
+ARROW_EXPORT Result<Datum> WeeksBetween(const Datum& left, const Datum& right,
+ ExecContext* ctx = NULLPTR);
+
+/// \brief Month Day Nano Between finds the number of months, days, and nonaseconds
+/// between two values
+///
+/// \param[in] left input treated as the start time
+/// \param[in] right input treated as the end time
+/// \param[in] ctx the function execution context, optional
+/// \return the resulting datum
+///
+/// \since 8.0.0
+/// \note API not yet finalized
+ARROW_EXPORT Result<Datum> MonthDayNanoBetween(const Datum& left, const Datum& right,
+ ExecContext* ctx = NULLPTR);
+
+/// \brief DayTime Between finds the number of days and milliseconds between two values
+///
+/// \param[in] left input treated as the start time
+/// \param[in] right input treated as the end time
+/// \param[in] ctx the function execution context, optional
+/// \return the resulting datum
+///
+/// \since 8.0.0
+/// \note API not yet finalized
+ARROW_EXPORT Result<Datum> DayTimeBetween(const Datum& left, const Datum& right,
+ ExecContext* ctx = NULLPTR);
+
+/// \brief Days Between finds the number of days between two values
+///
+/// \param[in] left input treated as the start time
+/// \param[in] right input treated as the end time
+/// \param[in] ctx the function execution context, optional
+/// \return the resulting datum
+///
+/// \since 8.0.0
+/// \note API not yet finalized
+ARROW_EXPORT Result<Datum> DaysBetween(const Datum& left, const Datum& right,
+ ExecContext* ctx = NULLPTR);
+
+/// \brief Hours Between finds the number of hours between two values
+///
+/// \param[in] left input treated as the start time
+/// \param[in] right input treated as the end time
+/// \param[in] ctx the function execution context, optional
+/// \return the resulting datum
+///
+/// \since 8.0.0
+/// \note API not yet finalized
+ARROW_EXPORT Result<Datum> HoursBetween(const Datum& left, const Datum& right,
+ ExecContext* ctx = NULLPTR);
+
+/// \brief Minutes Between finds the number of minutes between two values
+///
+/// \param[in] left input treated as the start time
+/// \param[in] right input treated as the end time
+/// \param[in] ctx the function execution context, optional
+/// \return the resulting datum
+///
+/// \since 8.0.0
+/// \note API not yet finalized
+ARROW_EXPORT Result<Datum> MinutesBetween(const Datum& left, const Datum& right,
+ ExecContext* ctx = NULLPTR);
+
+/// \brief Seconds Between finds the number of hours between two values
+///
+/// \param[in] left input treated as the start time
+/// \param[in] right input treated as the end time
+/// \param[in] ctx the function execution context, optional
+/// \return the resulting datum
+///
+/// \since 8.0.0
+/// \note API not yet finalized
+ARROW_EXPORT Result<Datum> SecondsBetween(const Datum& left, const Datum& right,
+ ExecContext* ctx = NULLPTR);
+
+/// \brief Milliseconds Between finds the number of milliseconds between two values
+///
+/// \param[in] left input treated as the start time
+/// \param[in] right input treated as the end time
+/// \param[in] ctx the function execution context, optional
+/// \return the resulting datum
+///
+/// \since 8.0.0
+/// \note API not yet finalized
+ARROW_EXPORT Result<Datum> MillisecondsBetween(const Datum& left, const Datum& right,
+ ExecContext* ctx = NULLPTR);
+
+/// \brief Microseconds Between finds the number of microseconds between two values
+///
+/// \param[in] left input treated as the start time
+/// \param[in] right input treated as the end time
+/// \param[in] ctx the function execution context, optional
+/// \return the resulting datum
+///
+/// \since 8.0.0
+/// \note API not yet finalized
+ARROW_EXPORT Result<Datum> MicrosecondsBetween(const Datum& left, const Datum& right,
+ ExecContext* ctx = NULLPTR);
+
+/// \brief Nanoseconds Between finds the number of nanoseconds between two values
+///
+/// \param[in] left input treated as the start time
+/// \param[in] right input treated as the end time
+/// \param[in] ctx the function execution context, optional
+/// \return the resulting datum
+///
+/// \since 8.0.0
+/// \note API not yet finalized
+ARROW_EXPORT Result<Datum> NanoesecondsBetween(const Datum& left, const Datum& right,
+ ExecContext* ctx = NULLPTR);
Review Comment:
I don't think there was any reason, I just neglected to add those.
--
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] ursabot commented on pull request #13302: ARROW-16741: [C++] Add Benchmarks for Binary Temporal Operations
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #13302:
URL: https://github.com/apache/arrow/pull/13302#issuecomment-1151114566
Benchmark runs are scheduled for baseline = fc082c5e8982f9db8366feaab25d6449b273d35e and contender = dc9cd205c9bfd68813806b88fad7eabd11783188. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped :warning: Only ['lang', 'name'] filters are supported on ec2-t3-xlarge-us-east-2] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/5eb75608b7a24eb1a40c2bb85c3ec920...4b2541fe5ec4462785b75a7c325c4472/)
[Skipped :warning: Only ['lang', 'name'] filters are supported on test-mac-arm] [test-mac-arm](https://conbench.ursa.dev/compare/runs/9e17c9b1a8444b3f93d4e85268f6d12f...9b623b78454c4bdf852b75cc6c2ae204/)
[Skipped :warning: Only ['lang', 'name'] filters are supported on ursa-i9-9960x] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/2a017862d76f467b9fd1b4d91117e53c...92939d989d72499d9b37abf7945092bd/)
[Scheduled] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/85289eeb3c794766948e9065016c4717...aabd4c1c4d934cfa81256b6f772453f1/)
Buildkite builds:
[Scheduled] [`dc9cd205` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/925)
[Finished] [`fc082c5e` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/881)
[Finished] [`fc082c5e` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/877)
[Failed] [`fc082c5e` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/863)
[Finished] [`fc082c5e` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/879)
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
--
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] lidavidm merged pull request #13302: ARROW-16741: [C++] Add Benchmarks for Binary Temporal Operations
Posted by GitBox <gi...@apache.org>.
lidavidm merged PR #13302:
URL: https://github.com/apache/arrow/pull/13302
--
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] rok commented on a diff in pull request #13302: ARROW-16741: [C++] Add Benchmarks for Binary Temporal Operations
Posted by GitBox <gi...@apache.org>.
rok commented on code in PR #13302:
URL: https://github.com/apache/arrow/pull/13302#discussion_r892604335
##########
cpp/src/arrow/compute/kernels/scalar_temporal_benchmark.cc:
##########
@@ -172,6 +194,10 @@ auto non_zoned = timestamp(TimeUnit::NANO);
#define DECLARE_TEMPORAL_BENCHMARKS_ZONED(OP) \
BENCHMARK_TEMPLATE(BenchmarkTemporal, OP, zoned)->Apply(SetArgs);
+#define DECLARE_TEMPORAL_BINARY_BENCHMARKS(OP) \
+ BENCHMARK_TEMPLATE(BenchmarkTemporalBinary, OP, non_zoned)->Apply(SetArgs); \
+ BENCHMARK_TEMPLATE(BenchmarkTemporalBinary, OP, zoned)->Apply(SetArgs);
Review Comment:
[A more complete example.](https://github.com/apache/arrow/blob/master/cpp/src/arrow/testing/generator.cc#L101-L155)
--
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] rok commented on a diff in pull request #13302: ARROW-16741: [C++] Add Benchmarks for Binary Temporal Operations
Posted by GitBox <gi...@apache.org>.
rok commented on code in PR #13302:
URL: https://github.com/apache/arrow/pull/13302#discussion_r892581372
##########
cpp/src/arrow/compute/kernels/scalar_temporal_benchmark.cc:
##########
@@ -214,5 +240,19 @@ BENCHMARK_TEMPLATE(BenchmarkStrptime, non_zoned)->Apply(SetArgs);
BENCHMARK_TEMPLATE(BenchmarkStrptime, zoned)->Apply(SetArgs);
BENCHMARK(BenchmarkAssumeTimezone)->Apply(SetArgs);
+// binary temporal benchmarks
Review Comment:
`scalar_arithmetic_benchmark.cc` benchmarks non-temporal arrays right now. It would be useful to also benchmark for temporal zoned/nonzoned temporal types as computation would follow a different codepath for those.
--
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] iChauster commented on a diff in pull request #13302: ARROW-16741: [C++] Add Benchmarks for Binary Temporal Operations
Posted by GitBox <gi...@apache.org>.
iChauster commented on code in PR #13302:
URL: https://github.com/apache/arrow/pull/13302#discussion_r892779444
##########
cpp/src/arrow/compute/kernels/scalar_temporal_benchmark.cc:
##########
@@ -172,6 +194,10 @@ auto non_zoned = timestamp(TimeUnit::NANO);
#define DECLARE_TEMPORAL_BENCHMARKS_ZONED(OP) \
BENCHMARK_TEMPLATE(BenchmarkTemporal, OP, zoned)->Apply(SetArgs);
+#define DECLARE_TEMPORAL_BINARY_BENCHMARKS(OP) \
+ BENCHMARK_TEMPLATE(BenchmarkTemporalBinary, OP, non_zoned)->Apply(SetArgs); \
+ BENCHMARK_TEMPLATE(BenchmarkTemporalBinary, OP, zoned)->Apply(SetArgs);
Review Comment:
Not sure why, but it seems like `date64` is viable as an input type, but `time64` / `time32`, and `duration` are not.
--
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] iChauster commented on a diff in pull request #13302: ARROW-16741: [C++] Add Benchmarks for Binary Temporal Operations
Posted by GitBox <gi...@apache.org>.
iChauster commented on code in PR #13302:
URL: https://github.com/apache/arrow/pull/13302#discussion_r888402428
##########
cpp/src/arrow/compute/kernels/scalar_temporal_benchmark.cc:
##########
@@ -214,5 +240,19 @@ BENCHMARK_TEMPLATE(BenchmarkStrptime, non_zoned)->Apply(SetArgs);
BENCHMARK_TEMPLATE(BenchmarkStrptime, zoned)->Apply(SetArgs);
BENCHMARK(BenchmarkAssumeTimezone)->Apply(SetArgs);
+// binary temporal benchmarks
Review Comment:
They are binary, but I believe these kernels are already benchmarked in `scalar_arithmetic_benchmark.cc`. Does this mean adding support for adding, subtracting, (multiplying and dividing!) timestamps, and then benchmarking those implementations?
##########
cpp/src/arrow/compute/kernels/scalar_temporal_benchmark.cc:
##########
@@ -172,6 +194,10 @@ auto non_zoned = timestamp(TimeUnit::NANO);
#define DECLARE_TEMPORAL_BENCHMARKS_ZONED(OP) \
BENCHMARK_TEMPLATE(BenchmarkTemporal, OP, zoned)->Apply(SetArgs);
+#define DECLARE_TEMPORAL_BINARY_BENCHMARKS(OP) \
+ BENCHMARK_TEMPLATE(BenchmarkTemporalBinary, OP, non_zoned)->Apply(SetArgs); \
+ BENCHMARK_TEMPLATE(BenchmarkTemporalBinary, OP, zoned)->Apply(SetArgs);
Review Comment:
Hi @rok, thanks for the timely review!
I was a bit curious about this, since it seems for `BenchmarkTemporalBinary` and `BenchmarkTemporal` (the original unary version), we are picking random `int64_t` -- which I assume is `date64`. How do we achieve the other time types, and is there some helpful example code I can follow?
--
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