Introduce kudo writer. #2559

liurenjie1024 · 2024-11-01T09:23:01Z

This pr is part of #2532 , which introduces writer part of kudo format.

Signed-off-by: liurenjie1024 <[email protected]>

liurenjie1024 · 2024-11-01T09:23:41Z

build

src/main/java/com/nvidia/spark/rapids/jni/kudo/KudoSerializer.java

src/main/java/com/nvidia/spark/rapids/jni/kudo/SerializedTableHeader.java

src/main/java/com/nvidia/spark/rapids/jni/kudo/SerializedTableHeaderCalc.java

src/main/java/com/nvidia/spark/rapids/jni/kudo/DataOutputStreamWriter.java

src/main/java/com/nvidia/spark/rapids/jni/kudo/SerializedTableHeaderCalc.java

Signed-off-by: liurenjie1024 <[email protected]>

liurenjie1024 · 2024-11-04T06:12:19Z

build

jlowe

There should be some unit tests added (e.g.: serializing row-count only, serializing a simple table of a single int column, serializing a list, serializing a struct, etc.) that examine the bytes serialized to ensure it matches the expected format.

src/main/java/com/nvidia/spark/rapids/jni/kudo/KudoTableHeader.java

src/main/java/com/nvidia/spark/rapids/jni/kudo/KudoSerializer.java

src/main/java/com/nvidia/spark/rapids/jni/kudo/KudoTableHeader.java

src/main/java/com/nvidia/spark/rapids/jni/kudo/SerializedTableHeaderCalc.java

src/main/java/com/nvidia/spark/rapids/jni/kudo/KudoTableHeaderCalc.java

src/main/java/com/nvidia/spark/rapids/jni/kudo/SerializedTableHeaderCalc.java

src/main/java/com/nvidia/spark/rapids/jni/kudo/DataWriter.java

revans2 · 2024-11-04T21:55:37Z

src/main/java/com/nvidia/spark/rapids/jni/kudo/KudoSerializer.java

+ *     <tr>
+ *         <td>Offset</td>
+ *         <td>4</td>
+ *         <td>Row offset in original table, in little endian format</td>


Is this actually little endian? I thought that DataOutputStream always writes data in network order (big endian)

https://docs.oracle.com/javase/8/docs/api/java/io/DataOutput.html#writeLong-long-

https://guava.dev/releases/17.0/api/docs/com/google/common/io/LittleEndianDataOutputStream.html

Not sure if this is bleedover from what I did with jcudf serialization or not. That did some things little-endian on the hope that the data could be read by native code without needing to remap it at all.

This comment goes for all of the rows here that mention little endian.

Good catch! Fixed comments.

revans2 · 2024-11-04T21:57:18Z

src/main/java/com/nvidia/spark/rapids/jni/kudo/KudoSerializer.java

+ * The body consists of three part:
+ * <ol>
+ *     <li>Validity buffers for every column with validity in depth-first ordering of schema columns. Each buffer of
+ *     each column is 4 bytes padded.


nit: Can we have a follow on issue to see if padding actually is a win or not? I can see that padding might make some things faster for memory access, but most of the time having less data might be even better. But this is super minor.

At first the 4 byte padding is just to ensure that it would be easier to handle offset at read time, since we always need to getInt from buffer directly. Now we have another justification for this padding, see #2532 (comment) . Also I think whether it's a win or not depends on data distribution.

src/main/java/com/nvidia/spark/rapids/jni/kudo/KudoSerializer.java

revans2 · 2024-11-04T22:04:33Z

src/main/java/com/nvidia/spark/rapids/jni/kudo/KudoSerializer.java

+  }
+
+  static int safeLongToInt(long value) {
+    assert value >= Integer.MIN_VALUE  : "Value is too small to fit in an int";


Do we ever want to write negative numbers for any of these values?

This method was used for more general purpose rather than in table header only. I've fix it to invalidate negative numbers, and will add it back when necessary.

src/main/java/com/nvidia/spark/rapids/jni/kudo/KudoTableHeader.java

Signed-off-by: liurenjie1024 <[email protected]>

liurenjie1024 · 2024-11-05T07:04:28Z

There should be some unit tests added (e.g.: serializing row-count only, serializing a simple table of a single int column, serializing a list, serializing a struct, etc.) that examine the bytes serialized to ensure it matches the expected format.

I've added some simple uts, more sophiscated tests will come after we checked in reader part.

Signed-off-by: liurenjie1024 <[email protected]>

liurenjie1024 · 2024-11-05T09:09:48Z

build

src/main/java/com/nvidia/spark/rapids/jni/kudo/KudoSerializer.java

src/main/java/com/nvidia/spark/rapids/jni/kudo/KudoTableHeader.java

src/test/java/com/nvidia/spark/rapids/jni/KudoSerializerTest.java

revans2

It looks good enough to ship for me. Especially because the API I have issues with is package private so we can change it later. But I want to hear from Jason on this too.

revans2 · 2024-11-05T16:09:04Z

src/main/java/com/nvidia/spark/rapids/jni/kudo/KudoTableHeader.java

-  public KudoTableHeader(DataInputStream din) throws IOException {
-    readFrom(din);
+    return new KudoTableHeader(offset, numRows, validityBufferLen, offsetBufferLen, totalDataLen, numColumns,
+        hasValidityBuffer);
  }

  KudoTableHeader(long offset, long numRows, long validityBufferLen, long offsetBufferLen,


Still confused why most of these are longs if we are going to make sure that they fit in the range of ints. Especially if this is a package private API that we are the consumers/producers of. That means that safeLongToNonNegativeInt, really would just become something to ensure that the value is not-negative.

I get your point. The problem is that if we use int arguments, what we need to do is:

Check values are non negative in constructor.

Check values are in the range of ints in the caller.

Since this api is currently package private and the package is both consumer and producer, I'm just putting these checks in one place. Now I think splitting these checks makes api semantics clearer and easier to read.

Signed-off-by: liurenjie1024 <[email protected]>

liurenjie1024 · 2024-11-06T02:36:48Z

build

Signed-off-by: liurenjie1024 <[email protected]>

abellina · 2024-11-08T14:51:48Z

src/main/java/com/nvidia/spark/rapids/jni/kudo/KudoSerializer.java

+  public long writeToStream(Table table, OutputStream out, int rowOffset, int numRows) {
+    HostColumnVector[] columns = null;
+    try {
+      columns = IntStream.range(0, table.getNumberOfColumns())


This looks like we are writing the whole Table (not sliced? but this is probably what I am missing) for every single partition of such table. Is that true? In the past we had 1 copy per column, not 200 copies per column. I am not seeing where this is getting called so that could explain it.

This is currently used in tests only, not used in spark-rapids. What's actually used is the one under this. I agree that it would be wasteful to copy for each partition. Would marking it as tests only be useful?

ah yes, that would be great to mark as "test only". It wasn't obvious to me.

abellina · 2024-11-08T14:54:06Z

src/main/java/com/nvidia/spark/rapids/jni/kudo/KudoSerializer.java

+    return paddedBytes;
+  }
+
+  static long padFor64byteAlignment(long orig) {


this looks to be unused.

It's used by following pr.

abellina · 2024-11-08T15:00:44Z

src/test/java/com/nvidia/spark/rapids/jni/KudoSerializerTest.java

+public class KudoSerializerTest {
+
+  @Test
+  public void testRowCountOnly() throws Exception {


this doesn't test a no-rows case, but it would be nice to add.

Sure, will fix in following pr.

abellina · 2024-11-08T15:01:58Z

src/test/java/com/nvidia/spark/rapids/jni/KudoSerializerTest.java

+    return builder.build();
+  }
+
+  private static Table buildSimpleTable() {


it would be nice to add a deeply nested case as well. The integration tests in spark-rapids are likely to cover some of this, but as a serialization format, I could see a bit more permutations of columns added to the tests.

There are more tests in following pr.

abellina · 2024-11-08T15:05:52Z

src/test/java/com/nvidia/spark/rapids/jni/KudoSerializerTest.java

+        new HostColumnVector.BasicType(true, DType.INT64)
+    );
+    return new Table.TestBuilder()
+        .column(1, 2, 3, 4)


could we have a case where the number of bytes in each slice is Int.MaxValue? We have tests for a small number of things, and 0 rows, but not the limit.

Good point. Will add some tests in following pr.

Introduce kudo writer

07717d6

Signed-off-by: liurenjie1024 <[email protected]>

liurenjie1024 requested review from jlowe and revans2 November 1, 2024 09:23

jlowe reviewed Nov 1, 2024

View reviewed changes

abellina self-requested a review November 1, 2024 19:25

Fix comments

b776e9c

Signed-off-by: liurenjie1024 <[email protected]>

jlowe reviewed Nov 4, 2024

View reviewed changes

revans2 reviewed Nov 4, 2024

View reviewed changes

Fix comments

0384217

Signed-off-by: liurenjie1024 <[email protected]>

liurenjie1024 added 4 commits November 5, 2024 16:16

Fix build break

eba7b07

Signed-off-by: liurenjie1024 <[email protected]>

Fix test error

84aee15

Signed-off-by: liurenjie1024 <[email protected]>

Fix test error

61b927e

Signed-off-by: liurenjie1024 <[email protected]>

Fix test error

5bd7201

Signed-off-by: liurenjie1024 <[email protected]>

jlowe mentioned this pull request Nov 5, 2024

[BUG] CI does not catch missing copyright or license on new files #2571

Closed

jlowe reviewed Nov 5, 2024

View reviewed changes

revans2 previously approved these changes Nov 5, 2024

View reviewed changes

Fix comments

f11e171

Signed-off-by: liurenjie1024 <[email protected]>

liurenjie1024 dismissed revans2’s stale review via f11e171 November 6, 2024 02:01

Fix comments

dc9ad94

Signed-off-by: liurenjie1024 <[email protected]>

Simple fix

14a1fb0

Signed-off-by: liurenjie1024 <[email protected]>

jlowe approved these changes Nov 6, 2024

View reviewed changes

jlowe merged commit 2aa3348 into NVIDIA:branch-24.12 Nov 6, 2024
3 checks passed

liurenjie1024 deleted the ray/kudo-writer branch November 7, 2024 01:34

abellina reviewed Nov 8, 2024

View reviewed changes

sameerz added the performance label Nov 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce kudo writer. #2559

Introduce kudo writer. #2559

liurenjie1024 commented Nov 1, 2024

liurenjie1024 commented Nov 1, 2024

liurenjie1024 commented Nov 4, 2024

jlowe left a comment

revans2 Nov 4, 2024

liurenjie1024 Nov 5, 2024

revans2 Nov 4, 2024

liurenjie1024 Nov 5, 2024

revans2 Nov 4, 2024

liurenjie1024 Nov 5, 2024 •

edited

Loading

liurenjie1024 commented Nov 5, 2024

liurenjie1024 commented Nov 5, 2024

revans2 left a comment

revans2 Nov 5, 2024

liurenjie1024 Nov 6, 2024

liurenjie1024 Nov 6, 2024

liurenjie1024 commented Nov 6, 2024

abellina Nov 8, 2024

liurenjie1024 Nov 11, 2024

abellina Nov 12, 2024

abellina Nov 8, 2024

liurenjie1024 Nov 11, 2024

abellina Nov 8, 2024

liurenjie1024 Nov 11, 2024

abellina Nov 8, 2024

liurenjie1024 Nov 11, 2024

abellina Nov 8, 2024 •

edited

Loading

liurenjie1024 Nov 11, 2024

Introduce kudo writer. #2559

Introduce kudo writer. #2559

Conversation

liurenjie1024 commented Nov 1, 2024

liurenjie1024 commented Nov 1, 2024

liurenjie1024 commented Nov 4, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liurenjie1024 Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

liurenjie1024 commented Nov 5, 2024

liurenjie1024 commented Nov 5, 2024

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

Choose a reason for hiding this comment

liurenjie1024 commented Nov 6, 2024

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

abellina Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liurenjie1024 Nov 5, 2024 •

edited

Loading

abellina Nov 8, 2024 •

edited

Loading