Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add unit tests for lead/lag on list for row window [skip ci] #8259

Merged
merged 2 commits into from
May 19, 2021

Conversation

wbo4958
Copy link
Contributor

@wbo4958 wbo4958 commented May 17, 2021

This PR just added the unit tests for the lead/lag on List/Struct type for both Row-window and Range-window, Since libcudf has already supported the functionality. See the plugin feature request NVIDIA/spark-rapids#2012

But there is an issue when setting the default values for lead/lag on List/Struct. I just commented the corresponding assertation out. I will re-enable it after libcudf fixes it.

@github-actions github-actions bot added the Java Affects Java cuDF API. label May 17, 2021
@wbo4958 wbo4958 changed the title add unit tests for lead/lag on list for row window add unit tests for lead/lag on list for row window [skip ci] May 17, 2021
@firestarman firestarman added feature request New feature or request non-breaking Non-breaking change labels May 17, 2021
@wbo4958 wbo4958 marked this pull request as ready for review May 17, 2021 06:14
@wbo4958 wbo4958 requested a review from a team as a code owner May 17, 2021 06:14
@wbo4958 wbo4958 requested review from mythrocks and jlowe May 17, 2021 06:20
@sperlingxx
Copy link
Contributor

LGTM

@firestarman
Copy link
Contributor

LGTM, but some tests have failed.

@wbo4958 wbo4958 changed the title add unit tests for lead/lag on list for row window [skip ci] add unit tests for lead/lag on list for row window [skip ci] May 17, 2021
@@ -3464,6 +3681,16 @@ void testRangeWindowingLead() {
.column(1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1) // GBY Key
.column(0, 0, 0, 0, 1, 1, 1, 1, 2, 2, 2, 2, 2) // GBY Key
.column(7, 5, 1, 9, 7, 9, 8, 2, 8, 0, 6, 6, 8) // Agg Column
.column(new ListType(false, new BasicType(true, DType.INT32)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a review suggestion. Just FYI: I think SparkSQL doesn't support calling LEAD()/LAG() on RANGE windows. It might be worth checking. This path might simply not get exercised from the spark-rapids plugin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the lead/lag for range window test

Copy link
Contributor

@mythrocks mythrocks left a comment

Choose a reason for hiding this comment

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

Some minor changes requested. The test results look good to my eye.

@wbo4958
Copy link
Contributor Author

wbo4958 commented May 19, 2021

@mythrocks Could you help to check it

@mythrocks
Copy link
Contributor

@gpucibot merge

@rapids-bot rapids-bot bot merged commit b0dc972 into rapidsai:branch-21.06 May 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request Java Affects Java cuDF API. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants