Add JNI methods for detecting and purging non-empty nulls from LIST and STRUCT #12742

razajafri · 2023-02-09T04:47:41Z

Description

This PR adds methods for detecting and purging non-empty nulls.

Checklist

I am familiar with the Contributing Guidelines.
New or existing tests cover these changes.
The documentation is up to date with these changes.

…mpty

ttnghia

What is the use case for this? We are working on the cudf side, trying to totally eliminate the non-empty null issue.

razajafri · 2023-02-09T16:52:35Z

What is the use case for this? We are working on the cudf side, trying to totally eliminate the non-empty null issue.

This is needed because we need to support HashAggregate[List] which cannot be done unless we support sorting with non-empty nulls, so as a work around cudf has these functions which we can use in the interim. For reference checkout NVIDIA/spark-rapids#6679, NVIDIA/spark-rapids#5430 and perhaps follow the trail from NVIDIA/spark-rapids#6680.

How far is the team in figuring out the solution for this and when is the fix in cudf expected to go in? An alternative, and my preference, would be to let this go in for now and revert it when cudf is ready.

ttnghia · 2023-02-09T17:52:00Z

I see the issue with ColumnView. Ideally, this (purge non-empty nulls) should only be called in cudf or JNI C++, not in Java. Unfortunately in Java we also have ColumnView constructor (and similar) that allow non-empty nulls to exist.

In addition, the PR description doesn't have any context/meaningful description. Please update it.

java/src/main/java/ai/rapids/cudf/ColumnView.java

java/src/main/native/src/ColumnViewJni.cpp

razajafri · 2023-02-09T18:56:08Z

I see the issue with ColumnView. Ideally, this (purge non-empty nulls) should only be called in cudf or JNI C++, not in Java. Unfortunately in Java we also have ColumnView constructor (and similar) that allow non-empty nulls to exist.

In addition, the PR description doesn't have any context/meaningful description. Please update it.

ColumnView in Java just calls into the cpp code, if cpp guarantees there will be no non-empty nulls then we won't need to call this from Java. As you said if we are working on a proper fix this wouldn't be needed hopefully.

…mpty

razajafri · 2023-02-10T21:30:20Z

I am having difficulty running pre-commit hooks and thus having to push multiple updates due to formatting errors

ttnghia · 2023-02-10T21:37:57Z

I am having difficulty running pre-commit hooks and thus having to push multiple updates due to formatting errors

You should run clang-format by yourself, not pre-commit hooks.

…mpty

razajafri · 2023-02-15T06:03:25Z

/test

razajafri · 2023-02-15T06:04:37Z

re-run tests

ttnghia · 2023-02-17T21:22:21Z

CI Java failed due to:

Error:  Tests run: 311, Failures: 0, Errors: 1, Skipped: 2, Time elapsed: 8.111 s <<< FAILURE! - in ai.rapids.cudf.ColumnVectorTest
Error:  ai.rapids.cudf.ColumnVectorTest  Time elapsed: 6.67 s  <<< ERROR!
ai.rapids.cudf.RmmException: Could not shut down RMM there appear to be outstanding allocations

razajafri · 2023-02-17T22:33:55Z

CI Java failed due to:

Error:  Tests run: 311, Failures: 0, Errors: 1, Skipped: 2, Time elapsed: 8.111 s <<< FAILURE! - in ai.rapids.cudf.ColumnVectorTest
Error:  ai.rapids.cudf.ColumnVectorTest  Time elapsed: 6.67 s  <<< ERROR!
ai.rapids.cudf.RmmException: Could not shut down RMM there appear to be outstanding allocations

Yes, I can see that thanks. The reason why I was confused about the CI failure was that the tests are passing locally for me, so I was wondering if the failure was because of something going haywire in the CI because the "update branch" was enabled.

razajafri · 2023-02-24T06:42:46Z

The CI job is failing but when I try to run the exact CI jobs using @ajschmidt8 help and by following steps listed here. The tests pass for me locally and inside rapidsai ci docker.

We are still looking into the cause of this

ttnghia · 2023-02-24T14:29:46Z

java/src/test/java/ai/rapids/cudf/ColumnVectorTest.java

+    List<Integer> list1 = Arrays.asList(4, 5, null);
+    List<Integer> list2 = Arrays.asList(7, 8, 9);
+    List<Integer> list3 = null;
+    ColumnVector input =  makeListsColumn(DType.INT32, list0, list1, list2, list3);


CI failed because there is memory leak in Java tests. Sorry I overlooked the Java tests. Please fix them.

ColumnVector as well as HostColumnVector must be wrapped in try blocks. Probably the buffer vars need to be wrapped too.

Hmm... makes me want to have this failure on my local so we can avoid this. It will still be good to get to the bottom of this discrepancy.

In the meanwhile, I will push a fix. Thanks for pointing us to this.

Agreed, the discrepancy isn't ideal.

Could it be caused by the types of GPUs that are used in CI vs. on our local machines?

I'm wondering if that or the amount of memory on a given GPU would contribute to this.

Maybe there is a variable that controls the test behavior, and that var is set by the local environment. @revans2 Do you know about this?

If a memory leak happens and java's GC is fast, then the leak is cleared up before RMM is shut down and there is no error. If GC is slow, then the leak is not cleared and you get the error. We have a flag to print out messages to help debug leaks, but it is off by default because it makes the tests take a lot longer. Just set -Dai.rapids.refcount.debug=true on the maven commend line when running the tests. Even if they pass you can then look at the logs to see if any leaks were detected and use the stack traces to track down what is leaked and why.

But in this case aren't the vectors off the heap? Did you mean the MemoryCleaner instead of GC?

java/src/test/java/ai/rapids/cudf/ColumnVectorTest.java

ttnghia · 2023-02-24T18:21:25Z

java/src/test/java/ai/rapids/cudf/ColumnVectorTest.java

@@ -6693,6 +6693,9 @@ void testApplyBooleanMaskFromListOfStructure() {
    }
  }

+  /**
+   * The caller needs to make sure to close the returned ColumnView


Only the returned ColumnView is not enough. There are more local variables/columns need to be closed.

Suggested change

* The caller needs to make sure to close the returned ColumnView

* The caller needs to make sure to call this function in a `try` block as local variables need to be closed.

ugh, I am rushing through this and context-switching isn't helping either. I think it will make sense for me to close all the local allocations and leave only the returned vector to be closed by the caller.

ttnghia · 2023-02-25T01:38:39Z

java/src/test/java/ai/rapids/cudf/ColumnVectorTest.java

+      ColumnView colWithNonEmptyNulls = new ColumnView(input.type, input.rows, Optional.of(2L), dmb,
+          input.getDeviceBufferFor(BufferType.OFFSET), input.getChildColumnViews());
+      assertEquals(2, colWithNonEmptyNulls.nullCount);
+      return colWithNonEmptyNulls;


I don't think this returned vector should be in this try block.

That's not the problem, the problem is that the input column will be closed while the view won't. Unfortunately my build env is completely ruined and I will have to spend time to fix it before I can proceed. Thanks for your quick reviews

razajafri · 2023-02-27T04:29:04Z

/merge

Added Jni methods for detecting and purging non-empty nulls

0f72ac7

razajafri added 3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Feb 9, 2023

razajafri self-assigned this Feb 9, 2023

razajafri requested a review from a team as a code owner February 9, 2023 04:47

github-actions bot added the Java Affects Java cuDF API. label Feb 9, 2023

Merge remote-tracking branch 'origin/branch-23.04' into SP-5430-non-e…

2b29f3b

…mpty

ttnghia requested changes Feb 9, 2023

View reviewed changes