Java support for casting of nested child columns [skip ci] #7417

razajafri · 2021-02-22T01:12:53Z

This PR adds a couple of very specialized methods that help us cast columns inside nested types.

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

java/src/main/native/src/ColumnVectorJni.cpp

razajafri · 2021-03-01T22:51:52Z

@revans2 I have addressed your concerns, PTAL

revans2 · 2021-03-01T22:58:54Z

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

+   */
+  public ColumnView replaceChildrenWithViews(int[] indices,
+                                             ColumnView[] views) {
+    assert(type == DType.STRUCT || type == DType.LIST);


There is a isNestedType in dtype that I think is more appropriate. We might also want to look at supporting this for strings at some point.

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

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

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

java/src/main/native/src/ColumnVectorJni.cpp

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

mythrocks · 2021-03-02T23:09:41Z

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

+
+    JNI_ARG_CHECK(env, m.empty(), "One or more invalid child indices passed to be replaced", 0);
+
+    std::unique_ptr<cudf::column_view> n_new_nested(new cudf::column_view(n_col_view->type(), n_col_view->size(), 


A general question, if you'll indulge me:
This idiom is used in several places in this file. Why do we allocate a unique_ptr and immediately turn around and release() it? Why not simply return the following?

return reinterpret_cast<jlong>( new cudf::column_view(n_col_view->type(), n_col_view->size(), nullptr, n_col_view->null_mask(), n_col_view->null_count(), n_col_view->offset(), children));

I simply did it to keep up with the conventions of our fathers and their fathers before them :D

I can change that if you like

I have made the change as it makes no sense to do that

mythrocks · 2021-03-02T23:17:57Z

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

+      m[indices[i]] = children_to_replace[i];
+    }
+
+    std::vector<cudf::column_view> children;


Nitpick: new_children?

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

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

jlowe · 2021-03-02T23:40:35Z

I'm not thrilled with the concept of adding nested-type specific APIs that seem very specific to a casting scenario. I think we could let callers do these "cast within nesting" and other, future view shenanigans by exposing a couple of lower-level APIs that lets users build custom ColumnView instances, e.g.:

  public ColumnView(DType type, long rows, Optional<Long> nullCount,
                      DeviceMemoryBuffer dataBuffer, DeviceMemoryBuffer validityBuffer,
                      DeviceMemoryBuffer offsetBuffer) {
...
}

  public ColumnView(DType type, long rows, Optional<Long> nullCount,
                      DeviceMemoryBuffer validityBuffer, DeviceMemoryBuffer offsetBuffer,
                      ColumnView[] childViews) {
...
}

Then for these casting cases, the caller can grab the validity buffer (and offset buffer if its a list) and use those to create a view with newly constructed child column views. This also lets us create views of non-nested types with a swapped-out validity buffer or data buffer if that becomes useful for some future case.

Thoughts?

razajafri · 2021-03-03T00:46:20Z

I'm not thrilled with the concept of adding nested-type specific APIs that seem very specific to a casting scenario. I think we could let callers do these "cast within nesting" and other, future view shenanigans by exposing a couple of lower-level APIs that lets users build custom ColumnView instances, e.g.:
  public ColumnView(DType type, long rows, Optional<Long> nullCount,
                      DeviceMemoryBuffer dataBuffer, DeviceMemoryBuffer validityBuffer,
                      DeviceMemoryBuffer offsetBuffer) {
...
}

  public ColumnView(DType type, long rows, Optional<Long> nullCount,
                      DeviceMemoryBuffer validityBuffer, DeviceMemoryBuffer offsetBuffer,
                      ColumnView[] childViews) {
...
}
Then for these casting cases, the caller can grab the validity buffer (and offset buffer if its a list) and use those to create a view with newly constructed child column views. This also lets us create views of non-nested types with a swapped-out validity buffer or data buffer if that becomes useful for some future case.

Thoughts?

I see the benefit of having this API. But in this case it won't help unless we make changes to our structure. I don't think it can happen with the way things are currently structured as this would mean changing the ColumnVector.getValidity to return a DeviceMemoryBuffer instead of DeviceMemoryBufferView.

Can I also bring up why we have the ColumnView as the parent of ColumnVector?

Co-authored-by: MithunR <[email protected]>

jlowe · 2021-03-03T14:27:56Z

I don't think it can happen with the way things are currently structured as this would mean changing the ColumnVector.getValidity to return a DeviceMemoryBuffer instead of DeviceMemoryBufferView.

Then use BaseDeviceMemoryBuffer in the new API so it can take buffer views or full buffers. The column view owns no memory, so BaseDeviceMemoryBuffer is more appropriate here.

Can I also bring up why we have the ColumnView as the parent of ColumnVector?

Every vector can act as a view, and we didn't want to create a delegate method in the vector to every view method.

revans2 · 2021-03-03T14:47:02Z

I like the idea of exposing a simple constructor for ColumnView instead. It is more flexible too, so we could do things like append column view to a struct or insert them instead of just replacing them. It would also have allowed us to do the equivalent of bit_cast ourselves.

My main concern with doing it right now is strings and lists. We want to eventually unify how they appear under the hood and if we expose APIs then it is going to be harder to cheange that without making breaking changes. I would prefer to have us start out with some factory methods for struct and list, but it is minor.

razajafri · 2021-03-03T17:08:05Z

I like the idea of exposing a simple constructor for ColumnView instead. It is more flexible too, so we could do things like append column view to a struct or insert them instead of just replacing them. It would also have allowed us to do the equivalent of bit_cast ourselves.

My main concern with doing it right now is strings and lists. We want to eventually unify how they appear under the hood and if we expose APIs then it is going to be harder to cheange that without making breaking changes. I would prefer to have us start out with some factory methods for struct and list, but it is minor.

Whats the verdict on this then? Should I go ahead and make the change that @jlowe recommends now or do we wait until we have unified the lists and strings?

Also, can you please explain what you mean by unifying the Strings and Lists? I thought they are already very similar, i.e. a string col is a List<char> col.

revans2 · 2021-03-03T19:25:55Z

Should I go ahead and make the change that @jlowe recommends now or do we wait until we have unified the lists and strings?

The only ColumnView constructor we have right now takes just an address, but the ColumnVector APIs expose all of the gory details so go ahead and just do what Jason suggested because we are going to have to modify ColumnVector anyways.

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

razajafri · 2021-03-05T20:40:55Z

@jlowe have I addressed all your concerns?

jlowe

Minor suggestions on comments and access scoping, but otherwise OK.

jlowe · 2021-03-06T05:21:22Z

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

@@ -49,6 +48,57 @@ protected ColumnView(long address) {
    this.nullCount = ColumnView.getNativeNullCount(viewHandle);
  }

+  /**
+   * Create a new column view based off of data already on the device. Ref count on the buffers
+   * is not incremented and none of the underlying buffers are owned by this view. If ownership


I think the comment needs to emphasize that the resulting instance is only valid as long as the underlying buffers remain valid. If someone closes those buffers while this view instance is still active and it is later accessed, the program as undefined behavior.

jlowe · 2021-03-06T05:21:33Z

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

-  private static long initViewHandle(DType type, int rows, int nc, DeviceMemoryBuffer dataBuffer,
-                                     DeviceMemoryBuffer validityBuffer,
-                                     DeviceMemoryBuffer offsetBuffer, long[] childHandles) {
+  protected static long initViewHandle(DType type, int rows, int nc,


This should be package-private

jlowe · 2021-03-06T05:22:08Z

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

+  }
+
+  /**
+   * Create a new column view based off of data already on the device. Ref count on the buffers


Similar comment on emphasizing lifetime requirements on input parameters relative to this instance.

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

mythrocks

Ah, you're no longer doing this with Java_ai_rapids_cudf_ColumnView_replaceChildrenWithViews.

LGTM.

razajafri · 2021-03-08T17:22:18Z

@gpucibot merge

@razajafri

This PR adds a couple of very specialized methods that help us cast columns inside nested types. Authors: - Raza Jafri (@razajafri) Approvers: - Robert (Bobby) Evans (@revans2) - Jason Lowe (@jlowe) - MithunR (@mythrocks) URL: rapidsai#7417

razajafri added 3 commits February 21, 2021 17:04

cast child columns for structs and lists

ceeddd2

don't copy column

78e633a

replace cols in list and struct

895433c

razajafri added 3 - Ready for Review Ready for review by team 4 - Needs cuDF (Java) Reviewer non-breaking Non-breaking change labels Feb 22, 2021

razajafri requested a review from a team as a code owner February 22, 2021 01:12

github-actions bot added the Java Affects Java cuDF API. label Feb 22, 2021

razajafri added the improvement Improvement / enhancement to an existing function label Feb 22, 2021

razajafri mentioned this pull request Feb 22, 2021

Decimal32 support NVIDIA/spark-rapids#1717

Merged

razajafri commented Feb 22, 2021

View reviewed changes

java/src/main/java/ai/rapids/cudf/ColumnVector.java Outdated Show resolved Hide resolved

revans2 reviewed Feb 22, 2021

View reviewed changes

java/src/main/java/ai/rapids/cudf/ColumnVector.java Outdated Show resolved Hide resolved

java/src/main/native/src/ColumnVectorJni.cpp Show resolved Hide resolved

razajafri added 4 commits February 26, 2021 16:38

updated method names

160d06c

addressed review comments

c82534f

code cleanup

2836e86

reformatted code

c8a35c6

revans2 reviewed Mar 1, 2021

View reviewed changes

addressed review comments

4be9b22

revans2 approved these changes Mar 2, 2021

View reviewed changes

java/src/main/java/ai/rapids/cudf/ColumnView.java Show resolved Hide resolved

razajafri added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team 4 - Needs cuDF (Java) Reviewer labels Mar 2, 2021

updated method doc

5e79b21

mythrocks requested changes Mar 2, 2021

View reviewed changes

jlowe changed the title ~~Java bindings for Decimal32 support [skip ci]~~ Java support for casting of nested child columns [skip ci] Mar 2, 2021

razajafri and others added 2 commits March 2, 2021 16:50

addressed review comments

0518c6e

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

1e17bdf

Co-authored-by: MithunR <[email protected]>

razajafri and others added 3 commits March 2, 2021 16:52

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

84d463c

Co-authored-by: MithunR <[email protected]>

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

beb05a4

Co-authored-by: MithunR <[email protected]>

fixed the return

6ed07cb

razajafri added 4 commits March 4, 2021 10:32

d32 changes

6abc629

Merge remote-tracking branch 'upstream/branch-0.19' into decimal32

2555030

Merge branch 'branch-0.19' into decimal32

ffd6b69

code cleanup

2937b31

jlowe reviewed Mar 4, 2021

View reviewed changes

jlowe approved these changes Mar 6, 2021

View reviewed changes

addressed review comments

ee866b2

mythrocks approved these changes Mar 8, 2021

View reviewed changes

rapids-bot bot merged commit 9017f22 into rapidsai:branch-0.19 Mar 8, 2021

razajafri deleted the decimal32 branch March 8, 2021 17:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Java support for casting of nested child columns [skip ci] #7417

Java support for casting of nested child columns [skip ci] #7417

razajafri commented Feb 22, 2021 •

edited

Loading

razajafri commented Mar 1, 2021

revans2 Mar 1, 2021

mythrocks Mar 2, 2021

razajafri Mar 2, 2021

razajafri Mar 3, 2021

mythrocks Mar 2, 2021

jlowe commented Mar 2, 2021

razajafri commented Mar 3, 2021

jlowe commented Mar 3, 2021

revans2 commented Mar 3, 2021

razajafri commented Mar 3, 2021 •

edited

Loading

revans2 commented Mar 3, 2021

razajafri commented Mar 5, 2021

jlowe left a comment

jlowe Mar 6, 2021

jlowe Mar 6, 2021

jlowe Mar 6, 2021

mythrocks left a comment

razajafri commented Mar 8, 2021


		JNI_ARG_CHECK(env, m.empty(), "One or more invalid child indices passed to be replaced", 0);

		std::unique_ptr<cudf::column_view> n_new_nested(new cudf::column_view(n_col_view->type(), n_col_view->size(),

Java support for casting of nested child columns [skip ci] #7417

Java support for casting of nested child columns [skip ci] #7417

Conversation

razajafri commented Feb 22, 2021 • edited Loading

razajafri commented Mar 1, 2021

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

jlowe commented Mar 2, 2021

razajafri commented Mar 3, 2021

jlowe commented Mar 3, 2021

revans2 commented Mar 3, 2021

razajafri commented Mar 3, 2021 • edited Loading

revans2 commented Mar 3, 2021

razajafri commented Mar 5, 2021

jlowe 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

mythrocks left a comment

Choose a reason for hiding this comment

razajafri commented Mar 8, 2021

razajafri commented Feb 22, 2021 •

edited

Loading

razajafri commented Mar 3, 2021 •

edited

Loading