ARROW-6065: [C++][Parquet] Clean up parquet/arrow/reader.cc, reduce code duplication, improve readability by wesm · Pull Request #4963 · apache/arrow

wesm · 2019-07-29T18:06:00Z

This is strictly a refactoring PR. I'm going to start working (for a new PR) on some refactoring of the handling of schemas and nested types (which is also pretty messy in my opinion). The motivation for this is to be able to more cleanly reason about direct dictionary-decoding without having to resort to such hacks as the current FixSchemas function

Also cleans up Parquet includes using IWYU

wesm · 2019-07-29T18:13:39Z

@emkornfield I'm also trying to clean up some business logic in such a way that it will help you with implementing nested reads, though there is little enough logic related to nested data that you might want to start fresh anyway

codecov-io · 2019-07-29T22:05:06Z

Codecov Report

Merging #4963 into master will increase coverage by 1.64%.
The diff coverage is 90.65%.

@@ Coverage Diff @@## master #4963 +/- ##
==========================================
+ Coverage 87.5% 89.14% +1.64% 
==========================================
Files 998 722 -276 Lines 141869 101612 -40257 Branches 1418 0 -1418 ==========================================
- Hits 124139 90587 -33552 + Misses 17368 11025 -6343 + Partials 362 0 -362

Impacted Files	Coverage Δ
cpp/src/parquet/column_writer.h	`88.88% <ø> (ø)`	⬆️
cpp/src/parquet/file_reader.cc	`94.3% <ø> (ø)`	⬆️
cpp/src/parquet/statistics.h	`100% <ø> (ø)`	⬆️
cpp/src/parquet/bloom_filter.cc	`91.13% <ø> (ø)`	⬆️
cpp/src/parquet/encoding.cc	`93.73% <ø> (ø)`	⬆️
cpp/src/parquet/types.cc	`93.76% <ø> (ø)`	⬆️
cpp/src/parquet/statistics.cc	`87.96% <ø> (ø)`	⬆️
cpp/src/parquet/file_writer.h	`100% <ø> (ø)`	⬆️
cpp/src/parquet/schema.cc	`90.07% <ø> (ø)`	⬆️
cpp/src/parquet/arrow/writer.h	`100% <ø> (ø)`	⬆️
... and 300 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 171c3f7...cf9f5e3. Read the comment docs.

pitrou

I didn't really look at implementation details. A couple comments below.

pitrou · 2019-07-30T09:59:55Z

 { include: ["<ext/alloc_traits.h>", private, "<unordered_map>", public ] },
 { include: ["<ext/alloc_traits.h>", private, "<unordered_set>", public ] },
 { include: ["<ext/alloc_traits.h>", private, "<vector>", public ] },
+ { include: ["<bits/exception.h>", private, "<exception>", public ] },


Wow. Are we really maintaing all this by ourselves? Sounds like IWYU is not exactly user-friendy.

yeah, see another project e.g. https://github.com/apache/kudu/tree/master/build-support/iwyu/mappings
the trouble is that IWYU in some cases will find the "minimal" header to obtain certain symbols which might be something internal to the STL implementation, but that will vary on different compilers (e.g. MSVC's internal STL stuff may be different) so you need to force it in some cases to use the right "official"/"public" headers

pitrou · 2019-07-30T10:00:27Z

- const int num_columns = 20;
- const int num_rows = 1000;
+ const int num_columns = 10;
+ const int num_rows = 100;


Is this to make the test faster?

Yes, and easier to debug

pitrou · 2019-07-30T10:03:00Z


 // ----------------------------------------------------------------------
-// File reader implementation
+// FileReaderImpl forward declaration


It's not really a "forward declaration", right? Seems like an implementation to me :-)

pitrou · 2019-07-30T10:04:36Z

+ int64_t GetTotalRecords(const std::vector<int>& row_groups, int column_chunk = 0) {
+ // Can throw exception
+ int64_t records = 0;
+ for (int j = 0; j < static_cast<int>(row_groups.size()); j++) {


Use a range-based loop? for (const auto& row_group : row_groups) ...

pitrou · 2019-07-30T10:10:22Z

+ for (auto& fut : futures) {
+ Status st = fut.get();
+ if (!st.ok()) {
+ final_status = std::move(st);


Or simply final_status &= fut.get().

This is copy-paste

pitrou · 2019-07-30T10:12:33Z

-
- class PARQUET_NO_EXPORT Impl;
- std::unique_ptr<Impl> impl_;
+ virtual ~FileReader() = default;


Hmm... I'm not sure it's ok to use = default on a virtual destructor of a DLL-exported class. I think it's safer to define an empty destructor explicitly in the .cc file, though I may be mistaken.

Hm. Would be nice to know what the C++ standard says

pitrou · 2019-07-30T10:13:00Z

 };

-class PARQUET_EXPORT RowGroupReader {
+class RowGroupReader {


Do we need to PARQUET_EXPORT this class and also ColumnChunkReader?

pitrou · 2019-07-30T10:14:50Z

+
+ for (int64_t i = 0; i < length; i++) {
+ if (values[i]) {
+ ::arrow::BitUtil::SetBit(data_ptr, i);


Should this use GenerateBitsUnrolled for higher perf?

pitrou · 2019-07-30T10:17:34Z

+ if (reader->read_dictionary()) {
+ return TransferDictionary(reader, logical_value_type, out);
+ }
+ auto binary_reader = dynamic_cast<internal::BinaryRecordReader*>(reader);


Use checked_cast?

static_cast doesn't work with this type

pitrou · 2019-07-30T10:17:44Z

+Status TransferDictionary(RecordReader* reader,
+ const std::shared_ptr<DataType>& logical_value_type,
+ std::shared_ptr<ChunkedArray>* out) {
+ auto dict_reader = dynamic_cast<internal::DictionaryRecordReader*>(reader);


Use checked_cast?

static_cast doesn't work with this type

wesm · 2019-07-30T14:35:29Z

I'll address a couple comments in my follow up patch. I'm going to merge this so I'm not stacking up patches

emkornfield · 2019-07-30T19:32:21Z

@wesm thanks, I've been a little delayed with the parquet stuff and this week, I'd like to try to knock off java/c++ compatibility since it seems you still have a few more patches to do.

wesm · 2019-07-30T19:59:21Z

@emkornfield yes, I'll let you know when it's the "all clear" viz-a-viz refactoring

wesm · 2019-07-29T18:06:00Z

This is strictly a refactoring PR. I'm going to start working (for a new PR) on some refactoring of the handling of schemas and nested types (which is also pretty messy in my opinion). The motivation for this is to be able to more cleanly reason about direct dictionary-decoding without having to resort to such hacks as the current FixSchemas function

Also cleans up Parquet includes using IWYU

wesm · 2019-07-29T18:13:39Z

@emkornfield I'm also trying to clean up some business logic in such a way that it will help you with implementing nested reads, though there is little enough logic related to nested data that you might want to start fresh anyway

codecov-io · 2019-07-29T22:05:06Z

Codecov Report

Merging #4963 into master will increase coverage by 1.64%.
The diff coverage is 90.65%.

@@ Coverage Diff @@## master #4963 +/- ##
==========================================
+ Coverage 87.5% 89.14% +1.64% 
==========================================
Files 998 722 -276 Lines 141869 101612 -40257 Branches 1418 0 -1418 ==========================================
- Hits 124139 90587 -33552 + Misses 17368 11025 -6343 + Partials 362 0 -362

Impacted Files	Coverage Δ
cpp/src/parquet/column_writer.h	`88.88% <ø> (ø)`	⬆️
cpp/src/parquet/file_reader.cc	`94.3% <ø> (ø)`	⬆️
cpp/src/parquet/statistics.h	`100% <ø> (ø)`	⬆️
cpp/src/parquet/bloom_filter.cc	`91.13% <ø> (ø)`	⬆️
cpp/src/parquet/encoding.cc	`93.73% <ø> (ø)`	⬆️
cpp/src/parquet/types.cc	`93.76% <ø> (ø)`	⬆️
cpp/src/parquet/statistics.cc	`87.96% <ø> (ø)`	⬆️
cpp/src/parquet/file_writer.h	`100% <ø> (ø)`	⬆️
cpp/src/parquet/schema.cc	`90.07% <ø> (ø)`	⬆️
cpp/src/parquet/arrow/writer.h	`100% <ø> (ø)`	⬆️
... and 300 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 171c3f7...cf9f5e3. Read the comment docs.

pitrou

I didn't really look at implementation details. A couple comments below.

pitrou · 2019-07-30T09:59:55Z

 { include: ["<ext/alloc_traits.h>", private, "<unordered_map>", public ] },
 { include: ["<ext/alloc_traits.h>", private, "<unordered_set>", public ] },
 { include: ["<ext/alloc_traits.h>", private, "<vector>", public ] },
+ { include: ["<bits/exception.h>", private, "<exception>", public ] },


Wow. Are we really maintaing all this by ourselves? Sounds like IWYU is not exactly user-friendy.

yeah, see another project e.g. https://github.com/apache/kudu/tree/master/build-support/iwyu/mappings
the trouble is that IWYU in some cases will find the "minimal" header to obtain certain symbols which might be something internal to the STL implementation, but that will vary on different compilers (e.g. MSVC's internal STL stuff may be different) so you need to force it in some cases to use the right "official"/"public" headers

pitrou · 2019-07-30T10:00:27Z

- const int num_columns = 20;
- const int num_rows = 1000;
+ const int num_columns = 10;
+ const int num_rows = 100;


Is this to make the test faster?

Yes, and easier to debug

pitrou · 2019-07-30T10:03:00Z


 // ----------------------------------------------------------------------
-// File reader implementation
+// FileReaderImpl forward declaration


It's not really a "forward declaration", right? Seems like an implementation to me :-)

pitrou · 2019-07-30T10:04:36Z

+ int64_t GetTotalRecords(const std::vector<int>& row_groups, int column_chunk = 0) {
+ // Can throw exception
+ int64_t records = 0;
+ for (int j = 0; j < static_cast<int>(row_groups.size()); j++) {


Use a range-based loop? for (const auto& row_group : row_groups) ...

pitrou · 2019-07-30T10:10:22Z

+ for (auto& fut : futures) {
+ Status st = fut.get();
+ if (!st.ok()) {
+ final_status = std::move(st);


Or simply final_status &= fut.get().

This is copy-paste

pitrou · 2019-07-30T10:12:33Z

-
- class PARQUET_NO_EXPORT Impl;
- std::unique_ptr<Impl> impl_;
+ virtual ~FileReader() = default;


Hmm... I'm not sure it's ok to use = default on a virtual destructor of a DLL-exported class. I think it's safer to define an empty destructor explicitly in the .cc file, though I may be mistaken.

Hm. Would be nice to know what the C++ standard says

pitrou · 2019-07-30T10:13:00Z

 };

-class PARQUET_EXPORT RowGroupReader {
+class RowGroupReader {


Do we need to PARQUET_EXPORT this class and also ColumnChunkReader?

pitrou · 2019-07-30T10:14:50Z

+
+ for (int64_t i = 0; i < length; i++) {
+ if (values[i]) {
+ ::arrow::BitUtil::SetBit(data_ptr, i);


Should this use GenerateBitsUnrolled for higher perf?

pitrou · 2019-07-30T10:17:34Z

+ if (reader->read_dictionary()) {
+ return TransferDictionary(reader, logical_value_type, out);
+ }
+ auto binary_reader = dynamic_cast<internal::BinaryRecordReader*>(reader);


Use checked_cast?

static_cast doesn't work with this type

pitrou · 2019-07-30T10:17:44Z

+Status TransferDictionary(RecordReader* reader,
+ const std::shared_ptr<DataType>& logical_value_type,
+ std::shared_ptr<ChunkedArray>* out) {
+ auto dict_reader = dynamic_cast<internal::DictionaryRecordReader*>(reader);


Use checked_cast?

static_cast doesn't work with this type

wesm · 2019-07-30T14:35:29Z

I'll address a couple comments in my follow up patch. I'm going to merge this so I'm not stacking up patches

emkornfield · 2019-07-30T19:32:21Z

@wesm thanks, I've been a little delayed with the parquet stuff and this week, I'd like to try to knock off java/c++ compatibility since it seems you still have a few more patches to do.

wesm · 2019-07-30T19:59:21Z

@emkornfield yes, I'll let you know when it's the "all clear" viz-a-viz refactoring

wesm · 2019-07-29T18:06:00Z

This is strictly a refactoring PR. I'm going to start working (for a new PR) on some refactoring of the handling of schemas and nested types (which is also pretty messy in my opinion). The motivation for this is to be able to more cleanly reason about direct dictionary-decoding without having to resort to such hacks as the current FixSchemas function

Also cleans up Parquet includes using IWYU

wesm · 2019-07-29T18:13:39Z

@emkornfield I'm also trying to clean up some business logic in such a way that it will help you with implementing nested reads, though there is little enough logic related to nested data that you might want to start fresh anyway

codecov-io · 2019-07-29T22:05:06Z

Codecov Report

Merging #4963 into master will increase coverage by 1.64%.
The diff coverage is 90.65%.

@@ Coverage Diff @@## master #4963 +/- ##
==========================================
+ Coverage 87.5% 89.14% +1.64% 
==========================================
Files 998 722 -276 Lines 141869 101612 -40257 Branches 1418 0 -1418 ==========================================
- Hits 124139 90587 -33552 + Misses 17368 11025 -6343 + Partials 362 0 -362

Impacted Files	Coverage Δ
cpp/src/parquet/column_writer.h	`88.88% <ø> (ø)`	⬆️
cpp/src/parquet/file_reader.cc	`94.3% <ø> (ø)`	⬆️
cpp/src/parquet/statistics.h	`100% <ø> (ø)`	⬆️
cpp/src/parquet/bloom_filter.cc	`91.13% <ø> (ø)`	⬆️
cpp/src/parquet/encoding.cc	`93.73% <ø> (ø)`	⬆️
cpp/src/parquet/types.cc	`93.76% <ø> (ø)`	⬆️
cpp/src/parquet/statistics.cc	`87.96% <ø> (ø)`	⬆️
cpp/src/parquet/file_writer.h	`100% <ø> (ø)`	⬆️
cpp/src/parquet/schema.cc	`90.07% <ø> (ø)`	⬆️
cpp/src/parquet/arrow/writer.h	`100% <ø> (ø)`	⬆️
... and 300 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 171c3f7...cf9f5e3. Read the comment docs.

pitrou

I didn't really look at implementation details. A couple comments below.

pitrou · 2019-07-30T09:59:55Z

 { include: ["<ext/alloc_traits.h>", private, "<unordered_map>", public ] },
 { include: ["<ext/alloc_traits.h>", private, "<unordered_set>", public ] },
 { include: ["<ext/alloc_traits.h>", private, "<vector>", public ] },
+ { include: ["<bits/exception.h>", private, "<exception>", public ] },


Wow. Are we really maintaing all this by ourselves? Sounds like IWYU is not exactly user-friendy.

yeah, see another project e.g. https://github.com/apache/kudu/tree/master/build-support/iwyu/mappings
the trouble is that IWYU in some cases will find the "minimal" header to obtain certain symbols which might be something internal to the STL implementation, but that will vary on different compilers (e.g. MSVC's internal STL stuff may be different) so you need to force it in some cases to use the right "official"/"public" headers

pitrou · 2019-07-30T10:00:27Z

- const int num_columns = 20;
- const int num_rows = 1000;
+ const int num_columns = 10;
+ const int num_rows = 100;


Is this to make the test faster?

Yes, and easier to debug

pitrou · 2019-07-30T10:03:00Z


 // ----------------------------------------------------------------------
-// File reader implementation
+// FileReaderImpl forward declaration


It's not really a "forward declaration", right? Seems like an implementation to me :-)

pitrou · 2019-07-30T10:04:36Z

+ int64_t GetTotalRecords(const std::vector<int>& row_groups, int column_chunk = 0) {
+ // Can throw exception
+ int64_t records = 0;
+ for (int j = 0; j < static_cast<int>(row_groups.size()); j++) {


Use a range-based loop? for (const auto& row_group : row_groups) ...

pitrou · 2019-07-30T10:10:22Z

+ for (auto& fut : futures) {
+ Status st = fut.get();
+ if (!st.ok()) {
+ final_status = std::move(st);


Or simply final_status &= fut.get().

This is copy-paste

pitrou · 2019-07-30T10:12:33Z

-
- class PARQUET_NO_EXPORT Impl;
- std::unique_ptr<Impl> impl_;
+ virtual ~FileReader() = default;


Hmm... I'm not sure it's ok to use = default on a virtual destructor of a DLL-exported class. I think it's safer to define an empty destructor explicitly in the .cc file, though I may be mistaken.

Hm. Would be nice to know what the C++ standard says

pitrou · 2019-07-30T10:13:00Z

 };

-class PARQUET_EXPORT RowGroupReader {
+class RowGroupReader {


Do we need to PARQUET_EXPORT this class and also ColumnChunkReader?

pitrou · 2019-07-30T10:14:50Z

+
+ for (int64_t i = 0; i < length; i++) {
+ if (values[i]) {
+ ::arrow::BitUtil::SetBit(data_ptr, i);


Should this use GenerateBitsUnrolled for higher perf?

pitrou · 2019-07-30T10:17:34Z

+ if (reader->read_dictionary()) {
+ return TransferDictionary(reader, logical_value_type, out);
+ }
+ auto binary_reader = dynamic_cast<internal::BinaryRecordReader*>(reader);


Use checked_cast?

static_cast doesn't work with this type

pitrou · 2019-07-30T10:17:44Z

+Status TransferDictionary(RecordReader* reader,
+ const std::shared_ptr<DataType>& logical_value_type,
+ std::shared_ptr<ChunkedArray>* out) {
+ auto dict_reader = dynamic_cast<internal::DictionaryRecordReader*>(reader);


Use checked_cast?

static_cast doesn't work with this type

wesm · 2019-07-30T14:35:29Z

I'll address a couple comments in my follow up patch. I'm going to merge this so I'm not stacking up patches

emkornfield · 2019-07-30T19:32:21Z

@wesm thanks, I've been a little delayed with the parquet stuff and this week, I'd like to try to knock off java/c++ compatibility since it seems you still have a few more patches to do.

wesm · 2019-07-30T19:59:21Z

@emkornfield yes, I'll let you know when it's the "all clear" viz-a-viz refactoring

wesm · 2019-07-29T18:06:00Z

This is strictly a refactoring PR. I'm going to start working (for a new PR) on some refactoring of the handling of schemas and nested types (which is also pretty messy in my opinion). The motivation for this is to be able to more cleanly reason about direct dictionary-decoding without having to resort to such hacks as the current FixSchemas function

Also cleans up Parquet includes using IWYU

wesm · 2019-07-29T18:13:39Z

@emkornfield I'm also trying to clean up some business logic in such a way that it will help you with implementing nested reads, though there is little enough logic related to nested data that you might want to start fresh anyway

codecov-io · 2019-07-29T22:05:06Z

Codecov Report

Merging #4963 into master will increase coverage by 1.64%.
The diff coverage is 90.65%.

@@ Coverage Diff @@## master #4963 +/- ##
==========================================
+ Coverage 87.5% 89.14% +1.64% 
==========================================
Files 998 722 -276 Lines 141869 101612 -40257 Branches 1418 0 -1418 ==========================================
- Hits 124139 90587 -33552 + Misses 17368 11025 -6343 + Partials 362 0 -362

Impacted Files	Coverage Δ
cpp/src/parquet/column_writer.h	`88.88% <ø> (ø)`	⬆️
cpp/src/parquet/file_reader.cc	`94.3% <ø> (ø)`	⬆️
cpp/src/parquet/statistics.h	`100% <ø> (ø)`	⬆️
cpp/src/parquet/bloom_filter.cc	`91.13% <ø> (ø)`	⬆️
cpp/src/parquet/encoding.cc	`93.73% <ø> (ø)`	⬆️
cpp/src/parquet/types.cc	`93.76% <ø> (ø)`	⬆️
cpp/src/parquet/statistics.cc	`87.96% <ø> (ø)`	⬆️
cpp/src/parquet/file_writer.h	`100% <ø> (ø)`	⬆️
cpp/src/parquet/schema.cc	`90.07% <ø> (ø)`	⬆️
cpp/src/parquet/arrow/writer.h	`100% <ø> (ø)`	⬆️
... and 300 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 171c3f7...cf9f5e3. Read the comment docs.

pitrou

I didn't really look at implementation details. A couple comments below.

pitrou · 2019-07-30T09:59:55Z

 { include: ["<ext/alloc_traits.h>", private, "<unordered_map>", public ] },
 { include: ["<ext/alloc_traits.h>", private, "<unordered_set>", public ] },
 { include: ["<ext/alloc_traits.h>", private, "<vector>", public ] },
+ { include: ["<bits/exception.h>", private, "<exception>", public ] },


Wow. Are we really maintaing all this by ourselves? Sounds like IWYU is not exactly user-friendy.

yeah, see another project e.g. https://github.com/apache/kudu/tree/master/build-support/iwyu/mappings
the trouble is that IWYU in some cases will find the "minimal" header to obtain certain symbols which might be something internal to the STL implementation, but that will vary on different compilers (e.g. MSVC's internal STL stuff may be different) so you need to force it in some cases to use the right "official"/"public" headers

pitrou · 2019-07-30T10:00:27Z

- const int num_columns = 20;
- const int num_rows = 1000;
+ const int num_columns = 10;
+ const int num_rows = 100;


Is this to make the test faster?

Yes, and easier to debug

pitrou · 2019-07-30T10:03:00Z


 // ----------------------------------------------------------------------
-// File reader implementation
+// FileReaderImpl forward declaration


It's not really a "forward declaration", right? Seems like an implementation to me :-)

pitrou · 2019-07-30T10:04:36Z

+ int64_t GetTotalRecords(const std::vector<int>& row_groups, int column_chunk = 0) {
+ // Can throw exception
+ int64_t records = 0;
+ for (int j = 0; j < static_cast<int>(row_groups.size()); j++) {


Use a range-based loop? for (const auto& row_group : row_groups) ...

pitrou · 2019-07-30T10:10:22Z

+ for (auto& fut : futures) {
+ Status st = fut.get();
+ if (!st.ok()) {
+ final_status = std::move(st);


Or simply final_status &= fut.get().

This is copy-paste

pitrou · 2019-07-30T10:12:33Z

-
- class PARQUET_NO_EXPORT Impl;
- std::unique_ptr<Impl> impl_;
+ virtual ~FileReader() = default;


Hmm... I'm not sure it's ok to use = default on a virtual destructor of a DLL-exported class. I think it's safer to define an empty destructor explicitly in the .cc file, though I may be mistaken.

Hm. Would be nice to know what the C++ standard says

pitrou · 2019-07-30T10:13:00Z

 };

-class PARQUET_EXPORT RowGroupReader {
+class RowGroupReader {


Do we need to PARQUET_EXPORT this class and also ColumnChunkReader?

pitrou · 2019-07-30T10:14:50Z

+
+ for (int64_t i = 0; i < length; i++) {
+ if (values[i]) {
+ ::arrow::BitUtil::SetBit(data_ptr, i);


Should this use GenerateBitsUnrolled for higher perf?

pitrou · 2019-07-30T10:17:34Z

+ if (reader->read_dictionary()) {
+ return TransferDictionary(reader, logical_value_type, out);
+ }
+ auto binary_reader = dynamic_cast<internal::BinaryRecordReader*>(reader);


Use checked_cast?

static_cast doesn't work with this type

pitrou · 2019-07-30T10:17:44Z

+Status TransferDictionary(RecordReader* reader,
+ const std::shared_ptr<DataType>& logical_value_type,
+ std::shared_ptr<ChunkedArray>* out) {
+ auto dict_reader = dynamic_cast<internal::DictionaryRecordReader*>(reader);


Use checked_cast?

static_cast doesn't work with this type

wesm · 2019-07-30T14:35:29Z

I'll address a couple comments in my follow up patch. I'm going to merge this so I'm not stacking up patches

emkornfield · 2019-07-30T19:32:21Z

@wesm thanks, I've been a little delayed with the parquet stuff and this week, I'd like to try to knock off java/c++ compatibility since it seems you still have a few more patches to do.

wesm · 2019-07-30T19:59:21Z

@emkornfield yes, I'll let you know when it's the "all clear" viz-a-viz refactoring

wesm · 2019-07-29T18:06:00Z

This is strictly a refactoring PR. I'm going to start working (for a new PR) on some refactoring of the handling of schemas and nested types (which is also pretty messy in my opinion). The motivation for this is to be able to more cleanly reason about direct dictionary-decoding without having to resort to such hacks as the current FixSchemas function

Also cleans up Parquet includes using IWYU

wesm · 2019-07-29T18:13:39Z

@emkornfield I'm also trying to clean up some business logic in such a way that it will help you with implementing nested reads, though there is little enough logic related to nested data that you might want to start fresh anyway

codecov-io · 2019-07-29T22:05:06Z

Codecov Report

Merging #4963 into master will increase coverage by 1.64%.
The diff coverage is 90.65%.

@@ Coverage Diff @@## master #4963 +/- ##
==========================================
+ Coverage 87.5% 89.14% +1.64% 
==========================================
Files 998 722 -276 Lines 141869 101612 -40257 Branches 1418 0 -1418 ==========================================
- Hits 124139 90587 -33552 + Misses 17368 11025 -6343 + Partials 362 0 -362

Impacted Files	Coverage Δ
cpp/src/parquet/column_writer.h	`88.88% <ø> (ø)`	⬆️
cpp/src/parquet/file_reader.cc	`94.3% <ø> (ø)`	⬆️
cpp/src/parquet/statistics.h	`100% <ø> (ø)`	⬆️
cpp/src/parquet/bloom_filter.cc	`91.13% <ø> (ø)`	⬆️
cpp/src/parquet/encoding.cc	`93.73% <ø> (ø)`	⬆️
cpp/src/parquet/types.cc	`93.76% <ø> (ø)`	⬆️
cpp/src/parquet/statistics.cc	`87.96% <ø> (ø)`	⬆️
cpp/src/parquet/file_writer.h	`100% <ø> (ø)`	⬆️
cpp/src/parquet/schema.cc	`90.07% <ø> (ø)`	⬆️
cpp/src/parquet/arrow/writer.h	`100% <ø> (ø)`	⬆️
... and 300 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 171c3f7...cf9f5e3. Read the comment docs.

pitrou

I didn't really look at implementation details. A couple comments below.

pitrou · 2019-07-30T09:59:55Z

 { include: ["<ext/alloc_traits.h>", private, "<unordered_map>", public ] },
 { include: ["<ext/alloc_traits.h>", private, "<unordered_set>", public ] },
 { include: ["<ext/alloc_traits.h>", private, "<vector>", public ] },
+ { include: ["<bits/exception.h>", private, "<exception>", public ] },


Wow. Are we really maintaing all this by ourselves? Sounds like IWYU is not exactly user-friendy.

yeah, see another project e.g. https://github.com/apache/kudu/tree/master/build-support/iwyu/mappings
the trouble is that IWYU in some cases will find the "minimal" header to obtain certain symbols which might be something internal to the STL implementation, but that will vary on different compilers (e.g. MSVC's internal STL stuff may be different) so you need to force it in some cases to use the right "official"/"public" headers

pitrou · 2019-07-30T10:00:27Z

- const int num_columns = 20;
- const int num_rows = 1000;
+ const int num_columns = 10;
+ const int num_rows = 100;


Is this to make the test faster?

Yes, and easier to debug

pitrou · 2019-07-30T10:03:00Z


 // ----------------------------------------------------------------------
-// File reader implementation
+// FileReaderImpl forward declaration


It's not really a "forward declaration", right? Seems like an implementation to me :-)

pitrou · 2019-07-30T10:04:36Z

+ int64_t GetTotalRecords(const std::vector<int>& row_groups, int column_chunk = 0) {
+ // Can throw exception
+ int64_t records = 0;
+ for (int j = 0; j < static_cast<int>(row_groups.size()); j++) {


Use a range-based loop? for (const auto& row_group : row_groups) ...

pitrou · 2019-07-30T10:10:22Z

+ for (auto& fut : futures) {
+ Status st = fut.get();
+ if (!st.ok()) {
+ final_status = std::move(st);


Or simply final_status &= fut.get().

This is copy-paste

pitrou · 2019-07-30T10:12:33Z

-
- class PARQUET_NO_EXPORT Impl;
- std::unique_ptr<Impl> impl_;
+ virtual ~FileReader() = default;


Hmm... I'm not sure it's ok to use = default on a virtual destructor of a DLL-exported class. I think it's safer to define an empty destructor explicitly in the .cc file, though I may be mistaken.

Hm. Would be nice to know what the C++ standard says

pitrou · 2019-07-30T10:13:00Z

 };

-class PARQUET_EXPORT RowGroupReader {
+class RowGroupReader {


Do we need to PARQUET_EXPORT this class and also ColumnChunkReader?

pitrou · 2019-07-30T10:14:50Z

+
+ for (int64_t i = 0; i < length; i++) {
+ if (values[i]) {
+ ::arrow::BitUtil::SetBit(data_ptr, i);


Should this use GenerateBitsUnrolled for higher perf?

pitrou · 2019-07-30T10:17:34Z

+ if (reader->read_dictionary()) {
+ return TransferDictionary(reader, logical_value_type, out);
+ }
+ auto binary_reader = dynamic_cast<internal::BinaryRecordReader*>(reader);


Use checked_cast?

static_cast doesn't work with this type

pitrou · 2019-07-30T10:17:44Z

+Status TransferDictionary(RecordReader* reader,
+ const std::shared_ptr<DataType>& logical_value_type,
+ std::shared_ptr<ChunkedArray>* out) {
+ auto dict_reader = dynamic_cast<internal::DictionaryRecordReader*>(reader);


Use checked_cast?

static_cast doesn't work with this type

wesm · 2019-07-30T14:35:29Z

I'll address a couple comments in my follow up patch. I'm going to merge this so I'm not stacking up patches

emkornfield · 2019-07-30T19:32:21Z

@wesm thanks, I've been a little delayed with the parquet stuff and this week, I'd like to try to knock off java/c++ compatibility since it seems you still have a few more patches to do.

wesm · 2019-07-30T19:59:21Z

@emkornfield yes, I'll let you know when it's the "all clear" viz-a-viz refactoring

wesm · 2019-07-29T18:06:00Z

This is strictly a refactoring PR. I'm going to start working (for a new PR) on some refactoring of the handling of schemas and nested types (which is also pretty messy in my opinion). The motivation for this is to be able to more cleanly reason about direct dictionary-decoding without having to resort to such hacks as the current FixSchemas function

Also cleans up Parquet includes using IWYU

wesm · 2019-07-29T18:13:39Z

@emkornfield I'm also trying to clean up some business logic in such a way that it will help you with implementing nested reads, though there is little enough logic related to nested data that you might want to start fresh anyway

codecov-io · 2019-07-29T22:05:06Z

Codecov Report

Merging #4963 into master will increase coverage by 1.64%.
The diff coverage is 90.65%.

@@ Coverage Diff @@## master #4963 +/- ##
==========================================
+ Coverage 87.5% 89.14% +1.64% 
==========================================
Files 998 722 -276 Lines 141869 101612 -40257 Branches 1418 0 -1418 ==========================================
- Hits 124139 90587 -33552 + Misses 17368 11025 -6343 + Partials 362 0 -362

Impacted Files	Coverage Δ
cpp/src/parquet/column_writer.h	`88.88% <ø> (ø)`	⬆️
cpp/src/parquet/file_reader.cc	`94.3% <ø> (ø)`	⬆️
cpp/src/parquet/statistics.h	`100% <ø> (ø)`	⬆️
cpp/src/parquet/bloom_filter.cc	`91.13% <ø> (ø)`	⬆️
cpp/src/parquet/encoding.cc	`93.73% <ø> (ø)`	⬆️
cpp/src/parquet/types.cc	`93.76% <ø> (ø)`	⬆️
cpp/src/parquet/statistics.cc	`87.96% <ø> (ø)`	⬆️
cpp/src/parquet/file_writer.h	`100% <ø> (ø)`	⬆️
cpp/src/parquet/schema.cc	`90.07% <ø> (ø)`	⬆️
cpp/src/parquet/arrow/writer.h	`100% <ø> (ø)`	⬆️
... and 300 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 171c3f7...cf9f5e3. Read the comment docs.

pitrou

I didn't really look at implementation details. A couple comments below.

pitrou · 2019-07-30T09:59:55Z

 { include: ["<ext/alloc_traits.h>", private, "<unordered_map>", public ] },
 { include: ["<ext/alloc_traits.h>", private, "<unordered_set>", public ] },
 { include: ["<ext/alloc_traits.h>", private, "<vector>", public ] },
+ { include: ["<bits/exception.h>", private, "<exception>", public ] },


Wow. Are we really maintaing all this by ourselves? Sounds like IWYU is not exactly user-friendy.

yeah, see another project e.g. https://github.com/apache/kudu/tree/master/build-support/iwyu/mappings
the trouble is that IWYU in some cases will find the "minimal" header to obtain certain symbols which might be something internal to the STL implementation, but that will vary on different compilers (e.g. MSVC's internal STL stuff may be different) so you need to force it in some cases to use the right "official"/"public" headers

pitrou · 2019-07-30T10:00:27Z

- const int num_columns = 20;
- const int num_rows = 1000;
+ const int num_columns = 10;
+ const int num_rows = 100;


Is this to make the test faster?

Yes, and easier to debug

pitrou · 2019-07-30T10:03:00Z


 // ----------------------------------------------------------------------
-// File reader implementation
+// FileReaderImpl forward declaration


It's not really a "forward declaration", right? Seems like an implementation to me :-)

pitrou · 2019-07-30T10:04:36Z

+ int64_t GetTotalRecords(const std::vector<int>& row_groups, int column_chunk = 0) {
+ // Can throw exception
+ int64_t records = 0;
+ for (int j = 0; j < static_cast<int>(row_groups.size()); j++) {


Use a range-based loop? for (const auto& row_group : row_groups) ...

pitrou · 2019-07-30T10:10:22Z

+ for (auto& fut : futures) {
+ Status st = fut.get();
+ if (!st.ok()) {
+ final_status = std::move(st);


Or simply final_status &= fut.get().

This is copy-paste

pitrou · 2019-07-30T10:12:33Z

-
- class PARQUET_NO_EXPORT Impl;
- std::unique_ptr<Impl> impl_;
+ virtual ~FileReader() = default;


Hmm... I'm not sure it's ok to use = default on a virtual destructor of a DLL-exported class. I think it's safer to define an empty destructor explicitly in the .cc file, though I may be mistaken.

Hm. Would be nice to know what the C++ standard says

pitrou · 2019-07-30T10:13:00Z

 };

-class PARQUET_EXPORT RowGroupReader {
+class RowGroupReader {


Do we need to PARQUET_EXPORT this class and also ColumnChunkReader?

pitrou · 2019-07-30T10:14:50Z

+
+ for (int64_t i = 0; i < length; i++) {
+ if (values[i]) {
+ ::arrow::BitUtil::SetBit(data_ptr, i);


Should this use GenerateBitsUnrolled for higher perf?

pitrou · 2019-07-30T10:17:34Z

+ if (reader->read_dictionary()) {
+ return TransferDictionary(reader, logical_value_type, out);
+ }
+ auto binary_reader = dynamic_cast<internal::BinaryRecordReader*>(reader);


Use checked_cast?

static_cast doesn't work with this type

pitrou · 2019-07-30T10:17:44Z

+Status TransferDictionary(RecordReader* reader,
+ const std::shared_ptr<DataType>& logical_value_type,
+ std::shared_ptr<ChunkedArray>* out) {
+ auto dict_reader = dynamic_cast<internal::DictionaryRecordReader*>(reader);


Use checked_cast?

static_cast doesn't work with this type

wesm · 2019-07-30T14:35:29Z

I'll address a couple comments in my follow up patch. I'm going to merge this so I'm not stacking up patches

emkornfield · 2019-07-30T19:32:21Z

@wesm thanks, I've been a little delayed with the parquet stuff and this week, I'd like to try to knock off java/c++ compatibility since it seems you still have a few more patches to do.

wesm · 2019-07-30T19:59:21Z

@emkornfield yes, I'll let you know when it's the "all clear" viz-a-viz refactoring

wesm · 2019-07-29T18:06:00Z

This is strictly a refactoring PR. I'm going to start working (for a new PR) on some refactoring of the handling of schemas and nested types (which is also pretty messy in my opinion). The motivation for this is to be able to more cleanly reason about direct dictionary-decoding without having to resort to such hacks as the current FixSchemas function

Also cleans up Parquet includes using IWYU

wesm · 2019-07-29T18:13:39Z

@emkornfield I'm also trying to clean up some business logic in such a way that it will help you with implementing nested reads, though there is little enough logic related to nested data that you might want to start fresh anyway

codecov-io · 2019-07-29T22:05:06Z

Codecov Report

Merging #4963 into master will increase coverage by 1.64%.
The diff coverage is 90.65%.

@@ Coverage Diff @@## master #4963 +/- ##
==========================================
+ Coverage 87.5% 89.14% +1.64% 
==========================================
Files 998 722 -276 Lines 141869 101612 -40257 Branches 1418 0 -1418 ==========================================
- Hits 124139 90587 -33552 + Misses 17368 11025 -6343 + Partials 362 0 -362

Impacted Files	Coverage Δ
cpp/src/parquet/column_writer.h	`88.88% <ø> (ø)`	⬆️
cpp/src/parquet/file_reader.cc	`94.3% <ø> (ø)`	⬆️
cpp/src/parquet/statistics.h	`100% <ø> (ø)`	⬆️
cpp/src/parquet/bloom_filter.cc	`91.13% <ø> (ø)`	⬆️
cpp/src/parquet/encoding.cc	`93.73% <ø> (ø)`	⬆️
cpp/src/parquet/types.cc	`93.76% <ø> (ø)`	⬆️
cpp/src/parquet/statistics.cc	`87.96% <ø> (ø)`	⬆️
cpp/src/parquet/file_writer.h	`100% <ø> (ø)`	⬆️
cpp/src/parquet/schema.cc	`90.07% <ø> (ø)`	⬆️
cpp/src/parquet/arrow/writer.h	`100% <ø> (ø)`	⬆️
... and 300 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 171c3f7...cf9f5e3. Read the comment docs.

pitrou

I didn't really look at implementation details. A couple comments below.

pitrou · 2019-07-30T09:59:55Z

 { include: ["<ext/alloc_traits.h>", private, "<unordered_map>", public ] },
 { include: ["<ext/alloc_traits.h>", private, "<unordered_set>", public ] },
 { include: ["<ext/alloc_traits.h>", private, "<vector>", public ] },
+ { include: ["<bits/exception.h>", private, "<exception>", public ] },


Wow. Are we really maintaing all this by ourselves? Sounds like IWYU is not exactly user-friendy.

yeah, see another project e.g. https://github.com/apache/kudu/tree/master/build-support/iwyu/mappings
the trouble is that IWYU in some cases will find the "minimal" header to obtain certain symbols which might be something internal to the STL implementation, but that will vary on different compilers (e.g. MSVC's internal STL stuff may be different) so you need to force it in some cases to use the right "official"/"public" headers

pitrou · 2019-07-30T10:00:27Z

- const int num_columns = 20;
- const int num_rows = 1000;
+ const int num_columns = 10;
+ const int num_rows = 100;


Is this to make the test faster?

Yes, and easier to debug

pitrou · 2019-07-30T10:03:00Z


 // ----------------------------------------------------------------------
-// File reader implementation
+// FileReaderImpl forward declaration


It's not really a "forward declaration", right? Seems like an implementation to me :-)

pitrou · 2019-07-30T10:04:36Z

+ int64_t GetTotalRecords(const std::vector<int>& row_groups, int column_chunk = 0) {
+ // Can throw exception
+ int64_t records = 0;
+ for (int j = 0; j < static_cast<int>(row_groups.size()); j++) {


Use a range-based loop? for (const auto& row_group : row_groups) ...

pitrou · 2019-07-30T10:10:22Z

+ for (auto& fut : futures) {
+ Status st = fut.get();
+ if (!st.ok()) {
+ final_status = std::move(st);


Or simply final_status &= fut.get().

This is copy-paste

pitrou · 2019-07-30T10:12:33Z

-
- class PARQUET_NO_EXPORT Impl;
- std::unique_ptr<Impl> impl_;
+ virtual ~FileReader() = default;


Hmm... I'm not sure it's ok to use = default on a virtual destructor of a DLL-exported class. I think it's safer to define an empty destructor explicitly in the .cc file, though I may be mistaken.

Hm. Would be nice to know what the C++ standard says

pitrou · 2019-07-30T10:13:00Z

 };

-class PARQUET_EXPORT RowGroupReader {
+class RowGroupReader {


Do we need to PARQUET_EXPORT this class and also ColumnChunkReader?

pitrou · 2019-07-30T10:14:50Z

+
+ for (int64_t i = 0; i < length; i++) {
+ if (values[i]) {
+ ::arrow::BitUtil::SetBit(data_ptr, i);


Should this use GenerateBitsUnrolled for higher perf?

pitrou · 2019-07-30T10:17:34Z

+ if (reader->read_dictionary()) {
+ return TransferDictionary(reader, logical_value_type, out);
+ }
+ auto binary_reader = dynamic_cast<internal::BinaryRecordReader*>(reader);


Use checked_cast?

static_cast doesn't work with this type

pitrou · 2019-07-30T10:17:44Z

+Status TransferDictionary(RecordReader* reader,
+ const std::shared_ptr<DataType>& logical_value_type,
+ std::shared_ptr<ChunkedArray>* out) {
+ auto dict_reader = dynamic_cast<internal::DictionaryRecordReader*>(reader);


Use checked_cast?

static_cast doesn't work with this type

wesm · 2019-07-30T14:35:29Z

I'll address a couple comments in my follow up patch. I'm going to merge this so I'm not stacking up patches

emkornfield · 2019-07-30T19:32:21Z

@wesm thanks, I've been a little delayed with the parquet stuff and this week, I'd like to try to knock off java/c++ compatibility since it seems you still have a few more patches to do.

wesm · 2019-07-30T19:59:21Z

@emkornfield yes, I'll let you know when it's the "all clear" viz-a-viz refactoring

wesm · 2019-07-29T18:06:00Z

This is strictly a refactoring PR. I'm going to start working (for a new PR) on some refactoring of the handling of schemas and nested types (which is also pretty messy in my opinion). The motivation for this is to be able to more cleanly reason about direct dictionary-decoding without having to resort to such hacks as the current FixSchemas function

Also cleans up Parquet includes using IWYU

wesm · 2019-07-29T18:13:39Z

@emkornfield I'm also trying to clean up some business logic in such a way that it will help you with implementing nested reads, though there is little enough logic related to nested data that you might want to start fresh anyway

codecov-io · 2019-07-29T22:05:06Z

Codecov Report

Merging #4963 into master will increase coverage by 1.64%.
The diff coverage is 90.65%.

@@ Coverage Diff @@## master #4963 +/- ##
==========================================
+ Coverage 87.5% 89.14% +1.64% 
==========================================
Files 998 722 -276 Lines 141869 101612 -40257 Branches 1418 0 -1418 ==========================================
- Hits 124139 90587 -33552 + Misses 17368 11025 -6343 + Partials 362 0 -362

Impacted Files	Coverage Δ
cpp/src/parquet/column_writer.h	`88.88% <ø> (ø)`	⬆️
cpp/src/parquet/file_reader.cc	`94.3% <ø> (ø)`	⬆️
cpp/src/parquet/statistics.h	`100% <ø> (ø)`	⬆️
cpp/src/parquet/bloom_filter.cc	`91.13% <ø> (ø)`	⬆️
cpp/src/parquet/encoding.cc	`93.73% <ø> (ø)`	⬆️
cpp/src/parquet/types.cc	`93.76% <ø> (ø)`	⬆️
cpp/src/parquet/statistics.cc	`87.96% <ø> (ø)`	⬆️
cpp/src/parquet/file_writer.h	`100% <ø> (ø)`	⬆️
cpp/src/parquet/schema.cc	`90.07% <ø> (ø)`	⬆️
cpp/src/parquet/arrow/writer.h	`100% <ø> (ø)`	⬆️
... and 300 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 171c3f7...cf9f5e3. Read the comment docs.

pitrou

I didn't really look at implementation details. A couple comments below.

pitrou · 2019-07-30T09:59:55Z

 { include: ["<ext/alloc_traits.h>", private, "<unordered_map>", public ] },
 { include: ["<ext/alloc_traits.h>", private, "<unordered_set>", public ] },
 { include: ["<ext/alloc_traits.h>", private, "<vector>", public ] },
+ { include: ["<bits/exception.h>", private, "<exception>", public ] },


Wow. Are we really maintaing all this by ourselves? Sounds like IWYU is not exactly user-friendy.

yeah, see another project e.g. https://github.com/apache/kudu/tree/master/build-support/iwyu/mappings
the trouble is that IWYU in some cases will find the "minimal" header to obtain certain symbols which might be something internal to the STL implementation, but that will vary on different compilers (e.g. MSVC's internal STL stuff may be different) so you need to force it in some cases to use the right "official"/"public" headers

pitrou · 2019-07-30T10:00:27Z

- const int num_columns = 20;
- const int num_rows = 1000;
+ const int num_columns = 10;
+ const int num_rows = 100;


Is this to make the test faster?

Yes, and easier to debug

pitrou · 2019-07-30T10:03:00Z


 // ----------------------------------------------------------------------
-// File reader implementation
+// FileReaderImpl forward declaration


It's not really a "forward declaration", right? Seems like an implementation to me :-)

pitrou · 2019-07-30T10:04:36Z

+ int64_t GetTotalRecords(const std::vector<int>& row_groups, int column_chunk = 0) {
+ // Can throw exception
+ int64_t records = 0;
+ for (int j = 0; j < static_cast<int>(row_groups.size()); j++) {


Use a range-based loop? for (const auto& row_group : row_groups) ...

pitrou · 2019-07-30T10:10:22Z

+ for (auto& fut : futures) {
+ Status st = fut.get();
+ if (!st.ok()) {
+ final_status = std::move(st);


Or simply final_status &= fut.get().

This is copy-paste

pitrou · 2019-07-30T10:12:33Z

-
- class PARQUET_NO_EXPORT Impl;
- std::unique_ptr<Impl> impl_;
+ virtual ~FileReader() = default;


Hmm... I'm not sure it's ok to use = default on a virtual destructor of a DLL-exported class. I think it's safer to define an empty destructor explicitly in the .cc file, though I may be mistaken.

Hm. Would be nice to know what the C++ standard says

pitrou · 2019-07-30T10:13:00Z

 };

-class PARQUET_EXPORT RowGroupReader {
+class RowGroupReader {


Do we need to PARQUET_EXPORT this class and also ColumnChunkReader?

pitrou · 2019-07-30T10:14:50Z

+
+ for (int64_t i = 0; i < length; i++) {
+ if (values[i]) {
+ ::arrow::BitUtil::SetBit(data_ptr, i);


Should this use GenerateBitsUnrolled for higher perf?

pitrou · 2019-07-30T10:17:34Z

+ if (reader->read_dictionary()) {
+ return TransferDictionary(reader, logical_value_type, out);
+ }
+ auto binary_reader = dynamic_cast<internal::BinaryRecordReader*>(reader);


Use checked_cast?

static_cast doesn't work with this type

pitrou · 2019-07-30T10:17:44Z

+Status TransferDictionary(RecordReader* reader,
+ const std::shared_ptr<DataType>& logical_value_type,
+ std::shared_ptr<ChunkedArray>* out) {
+ auto dict_reader = dynamic_cast<internal::DictionaryRecordReader*>(reader);


Use checked_cast?

static_cast doesn't work with this type

wesm · 2019-07-30T14:35:29Z

I'll address a couple comments in my follow up patch. I'm going to merge this so I'm not stacking up patches

emkornfield · 2019-07-30T19:32:21Z

@wesm thanks, I've been a little delayed with the parquet stuff and this week, I'd like to try to knock off java/c++ compatibility since it seems you still have a few more patches to do.

wesm · 2019-07-30T19:59:21Z

@emkornfield yes, I'll let you know when it's the "all clear" viz-a-viz refactoring

wesm added 4 commits July 29, 2019 09:48

More refactoring progress
02153bf

Refactor complete, tests passing again. IWYU pass
174f046

More cleaning
9d9a30b

Add missing API include, fix Python bindings
cf9f5e3

pitrou reviewed Jul 30, 2019
View reviewed changes

wesm closed this in dbd93e3Jul 30, 2019

wesm deleted the parquet-arrow-read-refactor branch July 30, 2019 20:00

asfimport mentioned this pull request Aug 1, 2019
[C++] Reorganize parquet/arrow/reader.cc, remove code duplication, improve readability #22466
Closed

wesm added 4 commits July 29, 2019 09:48

More refactoring progress
02153bf

Refactor complete, tests passing again. IWYU pass
174f046

More cleaning
9d9a30b

Add missing API include, fix Python bindings
cf9f5e3

pitrou reviewed Jul 30, 2019
View reviewed changes

wesm closed this in dbd93e3Jul 30, 2019

wesm deleted the parquet-arrow-read-refactor branch July 30, 2019 20:00

asfimport mentioned this pull request Aug 1, 2019
[C++] Reorganize parquet/arrow/reader.cc, remove code duplication, improve readability #22466
Closed

wesm added 4 commits July 29, 2019 09:48

More refactoring progress
02153bf

Refactor complete, tests passing again. IWYU pass
174f046

More cleaning
9d9a30b

Add missing API include, fix Python bindings
cf9f5e3

pitrou reviewed Jul 30, 2019
View reviewed changes

wesm closed this in dbd93e3Jul 30, 2019

wesm deleted the parquet-arrow-read-refactor branch July 30, 2019 20:00

asfimport mentioned this pull request Aug 1, 2019
[C++] Reorganize parquet/arrow/reader.cc, remove code duplication, improve readability #22466
Closed

wesm added 4 commits July 29, 2019 09:48

More refactoring progress
02153bf

Refactor complete, tests passing again. IWYU pass
174f046

More cleaning
9d9a30b

Add missing API include, fix Python bindings
cf9f5e3

pitrou reviewed Jul 30, 2019
View reviewed changes

wesm closed this in dbd93e3Jul 30, 2019

wesm deleted the parquet-arrow-read-refactor branch July 30, 2019 20:00

asfimport mentioned this pull request Aug 1, 2019
[C++] Reorganize parquet/arrow/reader.cc, remove code duplication, improve readability #22466
Closed

wesm added 4 commits July 29, 2019 09:48

More refactoring progress
02153bf

Refactor complete, tests passing again. IWYU pass
174f046

More cleaning
9d9a30b

Add missing API include, fix Python bindings
cf9f5e3

pitrou reviewed Jul 30, 2019
View reviewed changes

wesm closed this in dbd93e3Jul 30, 2019

wesm deleted the parquet-arrow-read-refactor branch July 30, 2019 20:00

asfimport mentioned this pull request Aug 1, 2019
[C++] Reorganize parquet/arrow/reader.cc, remove code duplication, improve readability #22466
Closed

wesm added 4 commits July 29, 2019 09:48

More refactoring progress
02153bf

Refactor complete, tests passing again. IWYU pass
174f046

More cleaning
9d9a30b

Add missing API include, fix Python bindings
cf9f5e3

pitrou reviewed Jul 30, 2019
View reviewed changes

wesm closed this in dbd93e3Jul 30, 2019

wesm deleted the parquet-arrow-read-refactor branch July 30, 2019 20:00

asfimport mentioned this pull request Aug 1, 2019
[C++] Reorganize parquet/arrow/reader.cc, remove code duplication, improve readability #22466
Closed

wesm added 4 commits July 29, 2019 09:48

More refactoring progress
02153bf

Refactor complete, tests passing again. IWYU pass
174f046

More cleaning
9d9a30b

Add missing API include, fix Python bindings
cf9f5e3

pitrou reviewed Jul 30, 2019
View reviewed changes

wesm closed this in dbd93e3Jul 30, 2019

wesm deleted the parquet-arrow-read-refactor branch July 30, 2019 20:00

asfimport mentioned this pull request Aug 1, 2019
[C++] Reorganize parquet/arrow/reader.cc, remove code duplication, improve readability #22466
Closed

wesm added 4 commits July 29, 2019 09:48

More refactoring progress
02153bf

Refactor complete, tests passing again. IWYU pass
174f046

More cleaning
9d9a30b

Add missing API include, fix Python bindings
cf9f5e3

pitrou reviewed Jul 30, 2019
View reviewed changes

wesm closed this in dbd93e3Jul 30, 2019

wesm deleted the parquet-arrow-read-refactor branch July 30, 2019 20:00

asfimport mentioned this pull request Aug 1, 2019
[C++] Reorganize parquet/arrow/reader.cc, remove code duplication, improve readability #22466
Closed

Uh oh!

Conversation

wesm commented Jul 29, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wesm commented Jul 29, 2019

Uh oh!

codecov-io commented Jul 29, 2019

Codecov Report

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wesm commented Jul 30, 2019

Uh oh!

emkornfield commented Jul 30, 2019

Uh oh!

wesm commented Jul 30, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

wesm commented Jul 29, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wesm commented Jul 29, 2019

Uh oh!

codecov-io commented Jul 29, 2019

Codecov Report

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wesm commented Jul 29, 2019•
edited
Loading

wesm commented Jul 29, 2019•
edited
Loading