[REVIEW] Support nullable timestamp columns in time range window functions #6557

mythrocks · 2020-10-19T22:35:04Z

Closes #6524.

Time-range based rolling window functions currently do not consider the possibility that the timestamp column might have nulls in them, thus muffing up the time-window calculation.

Note that grouped_time_range_rolling_window() requires that the timestamp column is sorted (ASC or DESC), and possibly grouped by other keys. The NULL values in the timestamp column will be clustered at the beginning or end of the column (if ungrouped), or beginning/end of each group.
E.g. The following is an example of a table grouped by the first column, ordered by ASCENDING timestamp (within each group), with NULLs appearing first.

GBY key	Timestamp key	Aggregation Value
0	NULL	0
0	NULL	1
0	2020-01-02	2
0	2020-01-03	3
0	2020-01-04	4
1	NULL	0
1	NULL	1
1	NULL	2
1	2020-01-03	3
1	2020-01-04	4
1	2020-01-05	5

In this case, when doing a rolling-window COUNT on the third column with 1 DAY preceding and 1 DAY following, the rows with NULL timestamps should not participate with those that have a valid timestamp. They will, however, participate in a window of their own. The output for rolling-window COUNT should be:

2, 2, 2, 3, 2, 3, 3, 3, 2, 3, 2

Note that the third row of the table must ignore the previous two rows, since their timestamps are NULL. The rolling-window for the null rows themselves must consider all rows in that group's null-range. This behaviour is congruent with ranged window operations in SQL systems (such as SparkSQL).

The commits included herewith provide support for nullable timestamp columns in time-range window functions.

1. Initial commit. Fix for non-grouped time-ranges with null timestamps, in ASC order. 2. Fixed non-grouped time-ranges with null timestampw, in DESC. 3. Fixed grouped time-ranges with null timestamp, in ASC. 4. Fixed grouped time-ranges with null timestamp, in DESC. 5. Fixed grouped time-ranges for ASC (following) 6. Tests for different combinations of timestamp null grouping, timestamp ordering, and key grouping. 7. Refactor: Common code gathered to utility function. 8. Code formatting

GPUtester · 2020-10-19T22:35:33Z

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

mythrocks · 2020-10-19T22:41:08Z

This change stomps all over the work in #6512. I realize that I'd be mixing concerns here, but would anyone be opposed to my including that refactor in this PR?
The refactor really only does a couple of simple things:

Move grouped_time_range*() and its helpers to a separate translation unit.
Remove support for timestamp_D columns. A timestamp_D input will be cast to a timestamp_S column, thus eliminating the int32_t template instantiations.
These changes should speed up the compile and reduce the object size.

vuule · 2020-10-19T22:43:38Z

This change stomps all over the work in #6512. I realize that I'd be mixing concerns here, but would anyone be opposed to my including that refactor in this PR?
The refactor really only does a couple of simple things:

Move grouped_time_range*() and its helpers to a separate translation unit.

Remove support for timestamp_D columns. A timestamp_D input will be cast to a timestamp_S column, thus eliminating the int32_t template instantiations.
These changes should speed up the compile and reduce the object size.

Is #6512 ready for review?

mythrocks · 2020-10-19T22:48:15Z

Is #6512 ready for review?

Hey, @vuule. It is, although I haven't benchmarked the runtime effect of casting timestamp_D -> timestamp_S.
It would be a little easier to redo those changes here (or after this PR) than to upmerge this PR after those changes have gone in. :]

Added to CHANGELOG.md.

…mestamp-branch-0.17

vuule · 2020-10-19T23:01:00Z

Hey, @vuule. It is, although I haven't benchmarked the runtime effect of casting timestamp_D -> timestamp_S.
It would be a little easier to redo those changes here (or after this PR) than to upmerge this PR after those changes have gone in. :]

Now I understand the question:) I think the diff would not work well if the code has been moved between files, so these two PRs combined would be really hard to review. I would prefer to keep them separate, mostly for this reason.

mythrocks · 2020-10-19T23:50:11Z

Thanks, @vuule. I'll make #6512 depend on #6557. I'll redo the #6512 change when this one goes in.

codecov · 2020-10-20T02:08:11Z

Codecov Report

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

@@             Coverage Diff              @@
##           branch-0.17    #6557   +/-   ##
============================================
  Coverage        82.54%   82.54%           
============================================
  Files               90       90           
  Lines            14928    14928           
============================================
  Hits             12322    12322           
  Misses            2606     2606

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 cbd2726...5d82e7c. Read the comment docs.

vuule

Partial review

cpp/src/rolling/rolling.cu

…mestamp-branch-0.17

1. Non-groupby case: Use null-count to calculate null bounds 2. Groupby case: Use partition_point()

mythrocks · 2020-10-22T17:14:03Z

Hmm. There are test failures that don't seem related to this change, at least on the face of it. From the test logs:

=========================== short test summary info ============================
FAILED custreamz/tests/test_dataframes.py::test_binary_stream_operators[dask]
FAILED custreamz/tests/test_dataframes.py::test_pair_arithmetic[dask] - Value...
FAILED custreamz/tests/test_dataframes.py::test_getitem[dask] - KeyError: 'gt...
FAILED custreamz/tests/test_dataframes.py::test_groupby_aggregate[dask-<lambda>0-<lambda>0-<lambda>0]
FAILED custreamz/tests/test_dataframes.py::test_groupby_aggregate[dask-<lambda>0-<lambda>0-<lambda>1]
FAILED custreamz/tests/test_dataframes.py::test_groupby_aggregate[dask-<lambda>0-<lambda>2-<lambda>0]
FAILED custreamz/tests/test_dataframes.py::test_groupby_aggregate[dask-<lambda>0-<lambda>2-<lambda>1]
FAILED custreamz/tests/test_dataframes.py::test_groupby_aggregate[dask-<lambda>1-<lambda>0-<lambda>0]
FAILED custreamz/tests/test_dataframes.py::test_groupby_aggregate[dask-<lambda>1-<lambda>0-<lambda>1]
FAILED custreamz/tests/test_dataframes.py::test_groupby_aggregate[dask-<lambda>1-<lambda>2-<lambda>0]
FAILED custreamz/tests/test_dataframes.py::test_groupby_aggregate[dask-<lambda>1-<lambda>2-<lambda>1]
FAILED custreamz/tests/test_dataframes.py::test_groupby_aggregate[dask-<lambda>2-<lambda>0-<lambda>0]
FAILED custreamz/tests/test_dataframes.py::test_groupby_aggregate[dask-<lambda>2-<lambda>0-<lambda>1]
FAILED custreamz/tests/test_dataframes.py::test_groupby_aggregate[dask-<lambda>2-<lambda>2-<lambda>0]
FAILED custreamz/tests/test_dataframes.py::test_groupby_aggregate[dask-<lambda>2-<lambda>2-<lambda>1]
FAILED custreamz/tests/test_dataframes.py::test_setitem[dask] - AttributeErro...
FAILED custreamz/tests/test_dataframes.py::test_setitem_overwrites[dask] - Va...
FAILED custreamz/tests/test_dataframes.py::test_integration_from_stream[dask]
= 18 failed, 582 passed, 4 skipped, 240 xfailed, 1 xpassed, 2 warnings in 275.40s (0:04:35) =
+++ EXITCODE=1
++ gpuci_logger 'Test notebooks'

vuule · 2020-10-22T17:15:40Z

Hmm. There are test failures that don't seem related to this change, at least on the face of it.

Yeah, this is a known CI issue. Started happening two days ago.

mythrocks · 2020-10-22T17:21:59Z

Thanks, @vuule. Phew.

As an aside, I think I've incorporated your suggestions in the current version of the code. Please let me know if this is more palatable.

vuule

Minor suggestions, requesting changes primarily for of test coverage.

cpp/src/rolling/rolling.cu

cpp/tests/grouped_rolling/grouped_rolling_test.cpp

1. Short circuit eval for all-null/no-null cases. 2. Test cases for all-null/no-null. 3. Code formatting

…mestamp-branch-0.17

harrism · 2020-10-26T03:57:24Z

rerun tests

mythrocks · 2020-10-26T17:32:31Z

@vuule, thank you for your review and advice. Hey, @davidwendt. I was wondering if you might take a gander at this change. Thanks.

cpp/src/rolling/rolling.cu

1. Fixed angle-brackets for header inclusion. 2. Switched to doxygen-style function documentation

…mestamp-branch-0.17

3. Code formatting

vuule

Looks good. Got an optional suggestion.

cpp/src/rolling/rolling.cu

1. Streamline null_bound calculation, with fewer comparisons.

2. Fix botched null_bound calculation cleanup. 3. Remove errant println in tests.

mythrocks · 2020-10-27T04:09:16Z

Thanks for the reviews, all. I have just merged this now.

mythrocks requested a review from a team as a code owner October 19, 2020 22:35

mythrocks requested review from vuule and davidwendt October 19, 2020 22:35

mythrocks self-assigned this Oct 19, 2020

mythrocks added the 2 - In Progress Currently a work in progress label Oct 19, 2020

mythrocks changed the title ~~Fix time range window queries with null timestamps~~ Support nullable timestamp columns in time range window functions Oct 19, 2020

mythrocks added 2 commits October 19, 2020 15:50

Fix time range window queries with null timestamps

72f20f3

Added to CHANGELOG.md.

Merge remote-tracking branch 'origin/branch-0.17' into null-checks-ti…

07ab590

…mestamp-branch-0.17

mythrocks added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Oct 20, 2020

mythrocks changed the title ~~Support nullable timestamp columns in time range window functions~~ [REVIEW] Support nullable timestamp columns in time range window functions Oct 20, 2020

vuule requested changes Oct 20, 2020

View reviewed changes

cpp/src/rolling/rolling.cu Outdated Show resolved Hide resolved

cpp/src/rolling/rolling.cu Outdated Show resolved Hide resolved

mythrocks added 2 commits October 21, 2020 20:17

Merge remote-tracking branch 'origin/branch-0.17' into null-checks-ti…

261454f

…mestamp-branch-0.17

Fix time range window queries with null timestamps

25aac84

1. Non-groupby case: Use null-count to calculate null bounds 2. Groupby case: Use partition_point()

vuule requested changes Oct 22, 2020

View reviewed changes

cpp/src/rolling/rolling.cu Outdated Show resolved Hide resolved

cpp/src/rolling/rolling.cu Outdated Show resolved Hide resolved

cpp/tests/grouped_rolling/grouped_rolling_test.cpp Show resolved Hide resolved

mythrocks added 2 commits October 23, 2020 12:59

Fix time range window queries with null timestamps

7f5a235

1. Short circuit eval for all-null/no-null cases. 2. Test cases for all-null/no-null. 3. Code formatting

Merge remote-tracking branch 'origin/branch-0.17' into null-checks-ti…

5aeeeb1

…mestamp-branch-0.17

Merge remote-tracking branch 'origin/branch-0.17' into null-checks-ti…

58e3426

…mestamp-branch-0.17

davidwendt reviewed Oct 26, 2020

View reviewed changes

cpp/src/rolling/rolling.cu Outdated Show resolved Hide resolved

davidwendt reviewed Oct 26, 2020

View reviewed changes

cpp/src/rolling/rolling.cu Outdated Show resolved Hide resolved

davidwendt approved these changes Oct 26, 2020

View reviewed changes

cpp/src/rolling/rolling.cu Outdated Show resolved Hide resolved

mythrocks added 3 commits October 26, 2020 11:31

Fix time range window queries with null timestamps

d93fb8f

1. Fixed angle-brackets for header inclusion. 2. Switched to doxygen-style function documentation

Merge remote-tracking branch 'origin/branch-0.17' into null-checks-ti…

7415241

…mestamp-branch-0.17

Fix time range window queries with null timestamps

f8b3ec5

3. Code formatting

vuule approved these changes Oct 26, 2020

View reviewed changes

cpp/src/rolling/rolling.cu Outdated Show resolved Hide resolved

mythrocks added 2 commits October 26, 2020 14:22

Fix time range window queries with null timestamps

9b97c78

1. Streamline null_bound calculation, with fewer comparisons.

Fix time range window queries with null timestamps

5d82e7c

2. Fix botched null_bound calculation cleanup. 3. Remove errant println in tests.

mythrocks merged commit 350cd16 into rapidsai:branch-0.17 Oct 27, 2020

mythrocks deleted the null-checks-timestamp-branch-0.17 branch October 27, 2020 04:56

mythrocks mentioned this pull request Oct 27, 2020

Refactor rolling.cu to reduce compile time #6512

Merged

mythrocks mentioned this pull request Nov 19, 2020

First class support for unbounded window function bounds #6811

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[REVIEW] Support nullable timestamp columns in time range window functions #6557

[REVIEW] Support nullable timestamp columns in time range window functions #6557

mythrocks commented Oct 19, 2020

GPUtester commented Oct 19, 2020

mythrocks commented Oct 19, 2020

vuule commented Oct 19, 2020

mythrocks commented Oct 19, 2020

vuule commented Oct 19, 2020

mythrocks commented Oct 19, 2020

codecov bot commented Oct 20, 2020 •

edited

Loading

vuule left a comment

mythrocks commented Oct 22, 2020

vuule commented Oct 22, 2020

mythrocks commented Oct 22, 2020

vuule left a comment

harrism commented Oct 26, 2020

mythrocks commented Oct 26, 2020

vuule left a comment

mythrocks commented Oct 27, 2020

[REVIEW] Support nullable timestamp columns in time range window functions #6557

[REVIEW] Support nullable timestamp columns in time range window functions #6557

Conversation

mythrocks commented Oct 19, 2020

GPUtester commented Oct 19, 2020

mythrocks commented Oct 19, 2020

vuule commented Oct 19, 2020

mythrocks commented Oct 19, 2020

vuule commented Oct 19, 2020

mythrocks commented Oct 19, 2020

codecov bot commented Oct 20, 2020 • edited Loading

Codecov Report

vuule left a comment

Choose a reason for hiding this comment

mythrocks commented Oct 22, 2020

vuule commented Oct 22, 2020

mythrocks commented Oct 22, 2020

vuule left a comment

Choose a reason for hiding this comment

harrism commented Oct 26, 2020

mythrocks commented Oct 26, 2020

vuule left a comment

Choose a reason for hiding this comment

mythrocks commented Oct 27, 2020

codecov bot commented Oct 20, 2020 •

edited

Loading