You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@arrow.apache.org by we...@apache.org on 2018/09/30 21:53:16 UTC
[arrow] branch master updated: PARQUET-1369: Disregard column sort
order if statistics max/min are equal
This is an automated email from the ASF dual-hosted git repository.
wesm pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/arrow.git
The following commit(s) were added to refs/heads/master by this push:
new dfe9523 PARQUET-1369: Disregard column sort order if statistics max/min are equal
dfe9523 is described below
commit dfe9523958a58c6d500b68b0fd8eec559cefc1e2
Author: Robert Gruener <ro...@uber.com>
AuthorDate: Sun Sep 30 17:53:07 2018 -0400
PARQUET-1369: Disregard column sort order if statistics max/min are equal
I need to still fix this PR up a little but I have now moved it from the old parquet-cpp repo
Author: Robert Gruener <ro...@uber.com>
Closes #2655 from rgruener/stats-order and squashes the following commits:
ebdc36d41 <Robert Gruener> PARQUET-1369: Disregard column sort order if statistics max/min are equal
---
cpp/src/parquet/metadata-test.cc | 37 ++++++++++++++++++++++++++++++++-----
cpp/src/parquet/metadata.cc | 35 ++++++++++++++++++++++++-----------
cpp/src/parquet/metadata.h | 2 +-
3 files changed, 57 insertions(+), 17 deletions(-)
diff --git a/cpp/src/parquet/metadata-test.cc b/cpp/src/parquet/metadata-test.cc
index 53653bd..bcf911e 100644
--- a/cpp/src/parquet/metadata-test.cc
+++ b/cpp/src/parquet/metadata-test.cc
@@ -16,9 +16,12 @@
// under the License.
#include "parquet/metadata.h"
+
#include <gtest/gtest.h>
+
#include "parquet/schema.h"
#include "parquet/statistics.h"
+#include "parquet/thrift.h"
#include "parquet/types.h"
namespace parquet {
@@ -219,12 +222,36 @@ TEST(ApplicationVersion, Basics) {
ASSERT_EQ(true, version.VersionLt(version1));
- ASSERT_FALSE(version1.HasCorrectStatistics(Type::INT96, SortOrder::UNKNOWN));
- ASSERT_TRUE(version.HasCorrectStatistics(Type::INT32, SortOrder::SIGNED));
- ASSERT_FALSE(version.HasCorrectStatistics(Type::BYTE_ARRAY, SortOrder::SIGNED));
- ASSERT_TRUE(version1.HasCorrectStatistics(Type::BYTE_ARRAY, SortOrder::SIGNED));
+ EncodedStatistics stats;
+ ASSERT_FALSE(version1.HasCorrectStatistics(Type::INT96, stats, SortOrder::UNKNOWN));
+ ASSERT_TRUE(version.HasCorrectStatistics(Type::INT32, stats, SortOrder::SIGNED));
+ ASSERT_FALSE(version.HasCorrectStatistics(Type::BYTE_ARRAY, stats, SortOrder::SIGNED));
+ ASSERT_TRUE(version1.HasCorrectStatistics(Type::BYTE_ARRAY, stats, SortOrder::SIGNED));
+ ASSERT_FALSE(
+ version1.HasCorrectStatistics(Type::BYTE_ARRAY, stats, SortOrder::UNSIGNED));
+ ASSERT_TRUE(version3.HasCorrectStatistics(Type::FIXED_LEN_BYTE_ARRAY, stats,
+ SortOrder::SIGNED));
+
+ // Check that the old stats are correct if min and max are the same
+ // regardless of sort order
+ EncodedStatistics stats_str;
+ stats_str.set_min("a").set_max("b");
+ ASSERT_FALSE(
+ version1.HasCorrectStatistics(Type::BYTE_ARRAY, stats_str, SortOrder::UNSIGNED));
+ stats_str.set_max("a");
+ ASSERT_TRUE(
+ version1.HasCorrectStatistics(Type::BYTE_ARRAY, stats_str, SortOrder::UNSIGNED));
+
+ // Check that the same holds true for ints
+ int32_t int_min = 100, int_max = 200;
+ EncodedStatistics stats_int;
+ stats_int.set_min(std::string(reinterpret_cast<const char*>(&int_min), 4))
+ .set_max(std::string(reinterpret_cast<const char*>(&int_max), 4));
+ ASSERT_FALSE(
+ version1.HasCorrectStatistics(Type::BYTE_ARRAY, stats_int, SortOrder::UNSIGNED));
+ stats_int.set_max(std::string(reinterpret_cast<const char*>(&int_min), 4));
ASSERT_TRUE(
- version3.HasCorrectStatistics(Type::FIXED_LEN_BYTE_ARRAY, SortOrder::SIGNED));
+ version1.HasCorrectStatistics(Type::BYTE_ARRAY, stats_int, SortOrder::UNSIGNED));
}
} // namespace metadata
diff --git a/cpp/src/parquet/metadata.cc b/cpp/src/parquet/metadata.cc
index 9c66c7a..f49393b 100644
--- a/cpp/src/parquet/metadata.cc
+++ b/cpp/src/parquet/metadata.cc
@@ -55,7 +55,8 @@ static std::shared_ptr<RowGroupStatistics> MakeTypedColumnStats(
return std::make_shared<TypedRowGroupStatistics<DType>>(
descr, metadata.statistics.min_value, metadata.statistics.max_value,
metadata.num_values - metadata.statistics.null_count,
- metadata.statistics.null_count, metadata.statistics.distinct_count, true);
+ metadata.statistics.null_count, metadata.statistics.distinct_count,
+ metadata.statistics.__isset.max_value || metadata.statistics.__isset.min_value);
}
// Default behavior
return std::make_shared<TypedRowGroupStatistics<DType>>(
@@ -100,7 +101,7 @@ class ColumnChunkMetaData::ColumnChunkMetaDataImpl {
for (auto encoding : meta_data.encodings) {
encodings_.push_back(FromThrift(encoding));
}
- stats_ = nullptr;
+ possible_stats_ = nullptr;
}
~ColumnChunkMetaDataImpl() {}
@@ -125,15 +126,22 @@ class ColumnChunkMetaData::ColumnChunkMetaDataImpl {
// Eg: UTF8
inline bool is_stats_set() const {
DCHECK(writer_version_ != nullptr);
- return column_->meta_data.__isset.statistics &&
- writer_version_->HasCorrectStatistics(type(), descr_->sort_order());
+ // If the column statistics don't exist or column sort order is unknown
+ // we cannot use the column stats
+ if (!column_->meta_data.__isset.statistics ||
+ descr_->sort_order() == SortOrder::UNKNOWN) {
+ return false;
+ }
+ if (possible_stats_ == nullptr) {
+ possible_stats_ = MakeColumnStats(column_->meta_data, descr_);
+ }
+ EncodedStatistics encodedStatistics = possible_stats_->Encode();
+ return writer_version_->HasCorrectStatistics(type(), encodedStatistics,
+ descr_->sort_order());
}
inline std::shared_ptr<RowGroupStatistics> statistics() const {
- if (stats_ == nullptr && is_stats_set()) {
- stats_ = MakeColumnStats(column_->meta_data, descr_);
- }
- return stats_;
+ return is_stats_set() ? possible_stats_ : nullptr;
}
inline Compression::type compression() const {
@@ -169,7 +177,7 @@ class ColumnChunkMetaData::ColumnChunkMetaDataImpl {
}
private:
- mutable std::shared_ptr<RowGroupStatistics> stats_;
+ mutable std::shared_ptr<RowGroupStatistics> possible_stats_;
std::vector<Encoding::type> encodings_;
const format::ColumnChunk* column_;
const ColumnDescriptor* descr_;
@@ -530,11 +538,16 @@ bool ApplicationVersion::VersionEq(const ApplicationVersion& other_version) cons
// parquet-mr/parquet-column/src/main/java/org/apache/parquet/CorruptStatistics.java
// PARQUET-686 has more disussion on statistics
bool ApplicationVersion::HasCorrectStatistics(Type::type col_type,
+ EncodedStatistics& statistics,
SortOrder::type sort_order) const {
// Parquet cpp version 1.3.0 onwards stats are computed correctly for all types
if ((application_ != "parquet-cpp") || (VersionLt(PARQUET_CPP_FIXED_STATS_VERSION()))) {
- // Only SIGNED are valid
- if (SortOrder::SIGNED != sort_order) {
+ // Only SIGNED are valid unless max and min are the same
+ // (in which case the sort order does not matter)
+ bool max_equals_min = statistics.has_min && statistics.has_max
+ ? statistics.min() == statistics.max()
+ : false;
+ if (SortOrder::SIGNED != sort_order && !max_equals_min) {
return false;
}
diff --git a/cpp/src/parquet/metadata.h b/cpp/src/parquet/metadata.h
index 79f4fdb..7e29fe9 100644
--- a/cpp/src/parquet/metadata.h
+++ b/cpp/src/parquet/metadata.h
@@ -85,7 +85,7 @@ class ApplicationVersion {
bool VersionEq(const ApplicationVersion& other_version) const;
// Checks if the Version has the correct statistics for a given column
- bool HasCorrectStatistics(Type::type primitive,
+ bool HasCorrectStatistics(Type::type primitive, EncodedStatistics& statistics,
SortOrder::type sort_order = SortOrder::SIGNED) const;
};