Skip to content

GH-43594: [C++] Remove std::optional from arrow::ArrayStatistics::is_{min,max}_exact #43595

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

Merged
kou merged 1 commit into from
Aug 16, 2024
Merged

GH-43594: [C++] Remove std::optional from arrow::ArrayStatistics::is_{min,max}_exact #43595

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 4 additions & 4 deletions cpp/src/arrow/array/statistics.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,14 +43,14 @@ struct ARROW_EXPORT ArrayStatistics {
/// \brief The minimum value, may not be set
std::optional<ValueType> min = std::nullopt;

/// \brief Whether the minimum value is exact or not, may not be set
std::optional<bool> is_min_exact = std::nullopt;
/// \brief Whether the minimum value is exact or not
Copy link
Member

@mapleFU mapleFU Aug 7, 2024

Choose a reason for hiding this comment

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

This looks good to me, but IMO is_min_exact = false might still has exact statistics but the reader cannot gurantee that, since apache/parquet-format#216 is new in 2.10 :-(

Copy link
Member Author

@kou kou Aug 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Are the followings correct?

  1. Parquet 2.9 or earlier data don't have is_min_value_exact/is_max_value_exact
  2. Parquet 2.9 or earlier data use only exact min/max
  3. Parquet 2.10 or later data use exact min/max or non-exact min/max
  4. Parquet 2.10 or later data may use exact min/max without is_min_value_exact/is_max_value_exact

You're focusing on the 2. case, right? Can our Parquet reader detect Parquet version? If so, can we always set true to is_min_exact/is_max_exact for Parquet 2.9 or earlier?

Copy link
Member

@mapleFU mapleFU Aug 16, 2024

Choose a reason for hiding this comment

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

Sorry for late replying

Parquet 2.9 or earlier data don't have is_min_value_exact/is_max_value_exact

Yes

Parquet 2.9 or earlier data use only exact min/max

I guess no, I'll send a mail to maillist to make it sure

Parquet 2.10 or later data use exact min/max or non-exact min/max

Yes

Parquet 2.10 or later data may use exact min/max without is_min_value_exact/is_max_value_exact

right

Copy link
Member

@mapleFU mapleFU Aug 16, 2024

Choose a reason for hiding this comment

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

If so, can we always set true to is_min_exact/is_max_exact for Parquet 2.9 or earlier?

Hmmm I'll try to make it clear

Copy link
Member Author

@kou kou Aug 16, 2024

Choose a reason for hiding this comment

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

Thanks!

Copy link
Member

@mapleFU mapleFU Aug 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Anyway we can mention "exact=false" can also means is exact, lol

Or we can denote that the parquet-c++ output is exact.

Copy link
Member Author

@kou kou Aug 16, 2024

Choose a reason for hiding this comment

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

OK. Let's use exact=true for Apache Parquet C++ output.

bool is_min_exact = false;

/// \brief The maximum value, may not be set
std::optional<ValueType> max = std::nullopt;

/// \brief Whether the maximum value is exact or not, may not be set
std::optional<bool> is_max_exact = std::nullopt;
/// \brief Whether the maximum value is exact or not
bool is_max_exact = false;

/// \brief Check two statistics for equality
bool Equals(const ArrayStatistics& other) const {
Expand Down
14 changes: 6 additions & 8 deletions cpp/src/arrow/array/statistics_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,27 +40,25 @@ TEST(ArrayStatisticsTest, TestDistinctCount) {
TEST(ArrayStatisticsTest, TestMin) {
ArrayStatistics statistics;
ASSERT_FALSE(statistics.min.has_value());
ASSERT_FALSE(statistics.is_min_exact.has_value());
ASSERT_FALSE(statistics.is_min_exact);
statistics.min = static_cast<uint64_t>(29);
statistics.is_min_exact = true;
ASSERT_TRUE(statistics.min.has_value());
ASSERT_TRUE(std::holds_alternative<uint64_t>(statistics.min.value()));
ASSERT_EQ(29, std::get<uint64_t>(statistics.min.value()));
ASSERT_TRUE(statistics.is_min_exact.has_value());
ASSERT_TRUE(statistics.is_min_exact.value());
ASSERT_TRUE(statistics.is_min_exact);
}

TEST(ArrayStatisticsTest, TestMax) {
ArrayStatistics statistics;
ASSERT_FALSE(statistics.max.has_value());
ASSERT_FALSE(statistics.is_max_exact.has_value());
ASSERT_FALSE(statistics.is_max_exact);
statistics.max = std::string("hello");
statistics.is_max_exact = false;
ASSERT_TRUE(statistics.max.has_value());
ASSERT_TRUE(std::holds_alternative<std::string>(statistics.max.value()));
ASSERT_EQ("hello", std::get<std::string>(statistics.max.value()));
ASSERT_TRUE(statistics.is_max_exact.has_value());
ASSERT_FALSE(statistics.is_max_exact.value());
ASSERT_FALSE(statistics.is_max_exact);
}

TEST(ArrayStatisticsTest, TestEquality) {
Expand All @@ -84,9 +82,9 @@ TEST(ArrayStatisticsTest, TestEquality) {
statistics2.min = std::string("world");
ASSERT_EQ(statistics1, statistics2);

statistics1.is_min_exact = false;
statistics1.is_min_exact = true;
ASSERT_NE(statistics1, statistics2);
statistics2.is_min_exact = false;
statistics2.is_min_exact = true;
ASSERT_EQ(statistics1, statistics2);

statistics1.max = static_cast<int64_t>(-29);
Expand Down