Refactor Parquet reader #12046

ttnghia · 2022-11-02T03:26:02Z

This is a rather non-simple refactor of Parquet reader, no new features or changes in algorithms were made:

Rename some functions.
Moving a lot of declarations and definitions of functions/structs/classes around.
Extract out some functions/structs/classes and put them into new files.
Rewrite doxgen for some functions
Use aliases for member variables (to shorten their names), instead of passing them as function parameters
Etc.

Note that this is merely moving the current implementation around, preparing for adding chunked Parquet reader which is a fairly large implementation.

This is also a blocker for:

…taining a mix of nested and non-nested types would result in incorrect row counts for the non-nested types. Also optimizes the preprocess path so that non-nested types do not end up getting visited by the kernel.

…ists. Fixed an additional issue in the decoding where flat column types underneath structs could end up ignoring skip_rows/num_rows.

Signed-off-by: Nghia Truong <[email protected]>

Signed-off-by: Nghia Truong <[email protected]> # Conflicts: # cpp/src/io/parquet/page_data.cu # cpp/src/io/parquet/reader_impl.cu # cpp/src/io/parquet/reader_impl.hpp

Signed-off-by: Nghia Truong <[email protected]>

vyasr

CMake LGTM

karthikeyann · 2022-11-04T14:08:17Z

rerun tests

PointKernel

Some nits.

Looks good!

cpp/src/io/parquet/writer_impl.cu

cpp/include/cudf/io/detail/parquet.hpp

cpp/src/io/parquet/parquet_gpu.cuh

cpp/src/io/parquet/reader_impl.hpp

Signed-off-by: Nghia Truong <[email protected]>

vuule

Very hard to review with all moved code, but (AFAICT) looks great!
Got some small suggestions, some are maybe not the kind of code improvements that this PR aims for.

cpp/src/io/parquet/reader_impl.hpp

cpp/src/io/parquet/reader_impl.cpp

cpp/src/io/parquet/reader_impl_preprocess.cu

cpp/src/io/parquet/reader_impl_helpers.hpp

vuule · 2022-11-08T01:11:44Z

cpp/src/io/parquet/reader_impl_helpers.hpp

+  size_t const start_row;  // TODO source index
+  size_type const source_index;


Any idea why the TODO is still here?

Also, I don't think these members should be const, it only does harm by preventing moves.

@nvdbaranec Do you have any idea why TODO?

cpp/src/io/parquet/reader_impl_helpers.hpp

vuule · 2022-11-08T01:16:11Z

cpp/src/io/parquet/reader_impl_helpers.cpp

+/**
+ * @brief Function that translates Parquet datatype to cuDF type enum
+ */
+type_id to_type_id(SchemaElement const& schema,


Feels like this and to_data_type should be SchemaElement members, but this is probably out of scope for this PR. Also, do we really need to_type_id when we have to_data_type?

to_type_id is used to interpret parquet schema information and determine the corresponding cudf type during reading. to_data_type takes a type_id and a parquet schema to determine the eventual column logical type.

hyperbolic2346

Lots to process, but looking good.

cpp/src/io/parquet/reader_impl.cpp

cpp/src/io/parquet/reader_impl_helpers.cpp

Signed-off-by: Nghia Truong <[email protected]>

ttnghia · 2022-11-10T21:53:55Z

@gpucibot merge

nvdbaranec and others added 30 commits September 23, 2022 10:59

Merge branch 'branch-22.12' into reader_preprocess_fix_and_opt

f330431

Fixed an issue with the tests: input columns cannot have unsanitary l…

eadfd63

…ists. Fixed an additional issue in the decoding where flat column types underneath structs could end up ignoring skip_rows/num_rows.

Merge branch 'branch-22.12' into reader_preprocess_fix_and_opt

c4de038

Copy parquet_reader_* into chunked_parquet_reader_*

222c9fe

Signed-off-by: Nghia Truong <[email protected]>

Modify chunked_parquet_reader_options

f49cfed

Signed-off-by: Nghia Truong <[email protected]>

Exploit inheritance to extend the options and options_builder classes

dd39804

Signed-off-by: Nghia Truong <[email protected]>

Remove unnecessary variable

81bc68f

Signed-off-by: Nghia Truong <[email protected]>

Misc

f8126be

Signed-off-by: Nghia Truong <[email protected]>

Add docs

0e7692c

Signed-off-by: Nghia Truong <[email protected]>

PR feedback changes.

9f9eeb0

Merge branch 'branch-22.12' into reader_preprocess_fix_and_opt

9b3ea62

Fixed some compile errors from merging.

d2e409a

Add chunked_parquet_reader

ed41ac1

Signed-off-by: Nghia Truong <[email protected]>

Add empty implementation

be782f2

Signed-off-by: Nghia Truong <[email protected]>

Merge branch 'branch-22.12' into parquet_reader

7908b66

Add a destructor and close

a7175c8

Signed-off-by: Nghia Truong <[email protected]>

Update docs

63a7bd6

Signed-off-by: Nghia Truong <[email protected]>

Fix comment

16c12d9

Signed-off-by: Nghia Truong <[email protected]>

Construct chunked_parquet_reader

cd85385

Signed-off-by: Nghia Truong <[email protected]>

Add comment

5944beb

Signed-off-by: Nghia Truong <[email protected]>

Rename function and implementing

7cfa72a

Signed-off-by: Nghia Truong <[email protected]>

MISC

4696bd3

Signed-off-by: Nghia Truong <[email protected]>

Bare bones implementation. Many types still not working.

99dc786

Merge branch 'branch-22.12' into parquet_reader

ad9c399

Merge branch 'chunked_reader_gpu' into parquet_reader

ecf225d

Signed-off-by: Nghia Truong <[email protected]> # Conflicts: # cpp/src/io/parquet/page_data.cu # cpp/src/io/parquet/reader_impl.cu # cpp/src/io/parquet/reader_impl.hpp

Add test

583a7ef

Signed-off-by: Nghia Truong <[email protected]>

Cleanup

b250d6f

Signed-off-by: Nghia Truong <[email protected]>

Modify docs

e7a9e3e

Signed-off-by: Nghia Truong <[email protected]>

Cleanup

811354a

Signed-off-by: Nghia Truong <[email protected]>

Extract device code out of parquet_gpu.hpp

c35aebe

Signed-off-by: Nghia Truong <[email protected]>

ttnghia changed the title ~~Refactor Parquet reader~~ Refactor Parquet reader and writer Nov 3, 2022

ttnghia changed the title ~~Refactor Parquet reader and writer~~ Refactor Parquet reader Nov 3, 2022

vyasr approved these changes Nov 4, 2022

View reviewed changes

PointKernel reviewed Nov 4, 2022

View reviewed changes

ttnghia added 4 commits November 4, 2022 09:52

Change std::vector<> const& to span<>

aa64f81

Signed-off-by: Nghia Truong <[email protected]>

Merge branch 'branch-22.12' into refactor_parquet_reader

a4547bf

Merge branch 'branch-22.12' into refactor_parquet_reader

e85b1da

Merge branch 'branch-22.12' into refactor_parquet_reader

2a37e5c

vuule requested changes Nov 8, 2022

View reviewed changes

hyperbolic2346 requested changes Nov 8, 2022

View reviewed changes

cpp/src/io/parquet/reader_impl.cpp Show resolved Hide resolved

cpp/src/io/parquet/reader_impl_helpers.cpp Show resolved Hide resolved

cpp/src/io/parquet/reader_impl_helpers.cpp Outdated Show resolved Hide resolved

ttnghia added 2 commits November 8, 2022 11:13

Remove using namespace

453a1b7

Signed-off-by: Nghia Truong <[email protected]>

Add nodiscard and fix const

bdb15be

Signed-off-by: Nghia Truong <[email protected]>

bdice mentioned this pull request Nov 8, 2022

Change cudf::detail::tdigest to cudf::tdigest::detail #12050

Merged

3 tasks

Fix input check

dcdeee8

Signed-off-by: Nghia Truong <[email protected]>

ttnghia requested review from hyperbolic2346 and vuule November 9, 2022 02:48

hyperbolic2346 approved these changes Nov 10, 2022

View reviewed changes

Merge branch 'branch-22.12' into refactor_parquet_reader

72c888f

vuule approved these changes Nov 10, 2022

View reviewed changes

rapids-bot bot merged commit 70c7b7a into rapidsai:branch-22.12 Nov 10, 2022

ttnghia deleted the refactor_parquet_reader branch November 14, 2022 18:24

vyasr added 4 - Needs Review Waiting for reviewer to review or respond and removed 4 - Needs cuIO Reviewer labels Feb 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Parquet reader #12046

Refactor Parquet reader #12046

ttnghia commented Nov 2, 2022 •

edited

Loading

vyasr left a comment

karthikeyann commented Nov 4, 2022

PointKernel left a comment

vuule left a comment

vuule Nov 8, 2022

ttnghia Nov 8, 2022

vuule Nov 8, 2022

PointKernel Nov 8, 2022

hyperbolic2346 left a comment

ttnghia commented Nov 10, 2022

		size_t const start_row; // TODO source index
		size_type const source_index;

Refactor Parquet reader #12046

Refactor Parquet reader #12046

Conversation

ttnghia commented Nov 2, 2022 • edited Loading

vyasr left a comment

Choose a reason for hiding this comment

karthikeyann commented Nov 4, 2022

PointKernel left a comment

Choose a reason for hiding this comment

vuule left a comment

Choose a reason for hiding this comment

vuule Nov 8, 2022

Choose a reason for hiding this comment

ttnghia Nov 8, 2022

Choose a reason for hiding this comment

vuule Nov 8, 2022

Choose a reason for hiding this comment

PointKernel Nov 8, 2022

Choose a reason for hiding this comment

hyperbolic2346 left a comment

Choose a reason for hiding this comment

ttnghia commented Nov 10, 2022

ttnghia commented Nov 2, 2022 •

edited

Loading