Fix calculation of null counts for Parquet statistics #12938

etseidl · 2023-03-13T23:40:05Z

Description

The current Parquet writer sometimes generates wrong values for null_count in the column chunk statistics and page indexes. This occurs for nested schemas when nulls occur at a level above the leaf values. This PR fixes the calculation by adding a non_leaf_nulls field to the statistics_group struct. This field is added to the chunk null_count calculated over leaf values in gpu_calculate_group_statistics().

Checklist

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

…feature/parquet_nulls

rapids-bot · 2023-03-13T23:40:10Z

Pull requests from external contributors require approval from a rapidsai organization member with write or admin permissions before CI can begin.

vyasr · 2023-03-13T23:45:53Z

/ok to test

vuule

Looks good, just a few nitpicks in tests

vuule · 2023-03-14T20:51:36Z

cpp/tests/io/parquet_test.cpp

+TEST_F(ParquetWriterTest, CheckColumnIndexListWithNulls)
+{
+  auto valids  = cudf::detail::make_counting_transform_iterator(0, [](auto i) { return i % 2; });
+  auto valids2 = cudf::detail::make_counting_transform_iterator(0, [](auto i) { return i != 3; });


You should be able to use our special test iterator here

Suggested change

auto valids2 = cudf::detail::make_counting_transform_iterator(0, [](auto i) { return i != 3; });

auto null_at_3 = cudf::test::iterators::null_at(3);

vuule · 2023-03-14T20:51:42Z

cpp/src/io/parquet/page_enc.cu

@@ -282,6 +282,7 @@ __global__ void __launch_bounds__(128)
      g.col            = ck_g->col_desc;
      g.start_row      = fragments[frag_id].start_value_idx;
      g.num_rows       = fragments[frag_id].num_leaf_values;
+      g.non_leaf_nulls = fragments[frag_id].num_values - g.num_rows;


This will account for all nulls above the leaf level, even if we have nested lists?

Yes, it should. For instance, here's a dump of the first test case:

value 1: R:0 D:2 V:<null> value 2: R:1 D:3 V:2 value 3: R:1 D:2 V:<null> value 4: R:0 D:1 V:<null> value 5: R:0 D:3 V:4 value 6: R:1 D:3 V:5 value 7: R:0 D:0 V:<null>

The R:0:D1 value is an empty list, while R0:D0 is a null list. The D2 nulls are leaves. Likewise for list of list:

value 1: R:0 D:5 V:1 value 2: R:2 D:5 V:2 value 3: R:2 D:5 V:3 value 4: R:1 D:3 V:<null> value 5: R:1 D:5 V:4 value 6: R:2 D:5 V:5 value 7: R:1 D:2 V:<null> value 8: R:1 D:4 V:<null> value 9: R:2 D:5 V:6 value 10: R:2 D:4 V:<null> value 11: R:0 D:5 V:7 value 12: R:2 D:5 V:8 value 13: R:0 D:1 V:<null> value 14: R:0 D:0 V:<null>

cpp/tests/io/parquet_test.cpp

bdice

A few small suggestions, mostly for tests. Nothing is blocking here, so I'll approve.

bdice · 2023-03-14T22:14:54Z

cpp/tests/io/parquet_test.cpp

@@ -4279,6 +4279,9 @@ TEST_F(ParquetWriterTest, CheckColumnOffsetIndexNulls)
      auto const ci    = read_column_index(source, chunk);
      auto const stats = parse_statistics(chunk);

+      // should be half nulls, except no nulls in column 0
+      EXPECT_EQ(stats.null_count, c > 0 ? num_rows / 2 : 0);


I weakly prefer this spelling:

Suggested change

EXPECT_EQ(stats.null_count, c > 0 ? num_rows / 2 : 0);

EXPECT_EQ(stats.null_count, c == 0 ? 0 : num_rows / 2);

bdice · 2023-03-14T22:17:34Z

cpp/tests/io/parquet_test.cpp

+{
+  auto null_at_even_idx =
+    cudf::detail::make_counting_transform_iterator(0, [](auto i) { return i % 2; });
+  auto null_at_idx_3 = cudf::test::iterators::null_at(3);


I think we typically write using cudf::test::iterators::null_at; and then write null_at(3) in the appropriate locations.

should I get rid of the declaration here and just use null_at(3) inline everywhere? (as with nulls_at({0, 2}) below?)

Yup, that would be fine.

bdice · 2023-03-14T22:20:24Z

cpp/tests/io/parquet_test.cpp

@@ -4465,6 +4471,138 @@ TEST_F(ParquetWriterTest, CheckColumnOffsetIndexStruct)
  }
 }

+TEST_F(ParquetWriterTest, CheckColumnIndexListWithNulls)
+{
+  auto null_at_even_idx =


It looks like the places where this is used are only nulling out one or two values. Could we just write using cudf::test::iterators::nulls_at; and then nulls_at({0, 2}) or similar in those locations? If there was a longer list, then I would support using an iterator, but not for 1-2 values where being explicit is still concise. I don't feel strongly here, so we could leave this as-is if you prefer.

cpp/tests/io/parquet_test.cpp

ttnghia · 2023-03-16T23:01:14Z

/ok to test

vuule · 2023-03-17T06:53:43Z

/merge

etseidl and others added 6 commits March 13, 2023 15:55

fix calculation of null values for parquet stats

743bcf0

Merge branch 'rapidsai:branch-23.04' into feature/parquet_nulls

17e6205

check nulls in more tests

41604ab

Merge branch 'feature/parquet_nulls' of github.com:etseidl/cudf into …

e2259d7

…feature/parquet_nulls

remove unneeded metadata

313be05

remove todo

2a9b927

etseidl requested a review from a team as a code owner March 13, 2023 23:40

etseidl requested review from harrism and vuule March 13, 2023 23:40

github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Mar 13, 2023

vyasr added bug Something isn't working 3 - Ready for Review Ready for review by team non-breaking Non-breaking change labels Mar 13, 2023

vuule reviewed Mar 14, 2023

View reviewed changes

descriptive names for validity iterators

42b57e1

vuule approved these changes Mar 14, 2023

View reviewed changes

bdice approved these changes Mar 14, 2023

View reviewed changes

implement suggestions from review

c7dd41c

harrism removed their request for review March 15, 2023 04:45

hyperbolic2346 approved these changes Mar 15, 2023

View reviewed changes

etseidl and others added 2 commits March 15, 2023 08:21

Merge branch 'rapidsai:branch-23.04' into feature/parquet_nulls

2e7154a

Merge branch 'branch-23.04' into feature/parquet_nulls

7b66638

rapids-bot bot merged commit 3540613 into rapidsai:branch-23.04 Mar 17, 2023

etseidl deleted the feature/parquet_nulls branch March 17, 2023 15:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix calculation of null counts for Parquet statistics #12938

Fix calculation of null counts for Parquet statistics #12938

etseidl commented Mar 13, 2023

rapids-bot bot commented Mar 13, 2023

vyasr commented Mar 13, 2023

vuule left a comment

vuule Mar 14, 2023

vuule Mar 14, 2023

etseidl Mar 14, 2023

bdice left a comment

bdice Mar 14, 2023

bdice Mar 14, 2023

etseidl Mar 14, 2023

bdice Mar 14, 2023

bdice Mar 14, 2023

ttnghia commented Mar 16, 2023

vuule commented Mar 17, 2023

	auto valids2 = cudf::detail::make_counting_transform_iterator(0, [](auto i) { return i != 3; });
	auto null_at_3 = cudf::test::iterators::null_at(3);

	EXPECT_EQ(stats.null_count, c > 0 ? num_rows / 2 : 0);
	EXPECT_EQ(stats.null_count, c == 0 ? 0 : num_rows / 2);

Fix calculation of null counts for Parquet statistics #12938

Fix calculation of null counts for Parquet statistics #12938

Conversation

etseidl commented Mar 13, 2023

Description

Checklist

rapids-bot bot commented Mar 13, 2023

vyasr commented Mar 13, 2023

vuule 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

bdice 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

ttnghia commented Mar 16, 2023

vuule commented Mar 17, 2023