Support nested types in ORC writer #3696

firestarman · 2021-09-29T06:11:51Z

This fixes #3494 .

And also adds support for lists.

Signed-off-by: Firestarman [email protected]

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

firestarman · 2021-09-29T06:12:49Z

Depending on rapidsai/cudf#9334. So mark it as draft.

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

razajafri · 2021-09-29T16:30:07Z

sql-plugin/src/main/scala/org/apache/spark/sql/rapids/GpuOrcFileFormat.scala

+
+  /**
+   * (We could try to merge this with `parquetWriterOptionsFromSchema` after fixing the issue
+   *  https://github.com/rapidsai/cudf/issues/7654)


This issue is circumvented by pruning masks ourselves and we are already calling the remove_validity_if_needed from the writeORCChunk in TableJni.cpp. So I don't see why this should be preventing us from merging the two methods?

Thanks for the info. But I tried locally and still getting the exception below when running the Parquet test test_write_map_nullable after changing the nullable to go back to the actual setting.

E Caused by: ai.rapids.cudf.CudfException: cuDF failure at: /home/liangcail/work/projects/on_github/cudf/cpp/src/io/parquet/writer_impl.cu:377: Mismatch in metadata prescribed nullability and input column nullability. Metadata for nullable input column cannot prescribe nullability = false E at ai.rapids.cudf.Table.writeParquetChunk(Native Method) E at ai.rapids.cudf.Table.access$300(Table.java:39)

This is probably a bug in plugin. Working on it.

Fixed it by using m.valueContainsNull instead of the parameter nullable when building options from MapType.

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

Nested map is not supported yet. Signed-off-by: Firestarman <[email protected]>

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

firestarman · 2021-10-12T01:07:37Z

@razajafri Could you review it again ?

wbo4958 · 2021-10-12T01:27:06Z

sql-plugin/src/main/scala/com/nvidia/spark/rapids/SchemaUtils.scala

@@ -21,7 +21,8 @@ import java.util.Optional
 import scala.collection.mutable.ArrayBuffer
 import scala.language.implicitConversions

-import ai.rapids.cudf.{ColumnView, Table}
+import ai.rapids.cudf._


I don't think it's a good way to import all from cudf.

Yes good suggestion, but this is done by IDE, if you prefer, i can change it back.

Not really. Since it's a breaking change, let's merge it as soon as possible.

wbo4958 · 2021-10-12T01:33:44Z

sql-plugin/src/main/scala/com/nvidia/spark/rapids/SchemaUtils.scala

@@ -21,7 +21,8 @@ import java.util.Optional
 import scala.collection.mutable.ArrayBuffer
 import scala.language.implicitConversions

-import ai.rapids.cudf.{ColumnView, Table}
+import ai.rapids.cudf._
+import ai.rapids.cudf.ColumnWriterOptions._


I didn't find ColumnWriterOptions in cudf side. Is there a pending PR?

Yes, it depends on rapidsai/cudf#9334. I will merge it once this PR gets approvals.

wbo4958 · 2021-10-12T01:38:52Z

overall, LTGM

firestarman · 2021-10-12T10:32:04Z

Will trigger CI after rapidsai/cudf#9334 being merged.

revans2 · 2021-10-12T11:45:18Z

Marked as draft just because rapidsai/cudf#9334 is not merged yet. When it is merged this can go back to ready.

firestarman · 2021-10-12T23:50:42Z

Marked as draft just because rapidsai/cudf#9334 is not merged yet. When it is merged this can go back to ready.

rapidsai/cudf#9334 is a breaking change to plugin, and this PR should be merged as soon as possible after #9334 being merged. So I plan to get an approval first, then merge the cudf PR and trigger the CI. Then I can merge this PR once CI is done. Otherwise it will take more time to get this in.
Even this got an approval, it still can not be merged because the CI is not passed yet.

res-life

LGTM

firestarman · 2021-10-13T01:35:05Z

build

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

firestarman · 2021-10-13T02:07:20Z

build

abellina · 2021-10-13T02:37:40Z

integration_tests/src/main/python/orc_write_test.py

+
+orc_write_basic_struct_gen = StructGen([['child'+str(ind), sub_gen] for ind, sub_gen in enumerate(orc_write_basic_gens)])
+
+# Some array/struct gens, but not all because of nesting


mind expanding on this comment a bit, why "not all because of nesting"?

Good suggestion, I will update it in a following PR since this PR should be merged as soon as possible.

I removed this comment.

abellina · 2021-10-13T02:40:36Z

sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuOverrides.scala

@@ -852,7 +852,8 @@ object GpuOverrides extends Logging {
    (OrcFormatType, FileFormatChecks(
      cudfRead = (TypeSig.commonCudfTypes + TypeSig.ARRAY + TypeSig.DECIMAL_64 +
          TypeSig.STRUCT + TypeSig.MAP).nested(),
-      cudfWrite = TypeSig.commonCudfTypes,
+      cudfWrite = (TypeSig.commonCudfTypes + TypeSig.ARRAY +


also I get confused with TypeSig. So we can read nested maps, but not write them? Or did I misunderstand? Also is there a follow on issue to handle the same types that the reader supports.

Yes, this is the truth. We can read nested map but can not write nested map. Which is limited by the cudf native orc writer.
I will file an issue to cudf first. Once cudf supported it, we can update the TypeSig here.

Now I removed the map support due to the test failures only happened in pre-merge builds.
We have the issue #3784 to track this.

abellina · 2021-10-13T02:50:43Z

sql-plugin/src/main/scala/com/nvidia/spark/rapids/SchemaUtils.scala

+        builder.withDecimalColumn(name, dt.precision, nullable)
+      case TimestampType =>
+        builder.withTimestampColumn(name, writeInt96, nullable)
+      case s: StructType =>


The previous version of this code for struct and array defaulted both to nullable (and had a comment that is missing in your change).

// we are setting this to nullable, in case the parent is a Map's key and wants to
// set this to false

Why don't these columns need to be set nullable?

We had discussion above for Zara's comments.
I moved similar comments to the beginning of method writerOptionsFromSchema.

Here setting to nullable with the comment before because nullale was hard-coded to true due to the issue rapidsai/cudf#7654.
But this issue has been circumvented by PR rapidsai/cudf#9061, so we can still keep it being nullable, but the comment is no longer needed.

You can see the structBuilder and listBuilder are still using nullable for parent columns, while for child columns, we should use containsNull for array and valueContainsNull for map to tell whether the children are nullable.

firestarman · 2021-10-13T03:43:55Z

build

firestarman · 2021-10-13T03:44:48Z

Can not find out which test failed from the log, re-build again.
My local build for 320 passed.

firestarman · 2021-10-13T05:44:14Z

build

GaryShen2008

Approved first to unblock the CI.

Try to fix a build error in premerge builds which running tests in parallel. Signed-off-by: Firestarman <[email protected]>

firestarman · 2021-10-13T07:07:19Z

build

Because map tests failed in premerge builds where tests run in parallel. Signed-off-by: Firestarman <[email protected]>

firestarman · 2021-10-13T08:06:17Z

build

firestarman · 2021-10-13T08:15:43Z

Tests of orc writing maps always fail, so removed the map support. It can not reproduce locally. We will add map support back in the future.

firestarman · 2021-10-13T10:15:47Z

@revans2 @razajafri
I am going to merge this to unblock CI.
If any concern, I will fix it in following PRs.

pxLi

approve to unblock CI. Please file following issues later

firestarman · 2021-10-13T10:16:58Z

approve to unblock CI. Please file following issues later

Thanks, and the test failure can be tracked by #3784 .

Support structs/lists in ORC writer

52da3f9

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

firestarman marked this pull request as draft September 29, 2021 06:12

firestarman mentioned this pull request Sep 29, 2021

JNI: Support nested types in ORC writer rapidsai/cudf#9334

Merged

firestarman added the cudf_dependency An issue or PR with this label depends on a new feature in cudf label Sep 29, 2021

firestarman linked an issue Sep 29, 2021 that may be closed by this pull request

[FEA] Support structs in ORC writer #3494

Closed

Comment update

533aa89

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

razajafri requested changes Sep 29, 2021

View reviewed changes

firestarman added 5 commits October 8, 2021 12:19

Address the comment

4d97290

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

Merge branch 'branch-21.12' into orc-write

4d07e95

Merge branch 'branch-21.12' into orc-write

bd5c0f7

ORC and Parquet share the same method to build options.

d0593b3

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

Add map support in orc writer

6ba8dd9

Nested map is not supported yet. Signed-off-by: Firestarman <[email protected]>

firestarman changed the title ~~Support structs and lists in ORC writer~~ Support nested types in ORC writer Oct 9, 2021

firestarman marked this pull request as ready for review October 11, 2021 00:05

firestarman marked this pull request as draft October 11, 2021 00:08

firestarman marked this pull request as ready for review October 11, 2021 00:19

firestarman added 2 commits October 11, 2021 13:39

Address comments

b1cac2c

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

Merge branch 'branch-21.12' into orc-write

e877b7e

firestarman linked an issue Oct 11, 2021 that may be closed by this pull request

[FEA] Support ORC write Map column(single level) #3784

Closed

firestarman requested a review from razajafri October 11, 2021 10:39

firestarman requested review from res-life and revans2 October 12, 2021 01:07

wbo4958 reviewed Oct 12, 2021

View reviewed changes

revans2 marked this pull request as draft October 12, 2021 11:44

res-life reviewed Oct 13, 2021

View reviewed changes

firestarman marked this pull request as ready for review October 13, 2021 01:35

firestarman added 2 commits October 13, 2021 09:53

Merge branch 'branch-21.12' into orc-write

225d487

Fix a build error

b2c6b73

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

abellina reviewed Oct 13, 2021

View reviewed changes

GaryShen2008 previously approved these changes Oct 13, 2021

View reviewed changes

New map gens for orc write

0b6e599

Try to fix a build error in premerge builds which running tests in parallel. Signed-off-by: Firestarman <[email protected]>

firestarman dismissed GaryShen2008’s stale review via 0b6e599 October 13, 2021 07:06

Remove map support.

e4f7bdd

Because map tests failed in premerge builds where tests run in parallel. Signed-off-by: Firestarman <[email protected]>

firestarman removed a link to an issue Oct 13, 2021

[FEA] Support ORC write Map column(single level) #3784

Closed

pxLi approved these changes Oct 13, 2021

View reviewed changes

GaryShen2008 approved these changes Oct 13, 2021

View reviewed changes

GaryShen2008 merged commit 7d73931 into NVIDIA:branch-21.12 Oct 13, 2021

firestarman deleted the orc-write branch October 13, 2021 10:20


		orc_write_basic_struct_gen = StructGen([['child'+str(ind), sub_gen] for ind, sub_gen in enumerate(orc_write_basic_gens)])

		# Some array/struct gens, but not all because of nesting

Support nested types in ORC writer #3696

Support nested types in ORC writer #3696

Conversation

firestarman commented Sep 29, 2021 • edited Loading

firestarman commented Sep 29, 2021 • edited Loading

Choose a reason for hiding this comment

firestarman Oct 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

firestarman commented Oct 12, 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

firestarman Oct 12, 2021 • edited Loading

Choose a reason for hiding this comment

wbo4958 commented Oct 12, 2021

firestarman commented Oct 12, 2021

revans2 commented Oct 12, 2021

firestarman commented Oct 12, 2021 • edited Loading

res-life left a comment

Choose a reason for hiding this comment

firestarman commented Oct 13, 2021

firestarman commented Oct 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abellina Oct 13, 2021 • edited Loading

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

firestarman Oct 13, 2021 • edited Loading

Choose a reason for hiding this comment

firestarman commented Oct 13, 2021

firestarman commented Oct 13, 2021 • edited Loading

firestarman commented Oct 13, 2021

GaryShen2008 left a comment

Choose a reason for hiding this comment

firestarman commented Oct 13, 2021

firestarman commented Oct 13, 2021

firestarman commented Oct 13, 2021 • edited Loading

firestarman commented Oct 13, 2021 • edited Loading

pxLi left a comment

Choose a reason for hiding this comment

firestarman commented Oct 13, 2021

firestarman commented Sep 29, 2021 •

edited

Loading

firestarman commented Sep 29, 2021 •

edited

Loading

firestarman Oct 8, 2021 •

edited

Loading

firestarman Oct 12, 2021 •

edited

Loading

firestarman commented Oct 12, 2021 •

edited

Loading

abellina Oct 13, 2021 •

edited

Loading

firestarman Oct 13, 2021 •

edited

Loading

firestarman commented Oct 13, 2021 •

edited

Loading

firestarman commented Oct 13, 2021 •

edited

Loading

firestarman commented Oct 13, 2021 •

edited

Loading