Skip to content

GH-46462: [C++][Parquet] Expose currently thrown EncodedStatistics when checking is_stats_set #46463

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
+77 −38
Open

GH-46462: [C++][Parquet] Expose currently thrown EncodedStatistics when checking is_stats_set #46463

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c755198
Expose EncodedStatistics and avoid spurious conversions
raulcd May 13, 2025
7758c39
Validate access to encoded_statistics on some tests and use them on p…
raulcd May 15, 2025
3ada3bd
Fixes related to code review
raulcd May 19, 2025
ed301a0
fix value access
raulcd May 19, 2025
e50b010
Fix possible resource deadlock
raulcd May 19, 2025
c802ae7
Update cpp/src/parquet/metadata.cc
raulcd May 20, 2025
15b4950
Check for is_stats_set before trying to generate possible_stats_
raulcd May 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 2 additions & 28 deletions cpp/src/parquet/column_reader.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,34 +196,8 @@ namespace {
template <typename H>
EncodedStatistics ExtractStatsFromHeader(const H& header) {
EncodedStatistics page_statistics;
if (!header.__isset.statistics) {
return page_statistics;
}
const format::Statistics& stats = header.statistics;
// Use the new V2 min-max statistics over the former one if it is filled
if (stats.__isset.max_value || stats.__isset.min_value) {
// TODO: check if the column_order is TYPE_DEFINED_ORDER.
if (stats.__isset.max_value) {
page_statistics.set_max(stats.max_value);
}
if (stats.__isset.min_value) {
page_statistics.set_min(stats.min_value);
}
} else if (stats.__isset.max || stats.__isset.min) {
// TODO: check created_by to see if it is corrupted for some types.
// TODO: check if the sort_order is SIGNED.
if (stats.__isset.max) {
page_statistics.set_max(stats.max);
}
if (stats.__isset.min) {
page_statistics.set_min(stats.min);
}
}
if (stats.__isset.null_count) {
page_statistics.set_null_count(stats.null_count);
}
if (stats.__isset.distinct_count) {
page_statistics.set_distinct_count(stats.distinct_count);
if (header.__isset.statistics) {
page_statistics = FromThrift(header.statistics);
}
return page_statistics;
}
Expand Down
29 changes: 23 additions & 6 deletions cpp/src/parquet/metadata.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,7 @@ class ColumnChunkMetaData::ColumnChunkMetaDataImpl {
size_statistics_->Validate(descr_);
}
possible_stats_ = nullptr;
possible_encoded_stats_ = nullptr;
possible_geo_stats_ = nullptr;
InitKeyValueMetadata();
}
Expand Down Expand Up @@ -312,14 +313,14 @@ class ColumnChunkMetaData::ColumnChunkMetaDataImpl {
return false;
}
{
// Because we are modifying possible_stats_ in a const method
// Because we are modifying possible_encoded_stats_ in a const method
const std::lock_guard<std::mutex> guard(stats_mutex_);
if (possible_stats_ == nullptr) {
possible_stats_ = MakeColumnStats(*column_metadata_, descr_);
if (possible_encoded_stats_ == nullptr) {
possible_encoded_stats_ =
std::make_shared<EncodedStatistics>(FromThrift(column_metadata_->statistics));
}
}
EncodedStatistics encodedStatistics = possible_stats_->Encode();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it is unnecessary to call Encode() every time is_stats_set(). Should we take a different approach by caching the result of is_stats_set()?

IMO, many Parquet metadata objects are meant to access once and cache them for follow-up uses. For example, FileMetaData will always create new RowGroupMetaData and ColumnMetaData objects so users are supposed to cache them to avoid creation cost.

return writer_version_->HasCorrectStatistics(type(), encodedStatistics,
return writer_version_->HasCorrectStatistics(type(), *possible_encoded_stats_,
descr_->sort_order());
}

Expand All @@ -334,8 +335,19 @@ class ColumnChunkMetaData::ColumnChunkMetaDataImpl {
return possible_geo_stats_ != nullptr && possible_geo_stats_->is_valid();
}

inline std::shared_ptr<EncodedStatistics> encoded_statistics() const {
return is_stats_set() ? possible_encoded_stats_ : nullptr;
}

inline std::shared_ptr<Statistics> statistics() const {
return is_stats_set() ? possible_stats_ : nullptr;
if (is_stats_set()) {
// Because we are modifying possible_stats_ in a const method
const std::lock_guard<std::mutex> guard(stats_mutex_);
if (possible_stats_ == nullptr) {
possible_stats_ = MakeColumnStats(*column_metadata_, descr_);
}
}
return possible_stats_;
}

inline std::shared_ptr<SizeStatistics> size_statistics() const {
Expand Down Expand Up @@ -425,6 +437,7 @@ class ColumnChunkMetaData::ColumnChunkMetaDataImpl {
}

mutable std::mutex stats_mutex_;
mutable std::shared_ptr<EncodedStatistics> possible_encoded_stats_;
mutable std::shared_ptr<Statistics> possible_stats_;
mutable std::shared_ptr<geospatial::GeoStatistics> possible_geo_stats_;
std::vector<Encoding::type> encodings_;
Expand Down Expand Up @@ -474,6 +487,10 @@ std::shared_ptr<schema::ColumnPath> ColumnChunkMetaData::path_in_schema() const
return impl_->path_in_schema();
}

std::shared_ptr<EncodedStatistics> ColumnChunkMetaData::encoded_statistics() const {
return impl_->encoded_statistics();
}

std::shared_ptr<Statistics> ColumnChunkMetaData::statistics() const {
return impl_->statistics();
}
Expand Down
1 change: 1 addition & 0 deletions cpp/src/parquet/metadata.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ class PARQUET_EXPORT ColumnChunkMetaData {
bool is_stats_set() const;
bool is_geo_stats_set() const;
std::shared_ptr<Statistics> statistics() const;
std::shared_ptr<EncodedStatistics> encoded_statistics() const;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just have a general question that when encoded_statistics() is useful and worth exposing it to downstream users?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I also wondering this...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can think of a couple of cases where this can be useful. For example if I am building a metadata inspection tool where I don't need the TypedStatistics but access to the encoded values, something like the printer improvement on the PR. It could be also useful if I am consolidating several parquet files and I want to get the encoded values directly but don't necessarily need the TypedStatistics.
As an example if I want to check the size of the plain encoded statistics on the metadata with something like the following snippet:

    size_t total_size = 0;
    for (int iter = 0; iter < 1000; ++iter) {
        for (int r = 0; r < metadata->num_row_groups(); r++) {
            const auto row_group = metadata->RowGroup(r);
            for (int c = 0; c < metadata->num_columns(); c++) {
                const auto stats = row_group->ColumnChunk(c)->statistics();
                if (stats->HasMinMax()) {
                    const std::string& min_encoded = stats->EncodeMin();
                    const std::string& max_encoded = stats->EncodeMax();
                    total_size += min_encoded.size() + max_encoded.size();
                }
            }
        }
    }

Having access to the encoded_statistics directly gave me almost an order of magnitude improvement just avoiding having to throw the EncodedStatistics and encode them again. When tested on a 10 column int64, 100 rowgroups:
With current branch: ~2994ms (average of 3043ms, 2984ms, 2955ms)
With my PR and minor changes to use the new API: ~361ms (average of 347ms, 367ms, 369ms)

@pitrou thoughts on this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree that it can be beneficial, for example if one is only interested in the null count.

Note that EncodedStatistics is already exposed on DataPage, so it makes sense to expose it here as well.

A separate improvement would be to make the construction of TypedStatistics faster, for example creating a decoder instance just to PLAIN-decode one value does not really make sense. But that's a bit orthogonal IMHO.

std::shared_ptr<SizeStatistics> size_statistics() const;
std::shared_ptr<geospatial::GeoStatistics> geo_statistics() const;

Expand Down
8 changes: 8 additions & 0 deletions cpp/src/parquet/metadata_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,10 @@ TEST(Metadata, TestBuildAccess) {
ASSERT_EQ(stats_float.max(), rg1_column2->statistics()->EncodeMax());
ASSERT_EQ(stats_int.min(), rg1_column1->statistics()->EncodeMin());
ASSERT_EQ(stats_int.max(), rg1_column1->statistics()->EncodeMax());
ASSERT_EQ(stats_float.min(), rg1_column2->encoded_statistics()->min());
ASSERT_EQ(stats_float.max(), rg1_column2->encoded_statistics()->max());
ASSERT_EQ(stats_int.min(), rg1_column1->encoded_statistics()->min());
ASSERT_EQ(stats_int.max(), rg1_column1->encoded_statistics()->max());
ASSERT_EQ(0, rg1_column1->statistics()->null_count());
ASSERT_EQ(0, rg1_column2->statistics()->null_count());
ASSERT_EQ(nrows, rg1_column1->statistics()->distinct_count());
Expand Down Expand Up @@ -205,6 +209,10 @@ TEST(Metadata, TestBuildAccess) {
ASSERT_EQ(stats_float.max(), rg2_column2->statistics()->EncodeMax());
ASSERT_EQ(stats_int.min(), rg1_column1->statistics()->EncodeMin());
ASSERT_EQ(stats_int.max(), rg1_column1->statistics()->EncodeMax());
ASSERT_EQ(stats_float.min(), rg2_column2->encoded_statistics()->min());
ASSERT_EQ(stats_float.max(), rg2_column2->encoded_statistics()->max());
ASSERT_EQ(stats_int.min(), rg1_column1->encoded_statistics()->min());
ASSERT_EQ(stats_int.max(), rg1_column1->encoded_statistics()->max());
ASSERT_EQ(0, rg2_column1->statistics()->null_count());
ASSERT_EQ(0, rg2_column2->statistics()->null_count());
ASSERT_EQ(nrows, rg2_column1->statistics()->distinct_count());
Expand Down
8 changes: 4 additions & 4 deletions cpp/src/parquet/printer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ void ParquetFilePrinter::DebugPrint(std::ostream& stream, std::list<int> selecte
// Print column metadata
for (auto i : selected_columns) {
auto column_chunk = group_metadata->ColumnChunk(i);
std::shared_ptr<Statistics> stats = column_chunk->statistics();
std::shared_ptr<EncodedStatistics> stats = column_chunk->encoded_statistics();

const ColumnDescriptor* descr = file_metadata->schema()->Column(i);
stream << "Column " << i << std::endl;
Expand All @@ -165,9 +165,9 @@ void ParquetFilePrinter::DebugPrint(std::ostream& stream, std::list<int> selecte
}
stream << " Values: " << column_chunk->num_values();
if (column_chunk->is_stats_set()) {
std::string min = stats->EncodeMin(), max = stats->EncodeMax();
stream << ", Null Values: " << stats->null_count()
<< ", Distinct Values: " << stats->distinct_count() << std::endl
std::string min = stats->min(), max = stats->max();
stream << ", Null Values: " << stats->null_count
<< ", Distinct Values: " << stats->distinct_count << std::endl
<< " Max: " << FormatStatValue(descr->physical_type(), max)
<< ", Min: " << FormatStatValue(descr->physical_type(), min);
} else {
Expand Down
7 changes: 7 additions & 0 deletions cpp/src/parquet/statistics_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -452,6 +452,13 @@ class TestStatistics : public PrimitiveTypedTest<TestType> {
EXPECT_TRUE(expected_stats->HasMinMax());
EXPECT_EQ(expected_stats->EncodeMin(), stats->EncodeMin());
EXPECT_EQ(expected_stats->EncodeMax(), stats->EncodeMax());

std::shared_ptr<EncodedStatistics> enc_stats = column_chunk->encoded_statistics();
EXPECT_EQ(null_count, enc_stats->null_count);
EXPECT_TRUE(enc_stats->has_min);
EXPECT_TRUE(enc_stats->has_max);
EXPECT_EQ(expected_stats->EncodeMin(), enc_stats->min());
EXPECT_EQ(expected_stats->EncodeMax(), enc_stats->max());
}
};

Expand Down
32 changes: 32 additions & 0 deletions cpp/src/parquet/thrift_internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,38 @@ static inline AadMetadata FromThrift(format::AesGcmCtrV1 aesGcmCtrV1) {
aesGcmCtrV1.supply_aad_prefix};
}

static inline EncodedStatistics FromThrift(const format::Statistics& stats) {
EncodedStatistics out;

// Use the new V2 min-max statistics over the former one if it is filled
if (stats.__isset.max_value || stats.__isset.min_value) {
// TODO: check if the column_order is TYPE_DEFINED_ORDER.
if (stats.__isset.max_value) {
out.set_max(stats.max_value);
}
if (stats.__isset.min_value) {
out.set_min(stats.min_value);
}
} else if (stats.__isset.max || stats.__isset.min) {
// TODO: check created_by to see if it is corrupted for some types.
// TODO: check if the sort_order is SIGNED.
if (stats.__isset.max) {
out.set_max(stats.max);
}
if (stats.__isset.min) {
out.set_min(stats.min);
}
}
if (stats.__isset.null_count) {
out.set_null_count(stats.null_count);
}
if (stats.__isset.distinct_count) {
out.set_distinct_count(stats.distinct_count);
}

return out;
}

static inline geospatial::EncodedGeoStatistics FromThrift(
const format::GeospatialStatistics& geo_stats) {
geospatial::EncodedGeoStatistics out;
Expand Down
Loading