GH-40592: [C++][Parquet] Implement SizeStatistics

[wgtmac]

Rationale for this change

Parquet format 2.10.0 has introduced SizeStatistics. parquet-mr has also implemented this: apache/parquet-java#1177. Now it is time for parquet-cpp to pick the ball.

What changes are included in this PR?

Implement reading and writing size statistics for parquet-cpp.

Are these changes tested?

Yes, a bunch of test cases have been added.

Are there any user-facing changes?

Yes, now parquet users are able to read and write size statistics.

• GitHub Issue: [C++][Parquet] Implement SizeStatistics #40592

[emkornfield]

A few high level questions/suggestions.

[wgtmac]

Finally this PR is complete on my side. Please take a look when you have time. Thanks! @emkornfield @pitrou @mapleFU

[pitrou]

Sorry for the delay @wgtmac . This is a first partial review, I'll go over the rest once these comments are answered or addressed :-)

[pitrou]

If you're using the pimpl idiom, then you should just return a SizeStatistics here, since all the implementation is already inside a std::unique_ptr.

[pitrou]

Conversely, you could also remove the pimpl idiom and return a subclass here instead. This is better if you want to be able to pass an optionally null pointer, or store a shared_ptr at some pointer.

[wgtmac]

I was following the pimpl idiom of class FileMetaData:

arrow/cpp/src/parquet/metadata.h

Lines 275 to 279 in 14384ac

	/// \brief Create a FileMetaData from a serialized thrift message.
	static std::shared_ptr<FileMetaData> Make(
	const void* serialized_metadata, uint32_t* inout_metadata_len,
	const ReaderProperties& properties = default_reader_properties(),
	std::shared_ptr<InternalFileDecryptor> file_decryptor = NULLPTR);

Returning a SizeStatistics instead of std::unique_ptr<SizeStatistics> make it impossible to store it in a smart pointer, which is on the contrary of the convention in this codebase.

Returning a subclass requires implementing virtual functions, which will be called frequently at every batch. This is something I want to avoid.

[pitrou]

Returning a subclass requires implementing virtual functions, which will be called frequently at every batch. This is something I want to avoid.

What do you mean, frequently?

(ideally this would be a simple struct:

struct SizeStatistics {
  std::optional<int64_t> unencoded_byte_array_data_bytes;
  std::vector<int64_t> definition_level_histogram;
  std::vector<int64_t> repetition_level_histogram;
};

[wgtmac]

Sure, let me change this.

[pitrou]

Wouldn't it be more logical for the BYTE_ARRAY encoders to accumulate the unencoded_byte_array_data_bytes, instead of visiting the input data again here?

[wgtmac]

There are two cases where BYTE_ARRAY encoders do not work:

1. When dictionary encoding is enabled.
2. When the input data is in a arrow::DictionaryArray.

[pitrou]

Are these two cases supposed to produce a unencoded_byte_array_data_bytes? In any case, the approach used here seems wasteful.

[emkornfield]

going to do another pass through, CI failure looks like a formatting issue.

[emkornfield]

LGTM, I'm OK with this as long as @pitrou is thank you for driving this.

[wgtmac]

@emkornfield @mapleFU Thanks for the feedback! I haven't addressed all comments from @pitrou yet. Will let you know once ready for review again.

[wgtmac]

@pitrou @emkornfield @mapleFU Gentle ping :)

[pitrou]

So we are dictionary-decoding the entire array just to run basic statistics? This seems incredibly wasteful.

[wgtmac]

I dislike the approach as you did. It seems that it has been used for collecting page statistics already for a long time. Do you think it is better to fix it in a separate PR or just do it in this one altogether?

[pitrou]

I think this is generally ill-designed and would deserve a rethink to avoid glaring inefficiencies. -1 from me on this PR.

(see the comments I posted above for more focussed complaints)