[REVIEW] Initial work for decimal type in Java/JNI [skip ci] #6514

nartal1 · 2020-10-13T08:35:40Z

This partially addresses tasks from issue #6515.
The goal of this PR is to add DType class and make the necessary changes in Java/JNI code to support scale in DType.

GPUtester · 2020-10-13T08:36:15Z

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

revans2

I know this is still WIP, but I wanted to call out a few things.

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

…type_class

sperlingxx · 2020-10-14T02:57:06Z

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

@@ -38,10 +38,12 @@

  protected final OffHeapState offHeap;
  protected final DType type;


Should we contain DataType instead of DType + scale in here? And we need implement getDecimal. I think we can change it in subsequent PR.

sperlingxx · 2020-10-14T04:01:50Z

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

@@ -3729,6 +3767,14 @@ public static ColumnVector fromDoubles(double... values) {
    return build(DType.FLOAT64, values.length, (b) -> b.appendArray(values));
  }

+ /* public static ColumnVector fromDecimal32(int... values) {


involve this commented codes later till we are fully ready?

Signed-off-by: Niranjan Artal <[email protected]>

nartal1 · 2020-10-14T08:26:35Z

I think the code in Builder and ColumnBuilder also needs to be changed from DType to DataType.

revans2 · 2020-10-14T13:13:04Z

I think the code in Builder and ColumnBuilder also needs to be changed from DType to DataType.

If it is simple to do yes, but if it is not simple we should look at only supporting the newer one that supports nested types and deprecating the older one. No point in maintaing two APIs that do the same thing.

nartal1 · 2020-10-14T22:38:51Z

@revans2 @jlowe It would be great if you could take a look and let me know if this is something in the right direction. As of now I am making changes and making sure we don't break any current tests. The refactor is in different places so would be helpful if I could get early feedback on whether to continue or if we need to pivot.

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

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

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

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

…type_class

razajafri · 2020-10-15T06:27:41Z

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

@@ -3088,7 +3114,7 @@ public long getColumnViewAddress() {

  @Override
  public ColumnViewAccess getChildColumnViewAccess(int childIndex) {
-    if (!type.isNestedType()) {
+    if (!type.typeId.isNestedType()) {


I would parrot @jlowe 's comment here to add the isNestedType to DataType to reduce code changes

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

razajafri · 2020-10-15T06:37:47Z

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

@@ -1239,48 +1281,48 @@ public Builder appendUTF8String(byte[] value, int offset, int length) {

    public Builder appendArray(byte... values) {
      assert (values.length + currentIndex) <= rows;
-      assert type == DType.INT8 || type == DType.UINT8 || type == DType.BOOL8;
-      data.setBytes(currentIndex * type.sizeInBytes, values, 0, values.length);
+      assert type.typeId == DType.INT8 || type.typeId == DType.UINT8 || type.typeId == DType.BOOL8;


Now that we are making a change this would be a good time to clean this up and have a helper method in DataType like the one we have e.g. 'isTimestamp` etc

razajafri · 2020-10-15T06:38:16Z

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

      currentIndex += values.length;
      return this;
    }

    public Builder appendArray(short... values) {
-      assert type == DType.INT16 || type == DType.UINT16;
+      assert type.typeId == DType.INT16 || type.typeId == DType.UINT16;


same comment here. can we have a helper?

razajafri · 2020-10-15T06:39:51Z

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

  }

  /**
   * Get the value at index.
   */
  public final short getShort(long index) {
-    assert type == DType.INT16 || type == DType.UINT16 : type + " is not stored as a short.";
+    assert type.typeId == DType.INT16 || type.typeId == DType.UINT16 : type + " is not stored as a short.";


same comment here. have a helper

razajafri · 2020-10-15T06:40:04Z

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

@@ -273,28 +286,28 @@ private void assertsForGet(long index) {
   * Get the value at index.
   */
  public byte getByte(long index) {
-    assert type == DType.INT8 || type == DType.UINT8 || type == DType.BOOL8 : type +
+    assert type.typeId == DType.INT8 || type.typeId == DType.UINT8 || type.typeId == DType.BOOL8 : type +


same comment here. have a helper

razajafri · 2020-10-15T06:42:45Z

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

+  try {
+    cudf::jni::auto_set_device(env);
+    cudf::column_view *column = reinterpret_cast<cudf::column_view *>(handle);
+    return static_cast<jint>(column->type().scale());


is type checking needed before calling scale() on type_id? will it blow up in case its not a decimal type?

We should not need the cast and I would prefer to not have it because it shows that we don't have issues mapping from the native scale to the JNI representation.

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

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

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

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

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

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

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

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

revans2 · 2020-10-15T15:01:45Z

I had an idea and I really would like some feedback on it. I think we can make a change that is mostly source compatible, but not binary compatible.

What if we renamed DType to something like DTypeEnum and made it an inner class of DataType, which we then rename DataType to DType. Inside that new DType we add in APIs that match the current DType (except for nativeId), and also public static final instances for each non DECIMAL type. I know it is not perfect, but it would let us make very few code changes. It would reduce the load on GC because most of the time there would not be new DType objects created. It would also be obvious in all places where the APIs got the nativeId that they would need to also be updated to look for a scale too.

i.e.

public final class DType {
  public enum DTypeEnum {
    EMPTY(0, 0, "NOT SUPPORTED"),
    INT8(1, 1, "byte"),
    ...
  }

  public static final DType INT8 = new DType(DTypeEnum.INT8);
  ...
}

We could also verify that if you try to create an object from just a nativeId and it is a DECIMAL* we can blow up.

jlowe · 2020-10-15T15:13:42Z

and also public static final instances for each non DECIMAL type

I was already thinking along these lines, but I hadn't considered taking over DType itself. I'm generally +1 on the idea of commandeering DType, although we will have a "knife switch" event when this gets checked in. The snapshot build published afterwards is going to break anything that hasn't recompiled.

I'm still worried about all the places code assumes checking for equality on a data type can be performed with == which is no longer the case, but we need to worry about that whether it's called DataType or DType. Not calling it DataType nicely side-steps the problem identified in HostColumnVector and also the upcoming name confusion with Spark's DataType.

nartal1 · 2020-10-15T17:40:27Z

Thanks @revans2 and @jlowe for reviewing and your inputs on new design. I will rework on this and update it here. Liked this idea where it solves confusion on naming DataType.

sperlingxx · 2020-10-21T06:51:51Z

Hi @revans2 @jlowe , @nartal1 and I are trying to implement the idea DType with DTypeEnum. Both of us want to ensure whether we fully understand this idea. Here are some code snippets to elaborate our understanding:

  public static final DType EMPTY = new DType(DTypeEnum.EMPTY);
  public static final DType INT8 = new DType(DTypeEnum.INT8);
  public static final DType INT16 = new DType(DTypeEnum.INT16);
  public static final DType INT32 = new DType(DTypeEnum.INT32);
  public static final DType INT64 = new DType(DTypeEnum.INT64);
  public static final DType UINT8 = new DType(DTypeEnum.UINT8);
  public static final DType UINT16 = new DType(DTypeEnum.UINT16);
  public static final DType UINT32 = new DType(DTypeEnum.UINT32);
  public static final DType UINT64 = new DType(DTypeEnum.UINT64);
  public static final DType FLOAT32 = new DType(DTypeEnum.FLOAT32);
  public static final DType FLOAT64 = new DType(DTypeEnum.FLOAT64);
  public static final DType BOOL8 = new DType(DTypeEnum.BOOL8);
  public static final DType TIMESTAMP_DAYS = new DType(DTypeEnum.TIMESTAMP_DAYS);
  public static final DType TIMESTAMP_SECONDS = new DType(DTypeEnum.TIMESTAMP_SECONDS);
  public static final DType TIMESTAMP_MILLISECONDS = new DType(DTypeEnum.TIMESTAMP_MILLISECONDS);
  public static final DType TIMESTAMP_MICROSECONDS = new DType(DTypeEnum.TIMESTAMP_MICROSECONDS);
  public static final DType TIMESTAMP_NANOSECONDS = new DType(DTypeEnum.TIMESTAMP_NANOSECONDS);
  public static final DType DURATION_DAYS = new DType(DTypeEnum.DURATION_DAYS);
  public static final DType DURATION_SECONDS = new DType(DTypeEnum.DURATION_SECONDS);
  public static final DType DURATION_MILLISECONDS = new DType(DTypeEnum.DURATION_MILLISECONDS);
  public static final DType DURATION_MICROSECONDS = new DType(DTypeEnum.DURATION_MICROSECONDS);
  public static final DType DURATION_NANOSECONDS = new DType(DTypeEnum.DURATION_NANOSECONDS);
  public static final DType STRING = new DType(DTypeEnum.STRING);
  public static final DType LIST = new DType(DTypeEnum.LIST);
  public static final DType STRUCT = new DType(DTypeEnum.STRUCT);

  private static final DType[] staticDTypes = new DType[] {
      EMPTY,
      INT8,
      INT16,
      INT32,
      INT64,
      UINT8,
      UINT16,
      UINT32,
      UINT64,
      FLOAT32,
      FLOAT64,
      BOOL8,
      TIMESTAMP_DAYS,
      TIMESTAMP_SECONDS,
      TIMESTAMP_MILLISECONDS,
      TIMESTAMP_NANOSECONDS,
      DURATION_DAYS,
      DURATION_SECONDS,
      DURATION_MILLISECONDS,
      DURATION_MICROSECONDS,
      DURATION_NANOSECONDS,
      null, // DICTIONARY32
      STRING,
      LIST,
      null, // DECIMAL32
      null, // DECIMAL64
      STRUCT
  };

  public static DType fromNative(int nativeId, int scale) {
    if (nativeId >=0 && nativeId < staticDTypes.length) {
      if (staticDTypes[nativeId] != null) {
        return staticDTypes[nativeId];
      }
      if (nativeId == DTypeEnum.DECIMAL32.nativeId) {
        // TODO: maybe we can verify scale at here?
        return new DType(DTypeEnum.DECIMAL32, scale);
      }
      if (nativeId == DTypeEnum.DECIMAL64.nativeId) {
        // TODO: maybe we can verify scale at here?
        return new DType(DTypeEnum.DECIMAL64, scale);
      }
    }
    throw new IllegalArgumentException("Could not translate " + nativeId + " into a DType");
  }

  public static DType fromNative(int nativeId) {
    // TODO: replace Integer.MIN_VALUE with a constant as noop placeholder ?
    return fromNative(nativeId, Integer.MIN_VALUE);
  }

Signed-off-by: Niranjan Artal <[email protected]>

…type_class

Signed-off-by: Niranjan Artal <[email protected]>

nartal1 · 2020-10-24T02:16:57Z

Thanks to @sperlingxx for adding scalar tests. I have merged it in this branch. Apologies for bringing up this late. Thought of adding this here to have some tests.

Signed-off-by: Niranjan Artal <[email protected]>

revans2 · 2020-10-26T13:10:43Z

java/src/main/native/src/dtype_utils.hpp

+ */
+#pragma once
+
+namespace dtype_utils{


nit: This is really minor because all of the functions are inline and the header is private to the JNI code, but I would prefer to have the namespace have something with cudf and java/jni in it.

@nartal1, here is an additional (non-actionable) note on @revans2's namespace comment:

These functions used to be in the "unnamed" namespace in ColumnVectorJni.cpp. Moving it to a separate header and out of the unnamed namespace forced external linkage, and led to the multiple definitions error you saw. This forced your hand with making them inline. You could have alternatively chosen to keep them in the "unnamed" namespace (nested within another cudf::jni namespace).

Thanks @mythrocks , I have made the changes as per suggestion.

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

mythrocks

Some minor changes suggested.

mythrocks · 2020-10-26T17:51:51Z

java/src/main/native/src/dtype_utils.hpp

+ */
+#pragma once
+
+namespace dtype_utils{


@nartal1, here is an additional (non-actionable) note on @revans2's namespace comment:

These functions used to be in the "unnamed" namespace in ColumnVectorJni.cpp. Moving it to a separate header and out of the unnamed namespace forced external linkage, and led to the multiple definitions error you saw. This forced your hand with making them inline. You could have alternatively chosen to keep them in the "unnamed" namespace (nested within another cudf::jni namespace).

java/src/main/native/src/ScalarJni.cpp

Signed-off-by: Niranjan Artal <[email protected]>

…type_class

nartal1 · 2020-10-26T20:12:04Z

@jlowe @mythrocks I think I have addressed review comments. Please take another look.
@razajafri Tagging you as well since you had few comments initially which were resolved :)

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

Signed-off-by: Niranjan Artal <[email protected]>

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

Signed-off-by: Niranjan Artal <[email protected]>

…type_class

nartal1 · 2020-10-27T06:30:25Z

Thanks for the approvals!

nartal1 added 3 commits October 12, 2020 23:34

Initial changes for supporting decimal type

4a4ea1d

Modify constructors and JNI methods to create column.

a549452

Add inner class NativeDataType

31f131b

nartal1 requested a review from a team as a code owner October 13, 2020 08:35

nartal1 marked this pull request as draft October 13, 2020 08:36

sperlingxx mentioned this pull request Oct 13, 2020

[FEA] Java bindings for fixed-point Decimal type #6515

Closed

16 tasks

revans2 reviewed Oct 13, 2020

View reviewed changes

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

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

jlowe reviewed Oct 13, 2020

View reviewed changes

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

nartal1 added 2 commits October 13, 2020 14:50

Merge branch 'branch-0.17' of https://github.com/rapidsai/cudf into d…

4e5f4e1

…type_class

Pass in scale and ordinal instead of saving native data_type handle

23ef61d

sperlingxx reviewed Oct 14, 2020

View reviewed changes

Add DataType in HostColumnVector and address review comments

17e1d30

Signed-off-by: Niranjan Artal <[email protected]>

Support DataType in Builder

b29c59b

jlowe reviewed Oct 14, 2020

View reviewed changes

Merge branch 'branch-0.17' of https://github.com/rapidsai/cudf into d…

a7ae605

…type_class

razajafri added the Java Affects Java cuDF API. label Oct 15, 2020

razajafri requested changes Oct 15, 2020

View reviewed changes

nartal1 added 2 commits October 15, 2020 01:34

addressed few review comments

9a2e948

addressed review comments

da9e4d6

jlowe reviewed Oct 15, 2020

View reviewed changes

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

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

revans2 reviewed Oct 15, 2020

View reviewed changes

nartal1 changed the title ~~[WIP] Initial work for decimal type in Java/JNI [skip ci]~~ [REVIEW] Initial work for decimal type in Java/JNI [skip ci] Oct 24, 2020

nartal1 added 3 commits October 23, 2020 19:09

update changelog

3635007

Signed-off-by: Niranjan Artal <[email protected]>

Merge branch 'branch-0.17' of https://github.com/rapidsai/cudf into d…

9680e44

…type_class

update changelog

fa8af4b

Signed-off-by: Niranjan Artal <[email protected]>

updated toString method:

5c52d88

Signed-off-by: Niranjan Artal <[email protected]>

revans2 reviewed Oct 26, 2020

View reviewed changes

revans2 approved these changes Oct 26, 2020

View reviewed changes

jlowe reviewed Oct 26, 2020

View reviewed changes

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

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

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

mythrocks requested changes Oct 26, 2020

View reviewed changes

nartal1 added 3 commits October 26, 2020 13:05

addressed review comments

6ad640b

Signed-off-by: Niranjan Artal <[email protected]>

Merge branch 'branch-0.17' of https://github.com/rapidsai/cudf into d…

9b5b4b1

…type_class

update changelog

767b50c

jlowe reviewed Oct 26, 2020

View reviewed changes

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

update comments

6d2bac9

Signed-off-by: Niranjan Artal <[email protected]>

jlowe reviewed Oct 26, 2020

View reviewed changes

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

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

update comments

74c311c

Signed-off-by: Niranjan Artal <[email protected]>

jlowe approved these changes Oct 26, 2020

View reviewed changes

mythrocks approved these changes Oct 26, 2020

View reviewed changes

razajafri approved these changes Oct 27, 2020

View reviewed changes

nartal1 added 2 commits October 26, 2020 23:16

Merge branch 'branch-0.17' of https://github.com/rapidsai/cudf into d…

ef9d557

…type_class

update changelog

f5c6d56

nartal1 added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 4 - Needs cuDF (Java) Reviewer labels Oct 27, 2020

kkraus14 merged commit 4b4d962 into rapidsai:branch-0.17 Oct 27, 2020

jlowe mentioned this pull request Feb 17, 2021

[FEA] Java bindings for DECIMAL32 and DECIMAL64 types #5819

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[REVIEW] Initial work for decimal type in Java/JNI [skip ci] #6514

[REVIEW] Initial work for decimal type in Java/JNI [skip ci] #6514

nartal1 commented Oct 13, 2020 •

edited

Loading

GPUtester commented Oct 13, 2020

revans2 left a comment

sperlingxx Oct 14, 2020

sperlingxx Oct 14, 2020

nartal1 commented Oct 14, 2020

revans2 commented Oct 14, 2020

nartal1 commented Oct 14, 2020

razajafri Oct 15, 2020

nartal1 Oct 15, 2020

razajafri Oct 15, 2020

nartal1 Oct 15, 2020

razajafri Oct 15, 2020

nartal1 Oct 15, 2020

razajafri Oct 15, 2020

nartal1 Oct 15, 2020

razajafri Oct 15, 2020

nartal1 Oct 15, 2020

razajafri Oct 15, 2020

revans2 Oct 21, 2020

revans2 commented Oct 15, 2020

jlowe commented Oct 15, 2020

nartal1 commented Oct 15, 2020

sperlingxx commented Oct 21, 2020

nartal1 commented Oct 24, 2020 •

edited

Loading

revans2 Oct 26, 2020

mythrocks Oct 26, 2020

nartal1 Oct 26, 2020

mythrocks left a comment

mythrocks Oct 26, 2020

nartal1 commented Oct 26, 2020

nartal1 commented Oct 27, 2020

		@@ -38,10 +38,12 @@

		protected final OffHeapState offHeap;
		protected final DType type;

[REVIEW] Initial work for decimal type in Java/JNI [skip ci] #6514

[REVIEW] Initial work for decimal type in Java/JNI [skip ci] #6514

Conversation

nartal1 commented Oct 13, 2020 • edited Loading

GPUtester commented Oct 13, 2020

revans2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nartal1 commented Oct 14, 2020

revans2 commented Oct 14, 2020

nartal1 commented Oct 14, 2020

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

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

revans2 commented Oct 15, 2020

jlowe commented Oct 15, 2020

nartal1 commented Oct 15, 2020

sperlingxx commented Oct 21, 2020

nartal1 commented Oct 24, 2020 • edited Loading

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

Choose a reason for hiding this comment

nartal1 commented Oct 26, 2020

nartal1 commented Oct 27, 2020

nartal1 commented Oct 13, 2020 •

edited

Loading

nartal1 commented Oct 24, 2020 •

edited

Loading