[REVIEW] duration types #5359

karthikeyann · 2020-06-02T14:29:35Z

addresses C++ part of #5272

add duration types
factories

GPUtester · 2020-06-02T14:30:12Z

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

jrhemstad · 2020-06-02T14:35:59Z

cpp/include/cudf/types.hpp

@@ -191,6 +195,16 @@ enum type_id {
  TIMESTAMP_MILLISECONDS,  ///< duration of milliseconds since Unix Epoch in int64
  TIMESTAMP_MICROSECONDS,  ///< duration of microseconds since Unix Epoch in int64
  TIMESTAMP_NANOSECONDS,   ///< duration of nanoseconds since Unix Epoch in int64
+  DURATION_YEARS,          ///< duration of years in int32


Whoah. My CPU just cried thinking about the compile time explosion this is going to cause. We don't need YEARS/MONTHS/WEEKS/~~DAYS~~/HOURS/MINUTES for duration since we don't have corresponding timestamp types for that.

We should only have/need durations for where we have timestamp types.

We need days since we have TIMESTAMP_DAYS, but otherwise agreed for now.

In the future the goal would be to support most of these though 😢

YEARS/MONTHS/WEEKS/DAYS/HOURS/MINUTES would be useful for constructing all timedelta operations. This would allow for eg. (+1 year + 1 month +1 days) operations with timestamp.
This is still open for discussion.

useful for constructing all timedelta operations

Useful, yes, practical, no. I do not think our compile time or library file size can handle add all of these types in addition to all of the other new ones being added.

This would allow for eg. (+1 year + 1 month +1 days) operations with timestamp.

You can still express that in terms of smaller units like DAYS. So these additional types are not essential for functionality.

FWIW, Arrow only supports durations on seconds, milliseconds, microseconds, and nanoseconds.

Numpy supports it on years, months, weeks, days, hour, and minutes as well but uses 64 bit types for all of them.

Maybe we only support seconds, milliseconds, microseconds, and nanoseconds for now, considering using a duration of 64 bit seconds allows you to capture all of a duration of 32 bit days, albeit with memory / performance tradeoffs.

For spark we need MONTHS, DAYS, and MICROSECONDS.

This is because if I want to go one year in the future it is 12 months, but it is not always 365 days. If I want to go 1 week in the future it is always 7 days, but I cannot always say how many microseconds it is.

I understand the use case of months, but isn't 1 week deterministic in the number of microseconds? https://godbolt.org/z/EhSau3

Well, looks like std::chrono::duration is all just ratio based: https://en.cppreference.com/w/cpp/chrono/duration

Unit Ratio

seconds 1

minutes 60

hours 3600

days 86400

weeks 604800

months 2629746

years 31556952

This is because if I want to go one year in the future it is 12 months, but it is not always 365 days.

According to C++ duration types, a year is always 365.2425 days. See:https://en.cppreference.com/w/cpp/chrono/duration

years is equal to 365.2425 days (the average length of a Gregorian year). months is equal to 30.436875 days (exactly 1/12 of years).

now, DAYS, SECONDS, MILLISECONDS, MICROSECONDS, NANOSECONDS are the only types in duration type.

kkraus14 · 2020-06-02T15:22:04Z

@karthikeyann Could we limit the scope of this PR to just the types and factories and use follow up PRs for other pieces? This also lets us parallelize the work more efficiently 😄

jrhemstad · 2020-06-02T15:53:01Z

cpp/src/dictionary/set_keys.cu

@@ -85,7 +85,6 @@ struct dispatch_compute_indices {
             cudaStream_t stream)
  {
    CUDF_FAIL("list_view dictionary set_keys not supported yet");
-    return nullptr;


I suspect some compiler versions are going to complain about not having a return value here. Furthermore, this is an unrelated change.

IIRC, compiler complains if return type is auto.

cpp/include/cudf/utilities/traits.hpp

sriramch · 2020-06-02T16:58:44Z

cpp/include/cudf/wrappers/durations.hpp

+namespace detail {
+
+template <class Duration>
+struct duration : Duration {


what is the purpose of this type and the already existing timestamp type? is it to:

implicitly convert the tick counts into duration and timestamp (timestamp_X{3} etc.)?

convert between 2 time_point that have different duration time periods where one can't be otherwise converted to another (from a higher precision to a lower precision - seconds to days etc.)?

is this what is preventing us from using?
using duration_D = simt::std::chrono::days; and using timestamp_D = detail::timestamp<duration_D>;

The duration helper types like std::chrono::days don't provide sufficient guarantees for the size of the type, e.g., std::chrono::days is only guaranteed to be at least 25 bits.

That said, I can't recall why we made the explicit timestamp type instead of doing:

using timestamp_D = simt::std::chrono::time_point<simt::std::chrono::system_clock, simt::std::chrono::duration<int32_t, simt::std::ratio<86400>>>

@trxcllnt do you remember why we didn't make the libcudf timestamp types simply be aliases and instead made a concrete new type? I'm sure we had a good reason, but I can't remember right now.

Yeah I think there were a few things:

We didn't want to make the clock part of the type, since we expected to switch to utc_clock eventually once it lands in libcu++.

We have an extra constructor that takes a numeric typename Duration::rep r arg that's more convenient to use: timestamp_ms{1000}

We needed to override the conversion constructor to explicitly call duration_cast() since the libcu++ impl doesn't call it (possible bug in libcu++?).

for the 2nd item above - should we be even supporting constructing a timestamp with a tick count? in this case, what does 1000 really represent? shouldn't it be a duration (of ms in this example) or another time_point with a similar or a different duration. presently, we interpret to mean add 1000 ms to the system_clock epoch (as we only support system_clock for now), which doesn't look to be the original intent, as 1000 could mean different units or displacement from some other clock.

i think 3rd item is required if we want to truncate and lose precision - i.e. assigning say timestamp_s to a timestamp_D. that constructor will prevent this if the periods aren't divisible - is_convertible should evaluate to true.

2). 2nd item is present for ease the testing code. timestamp(typename Duration::rep r) should be explicit.
3) 3rd item is not bug since is_convertible takes care of it. (lossy conversion is not allowed as per std::chrono, needs explicit duration_cast). std::chrono does not recommend using duration_cast (lossy conversion) implicitly. One plus point is that it's explicit constructor.
Perhaps lossy conversion is the requirement from users (Spark, Python, etc). is it?

@karthikeyann do you think we should move away from having our own concrete timestamp type and instead just us a type alias for both timestamps and durations?

We can't replace due to a CUDA limitation. If timestamp is replaced with simt::std::chrono::time_point, following error is hit due to limitation of shared memory initialization.
cpp/include/cudf/detail/copy_if.cuh(106): error: initializer not allowed for __shared__ variable

cudf/cpp/include/cudf/detail/copy_if.cuh

Line 106 in efebcd1

__shared__ T temp_data[block_size];

This is a limitation of device variable. https://docs.nvidia.com/cuda/cuda-c-programming-guide/index.html#device-memory-specifiers

The constructor function has no parameters, the initializer list is empty and the function body is an empty compound statement.

The default constructors of all base classes of its class can be considered empty.

simt::std::chrono::time_point calls parameterized constructor of duration
https://github.com/rapidsai/thirdparty-libcxx/blob/a97a7380c76346c22bb67b93695bed19592afad2/include/chrono#L1372

_LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX11 time_point() : __d_(duration::zero()) {}

timestamp class avoids it by calling no parameter constructor of duration.

simt::std::chrono::duration is used for duration types.
for timestamp, wait for #5400 .

cpp/include/cudf/scalar/scalar.hpp

- this builds on rapidsai#5359 and fixes or adds a few things - unifies the duration and timestamp scalars - adds `is_chrono` and relevant chrono traits - reworked the duration type so that we can support conversions between durations that can't be supported by the language(lower_precision_duration = higher_precision_duration) - include duration types in the typed tests - added tests for new unary casts with durations i have left a few TODOs which will be in the follow-up pr's

codecov · 2020-06-07T17:27:57Z

Codecov Report

Merging #5359 into branch-0.15 will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff              @@
##           branch-0.15    #5359   +/-   ##
============================================
  Coverage        88.68%   88.68%           
============================================
  Files               57       57           
  Lines            10780    10780           
============================================
  Hits              9560     9560           
  Misses            1220     1220

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 029005c...030927b. Read the comment docs.

- sync with upstream 0.15 and rapidsai#5359

jrhemstad · 2020-06-08T14:36:02Z

cpp/include/cudf/wrappers/durations.hpp

+/**
+ * @brief Type alias representing an int32_t duration of days.
+ **/
+using duration_D = simt::std::chrono::duration<int32_t, simt::std::chrono::days::period>;


Can we use these as the duration type of the timestamp types?

sriramch · 2020-06-08T14:41:41Z

cpp/include/cudf/wrappers/timestamps.hpp

@@ -45,6 +45,7 @@ struct timestamp : time_point<Duration> {
   *
   * @param other The `timestamp` to copy
   */
+  // TODO: This explict truncation is intended?


would this one and the constructor constexpr timestamp(Duration d) have to be explicitly defined? they are:

either relaxing or adding explicit specifiers that aren't in the chrono's time_point type

allows narrowing/truncation of types, when it is disallowed by std::chrono

is it not possible to inherit them from the base type?

Yeah the list of timestamp constructors evolved over time, and in response to various compiler errors (@karthikeyann reminds me of one I forgot in this comment). Some of them may not be valid reasons any longer.

I'd say change/remove whichever ones we want, and recompile to see what breaks (this can be frustrating, as changing this header triggers a near full re-compile).

devavret · 2020-06-08T14:45:43Z

Does this PR ensure that timestamp_s - timestamp_s == duration_s? Is that automatic or would this need overloading timestamp_s::operator-()

sriramch · 2020-06-08T14:58:21Z

Does this PR ensure that timestamp_s - timestamp_s == duration_s? Is that automatic or would this need overloading timestamp_s::operator-()

the support for these operators (add/sub etc.) with the timestamp/duration operands will be added as a part of a follow-up pr's focusing on unary/binary ops.

trxcllnt

Left a few comments, but approving as they're minor.

trxcllnt · 2020-06-08T17:38:17Z

cpp/tests/utilities/column_utilities.cu

+  template <typename Element, typename std::enable_if_t<is_duration<Element>()>* = nullptr>
+  void operator()(cudf::column_view const& col, std::vector<std::string>& out)
+  {
+    CUDF_FAIL("duration printing not supported yet");


Why not implement this?

Supposedly coming in a later PR where testing code will be updated to work with duration types.

trxcllnt · 2020-06-08T18:02:34Z

cpp/include/cudf/wrappers/timestamps.hpp

@@ -45,6 +45,7 @@ struct timestamp : time_point<Duration> {
   *
   * @param other The `timestamp` to copy
   */
+  // TODO: This explict truncation is intended?


Yeah the list of timestamp constructors evolved over time, and in response to various compiler errors (@karthikeyann reminds me of one I forgot in this comment). Some of them may not be valid reasons any longer.

I'd say change/remove whichever ones we want, and recompile to see what breaks (this can be frustrating, as changing this header triggers a near full re-compile).

devavret

LGTM

devavret · 2020-06-08T18:33:45Z

cpp/src/io/csv/csv_gpu.cu

@@ -351,6 +353,23 @@ __inline__ __device__ cudf::timestamp_ns decode_value(const char *data,
  return milli * 1000000;
 }

+// The purpose of this is merely to allow compilation ONLY
+// TODO : make this work for json


Is this a TODO to add parsing implementation to csv or is it a reminder to do the same thing for json?

devavret · 2020-06-08T18:35:13Z

cpp/tests/utilities/column_utilities.cu

+  template <typename Element, typename std::enable_if_t<is_duration<Element>()>* = nullptr>
+  void operator()(cudf::column_view const& col, std::vector<std::string>& out)
+  {
+    CUDF_FAIL("duration printing not supported yet");


Supposedly coming in a later PR where testing code will be updated to work with duration types.

karthikeyann added 15 commits June 2, 2020 16:36

add duration types (durations.hpp)

2d4b2ca

add duration traits

2ac83eb

add duration types to type_dispatcher

f2422b8

add duration types to type_id

9ee6a30

add duration_scalar

0e10471

add device view for duration_scalar

15fce99

aggregation SUM, PROD target_type_t for duration

51aeb6c

atomic operation on duration types

892f944

add decode_value placeholder for duration types in read_csv

afa7710

add decode_value placeholder for duration types in read_json

107a3dd

placeholder for duration type column_view_printer

d26cece

add make_duration_scalar

c905ab3

add make_duration_column factory

d19900b

add durations.hpp to libcudf/meta.yml

e21275c

removes unreachable code compiler warnings

d22fde1

karthikeyann requested a review from sriramch June 2, 2020 14:29

changelog entry for duration types rapidsai#5359

d4e9a88

jrhemstad requested changes Jun 2, 2020

View reviewed changes

jrhemstad reviewed Jun 2, 2020

View reviewed changes

remove years, months, weeks, hours, minutes duration types

98c4b87

sriramch reviewed Jun 2, 2020

View reviewed changes

sriramch mentioned this pull request Jun 4, 2020

[REVIEW] duration type unary casts and binary ops #5394

Merged

This was referenced Jun 4, 2020

[FEA] Add duration column types #5272

Closed

[REVIEW] disallow SUM and MEAN of timestamp types #5319

Merged

Merge sriramch:duration_types and review suggestions

0a5c754

jrhemstad mentioned this pull request Jun 5, 2020

[FEA] Use libcu++ from GitHub once it is available #5400

Closed

replace class duration with simt::std::chrono::duration types

37b1dcc

karthikeyann added 3 commits June 5, 2020 22:01

fix compilation issue, explicity type cast

185241e

using static member function of duration

cd0e7a0

replace unnecessary duration::rep with ::rep

3c015d8

karthikeyann marked this pull request as ready for review June 7, 2020 15:14

karthikeyann requested review from a team as code owners June 7, 2020 15:14

karthikeyann requested review from devavret and davidwendt June 7, 2020 15:14

Merge branch 'branch-0.15' into fea-duration_types

030927b

karthikeyann changed the title ~~[WIP] duration types~~ [REVIEW] duration types Jun 8, 2020

sriramch added a commit to sriramch/cudf that referenced this pull request Jun 8, 2020

- enable tests with duration types

ab108a7

- sync with upstream 0.15 and rapidsai#5359

jrhemstad reviewed Jun 8, 2020

View reviewed changes

jrhemstad approved these changes Jun 8, 2020

View reviewed changes

sriramch reviewed Jun 8, 2020

View reviewed changes

raydouglass approved these changes Jun 8, 2020

View reviewed changes

karthikeyann requested a review from trxcllnt June 8, 2020 17:30

trxcllnt approved these changes Jun 8, 2020

View reviewed changes

devavret approved these changes Jun 8, 2020

View reviewed changes

karthikeyann merged commit 3fd7b38 into rapidsai:branch-0.15 Jun 9, 2020

trevorsm7 mentioned this pull request Jun 25, 2020

[REVIEW] Add zero-copy logical cast #5358

Merged

Unit	Ratio
seconds	1
minutes	60
hours	3600
days	86400
weeks	604800
months	2629746
years	31556952

[REVIEW] duration types #5359

[REVIEW] duration types #5359

Conversation

karthikeyann commented Jun 2, 2020 • edited Loading

GPUtester commented Jun 2, 2020

jrhemstad Jun 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karthikeyann Jun 2, 2020 • edited Loading

Choose a reason for hiding this comment

jrhemstad Jun 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkraus14 commented Jun 2, 2020

Choose a reason for hiding this comment

karthikeyann Jun 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trxcllnt Jun 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 7, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devavret commented Jun 8, 2020

sriramch commented Jun 8, 2020

trxcllnt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devavret left a comment

Choose a reason for hiding this comment

devavret Jun 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karthikeyann commented Jun 2, 2020 •

edited

Loading

jrhemstad Jun 2, 2020 •

edited

Loading

karthikeyann Jun 2, 2020 •

edited

Loading

jrhemstad Jun 2, 2020 •

edited

Loading

karthikeyann Jun 2, 2020 •

edited

Loading

trxcllnt Jun 4, 2020 •

edited

Loading

codecov bot commented Jun 7, 2020 •

edited

Loading

devavret Jun 8, 2020 •

edited

Loading