You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "andishgar (via GitHub)" <gi...@apache.org> on 2025/05/10 06:40:39 UTC
[PR] GH-45639 : [C++][Statistics] Add ARROW:average_byte_width:exact and ARROW:average_byte_width:approximate statistics attributes to arrow::ArrayStatistic [arrow]
andishgar opened a new pull request, #46385:
URL: https://github.com/apache/arrow/pull/46385
### Rationale for this change
Add `ARROW:average_byte_width:exact` and `ARROW:average_byte_width:approximate` Statistics attribute to `arrow::ArrayStatistics`
### What changes are included in this PR?
Add average_byte_width and is_average_byte_width_exact member variables to arrow::ArrayStatistics
### Are these changes tested?
Yes, I run the relevant unit test
### Are there any user-facing changes?
Yes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45639: [C++][Statistics] Add support for ARROW:average_byte_width:{exac,approximate} [arrow]
Posted by "andishgar (via GitHub)" <gi...@apache.org>.
andishgar commented on code in PR #46385:
URL: https://github.com/apache/arrow/pull/46385#discussion_r2194663512
##########
cpp/src/arrow/array/statistics_test.cc:
##########
@@ -81,6 +92,14 @@ TEST(TestArrayStatistics, EqualityNonDoulbeValue) {
statistics2.distinct_count = 2929;
ASSERT_EQ(statistics1, statistics2);
+ // Although this assertion is meaningless without average_byte_width,
+ // the purpose here is simply to check whether is_average_byte_width_exact
+ // is included in the Equality.
+ statistics1.is_average_byte_width_exact = true;
+ ASSERT_NE(statistics1, statistics2);
+ statistics2.is_average_byte_width_exact = true;
+ ASSERT_EQ(statistics1, statistics2);
+
Review Comment:
It's Okay .thanks for your response. I will apply chnages
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45639: [C++][Statistics] Add ARROW:average_byte_width:exact and ARROW:average_byte_width:approximate statistics attributes to arrow::ArrayStatistics [arrow]
Posted by "andishgar (via GitHub)" <gi...@apache.org>.
andishgar commented on code in PR #46385:
URL: https://github.com/apache/arrow/pull/46385#discussion_r2083469161
##########
cpp/src/arrow/array/statistics.h:
##########
@@ -131,7 +137,9 @@ struct ARROW_EXPORT ArrayStatistics {
bool Equals(const ArrayStatistics& other) const {
return null_count == other.null_count && distinct_count == other.distinct_count &&
min == other.min && is_min_exact == other.is_min_exact && max == other.max &&
- is_max_exact == other.is_max_exact;
+ is_max_exact == other.is_max_exact &&
+ average_byte_width == other.average_byte_width &&
Review Comment:
I have the following questions regarding the implementation:
1-Should I first create a separate issue to address the `min` and `max` member variables, given that they can be `double` values?
2- I plan to add `arrow::EqualOptions` to the `Equal` method. This would make the` operator==` redundant. Is it acceptable to remove `operator== ` in this case?
3-When comparing two `arrow::Array` objects for equality, should their `arrow::ArrayStatistics` also be checked?
4-Should I implement a separate `ApproximateEqual` method to handle `EqualOptions::atol,` or should this logic be integrated into the `Equal` method?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45639: [C++][Statistics] Add support for ARROW:average_byte_width:{exac,approximate} [arrow]
Posted by "andishgar (via GitHub)" <gi...@apache.org>.
andishgar commented on PR #46385:
URL: https://github.com/apache/arrow/pull/46385#issuecomment-3048618123
@kou Thanks for approving! I have a question regarding writing test cases for double values in my upcoming PRs.
Should I write a separate test case for each double value, similar to what you did in your last commit?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45639: [C++][Statistics] Add support for ARROW:average_byte_width:{exac,approximate} [arrow]
Posted by "andishgar (via GitHub)" <gi...@apache.org>.
andishgar commented on code in PR #46385:
URL: https://github.com/apache/arrow/pull/46385#discussion_r2192347620
##########
cpp/src/arrow/array/statistics_test.cc:
##########
@@ -81,6 +92,14 @@ TEST(TestArrayStatistics, EqualityNonDoulbeValue) {
statistics2.distinct_count = 2929;
ASSERT_EQ(statistics1, statistics2);
+ // Although this assertion is meaningless without average_byte_width,
+ // the purpose here is simply to check whether is_average_byte_width_exact
+ // is included in the Equality.
+ statistics1.is_average_byte_width_exact = true;
+ ASSERT_NE(statistics1, statistics2);
+ statistics2.is_average_byte_width_exact = true;
+ ASSERT_EQ(statistics1, statistics2);
+
Review Comment:
Actually, this test case is for non-double attributes, and I use non-double values for `min` and `max` here.
I created the test cases for double attributes in my last commit under the name `DoubleAttributesCombination`, which include similar assertions for double values.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45639: [C++][Statistics] Add ARROW:average_byte_width:exact and ARROW:average_byte_width:approximate statistics attributes to arrow::ArrayStatistics [arrow]
Posted by "andishgar (via GitHub)" <gi...@apache.org>.
andishgar commented on code in PR #46385:
URL: https://github.com/apache/arrow/pull/46385#discussion_r2083537653
##########
cpp/src/arrow/array/statistics.h:
##########
@@ -131,7 +137,9 @@ struct ARROW_EXPORT ArrayStatistics {
bool Equals(const ArrayStatistics& other) const {
return null_count == other.null_count && distinct_count == other.distinct_count &&
min == other.min && is_min_exact == other.is_min_exact && max == other.max &&
- is_max_exact == other.is_max_exact;
+ is_max_exact == other.is_max_exact &&
+ average_byte_width == other.average_byte_width &&
Review Comment:
Okay, I start to create an issue to solve the problem of `min` and `max` first
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45639: [C++][Statistics] Add ARROW:average_byte_width:exact and ARROW:average_byte_width:approximate statistics attributes to arrow::ArrayStatistics [arrow]
Posted by "andishgar (via GitHub)" <gi...@apache.org>.
andishgar commented on PR #46385:
URL: https://github.com/apache/arrow/pull/46385#issuecomment-2979461860
@kou I plan to continue working on this pull request, but I have a question.
As I mentioned [here](https://github.com/apache/arrow/pull/46779#issuecomment-2963846681), we should set arrow::EqualOptions::use_atol to false.
Should I wait until #46779 is merged, or should I make the change in this pull request?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45639: [C++][Statistics] Add support for ARROW:average_byte_width:{exac,approximate} [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou merged PR #46385:
URL: https://github.com/apache/arrow/pull/46385
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45639: [C++][Statistics] Add ARROW:average_byte_width:exact and ARROW:average_byte_width:approximate statistics attributes to arrow::ArrayStatistics [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #46385:
URL: https://github.com/apache/arrow/pull/46385#discussion_r2191576547
##########
cpp/src/arrow/array/statistics_test.cc:
##########
@@ -152,4 +163,19 @@ TEST_F(TestArrayStatisticsEqualityDoubleValue, ApproximateEquals) {
ASSERT_TRUE(statistics1_.Equals(statistics2_, options_.atol(1e-3).use_atol(true)));
}
+TEST_F(TestArrayStatisticsEqualityDoubleValue, DoubleAttributeCombination) {
+ statistics1_.max = 0.5;
+ ASSERT_FALSE(statistics1_.Equals(statistics2_));
+ statistics2_.max = 0.5;
+ ASSERT_TRUE(statistics1_.Equals(statistics2_));
+ statistics1_.min = 0.5;
+ ASSERT_FALSE(statistics1_.Equals(statistics2_));
+ statistics2_.min = 0.5;
Review Comment:
Hmm. I think that this case should be detected by existing other `min`/`max` tests... With the incorrect code, `min`/`max` are ignored. So existing `min`/`max` tests should detect it.
(I still think that we don't need `min`/`max` here for this case.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45639: [C++][Statistics] Add ARROW:average_byte_width:exact and ARROW:average_byte_width:approximate statistics attributes to arrow::ArrayStatistics [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #46385:
URL: https://github.com/apache/arrow/pull/46385#discussion_r2189235349
##########
cpp/src/arrow/array/statistics_test.cc:
##########
@@ -152,4 +163,19 @@ TEST_F(TestArrayStatisticsEqualityDoubleValue, ApproximateEquals) {
ASSERT_TRUE(statistics1_.Equals(statistics2_, options_.atol(1e-3).use_atol(true)));
}
+TEST_F(TestArrayStatisticsEqualityDoubleValue, DoubleAttributeCombination) {
+ statistics1_.max = 0.5;
+ ASSERT_FALSE(statistics1_.Equals(statistics2_));
+ statistics2_.max = 0.5;
+ ASSERT_TRUE(statistics1_.Equals(statistics2_));
+ statistics1_.min = 0.5;
+ ASSERT_FALSE(statistics1_.Equals(statistics2_));
+ statistics2_.min = 0.5;
Review Comment:
Do we need this?
Is only `average_byte_width` enough because `min`/`max` are tested in other tests?
##########
cpp/src/arrow/record_batch.cc:
##########
@@ -45,6 +45,7 @@
#include "arrow/util/logging_internal.h"
#include "arrow/util/vector.h"
#include "arrow/visit_type_inline.h"
+#include "util/logger.h"
Review Comment:
Why do we need this?
##########
cpp/src/arrow/compare.cc:
##########
@@ -1563,6 +1563,9 @@ bool ArrayStatisticsEqualsImpl(const ArrayStatistics& left, const ArrayStatistic
left.distinct_count == right.distinct_count &&
left.is_min_exact == right.is_min_exact &&
left.is_max_exact == right.is_max_exact &&
+ left.is_average_byte_width_exact == right.is_average_byte_width_exact &&
Review Comment:
Could you use the same order in `.h`?
```suggestion
left.is_average_byte_width_exact == right.is_average_byte_width_exact &&
left.is_min_exact == right.is_min_exact &&
left.is_max_exact == right.is_max_exact &&
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45639: [C++][Statistics] Add support for ARROW:average_byte_width:{exac,approximate} [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #46385:
URL: https://github.com/apache/arrow/pull/46385#issuecomment-3048647817
Ah, we don't need the test. It's duplicated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45639 : [C++][Statistics] Add ARROW:average_byte_width:exact and ARROW:average_byte_width:approximate statistics attributes to arrow::ArrayStatistics [arrow]
Posted by "andishgar (via GitHub)" <gi...@apache.org>.
andishgar commented on PR #46385:
URL: https://github.com/apache/arrow/pull/46385#issuecomment-2868732672
@kou, I’ve converted this pull request to a draft in order to clarify whether any modifications are required on the Parquet side.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45639: [C++][Statistics] Add support for ARROW:average_byte_width:{exac,approximate} [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #46385:
URL: https://github.com/apache/arrow/pull/46385#issuecomment-3055882401
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 673823b5a4775a8eb0e3a67e4b3e549febf4b44a.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/45697766095) has more details. It also includes information about 32 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45639: [C++][Statistics] Add support for ARROW:average_byte_width:{exac,approximate} [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #46385:
URL: https://github.com/apache/arrow/pull/46385#discussion_r2192325088
##########
cpp/src/arrow/array/statistics_test.cc:
##########
@@ -152,4 +171,11 @@ TEST_F(TestArrayStatisticsEqualityDoubleValue, ApproximateEquals) {
ASSERT_TRUE(statistics1_.Equals(statistics2_, options_.atol(1e-3).use_atol(true)));
}
+TEST_F(TestArrayStatisticsEqualityDoubleValue, AverageByteWidth) {
+ statistics1_.average_byte_width = 4.2;
+ ASSERT_FALSE(statistics1_.Equals(statistics2_));
+ statistics2_.average_byte_width = 4.2;
+ ASSERT_TRUE(statistics1_.Equals(statistics2_));
+}
+
Review Comment:
```suggestion
```
##########
cpp/src/arrow/array/statistics_test.cc:
##########
@@ -81,6 +92,14 @@ TEST(TestArrayStatistics, EqualityNonDoulbeValue) {
statistics2.distinct_count = 2929;
ASSERT_EQ(statistics1, statistics2);
+ // Although this assertion is meaningless without average_byte_width,
+ // the purpose here is simply to check whether is_average_byte_width_exact
+ // is included in the Equality.
+ statistics1.is_average_byte_width_exact = true;
+ ASSERT_NE(statistics1, statistics2);
+ statistics2.is_average_byte_width_exact = true;
+ ASSERT_EQ(statistics1, statistics2);
+
Review Comment:
Could you add `average_byte_width` case here like we did for `min`/`is_min_exact`?
```cpp
statistics1.average_byte_width = ...
...
statistics1.is_average_byte_width_exact = ...
...
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45639: [C++][Statistics] Add ARROW:average_byte_width:exact and ARROW:average_byte_width:approximate statistics attributes to arrow::ArrayStatistics [arrow]
Posted by "andishgar (via GitHub)" <gi...@apache.org>.
andishgar commented on PR #46385:
URL: https://github.com/apache/arrow/pull/46385#issuecomment-2869522894
@kou thank you for your feedback, I will look into it soon
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45639: [C++][Statistics] Add ARROW:average_byte_width:exact and ARROW:average_byte_width:approximate statistics attributes to arrow::ArrayStatistics [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #46385:
URL: https://github.com/apache/arrow/pull/46385#discussion_r2083522484
##########
cpp/src/arrow/array/statistics.h:
##########
@@ -131,7 +137,9 @@ struct ARROW_EXPORT ArrayStatistics {
bool Equals(const ArrayStatistics& other) const {
return null_count == other.null_count && distinct_count == other.distinct_count &&
min == other.min && is_min_exact == other.is_min_exact && max == other.max &&
- is_max_exact == other.is_max_exact;
+ is_max_exact == other.is_max_exact &&
+ average_byte_width == other.average_byte_width &&
Review Comment:
1. Ah, I didn't notice it. Yes.
2. Yes.
3. (I think) No. I think `arrow::Array::Equals()` should check only data. For example, metadata in its data type isn't used. But we may want to discuss this in a separated issue.
4. Yes or no. Let's discuss it as a separated issue. I think that adding `floating_approximate` to `EqualOptions` and using only `Equals` is better but others may think differ.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45639: [C++][Statistics] Add ARROW:average_byte_width:exact and ARROW:average_byte_width:approximate statistics attributes to arrow::ArrayStatistics [arrow]
Posted by "andishgar (via GitHub)" <gi...@apache.org>.
andishgar commented on code in PR #46385:
URL: https://github.com/apache/arrow/pull/46385#discussion_r2189250279
##########
cpp/src/arrow/record_batch.cc:
##########
@@ -45,6 +45,7 @@
#include "arrow/util/logging_internal.h"
#include "arrow/util/vector.h"
#include "arrow/visit_type_inline.h"
+#include "util/logger.h"
Review Comment:
Oops, sorry — this was left in for debugging. I forgot to remove it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45639 : [C++][Statistics] Add ARROW:average_byte_width:exact and ARROW:average_byte_width:approximate statistics attributes to arrow::ArrayStatistic [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #46385:
URL: https://github.com/apache/arrow/pull/46385#issuecomment-2868520319
:warning: GitHub issue #45639 **has been automatically assigned in GitHub** to PR creator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45639: [C++][Statistics] Add ARROW:average_byte_width:exact and ARROW:average_byte_width:approximate statistics attributes to arrow::ArrayStatistics [arrow]
Posted by "andishgar (via GitHub)" <gi...@apache.org>.
andishgar commented on code in PR #46385:
URL: https://github.com/apache/arrow/pull/46385#discussion_r2083469161
##########
cpp/src/arrow/array/statistics.h:
##########
@@ -131,7 +137,9 @@ struct ARROW_EXPORT ArrayStatistics {
bool Equals(const ArrayStatistics& other) const {
return null_count == other.null_count && distinct_count == other.distinct_count &&
min == other.min && is_min_exact == other.is_min_exact && max == other.max &&
- is_max_exact == other.is_max_exact;
+ is_max_exact == other.is_max_exact &&
+ average_byte_width == other.average_byte_width &&
Review Comment:
I have the following questions regarding the implementation:
1-Should I first create a separate issue to address the `min` and `max` member variables, given that they can be `double` values?
2- I plan to add `arrow::EqualOptions` to the `Equal` method. This would make the` operator==` redundant. Is it acceptable to remove `operator== ` in this case?
3-When comparing two `arrow::Array` objects for equality, should their `arrow::ArrayStatistics` also be checked?
4-Should I implement a separate `ApproximateEqual` method to handle `EqualOptions::atol,` or should this logic be integrated into the `Equal` method?
5-Why isn't `arrow::Scalar `used instead of all the individual types defined in `ArrayStatistics::Value`Type? As mentioned [here](https://github.com/apache/arrow/pull/43273#discussion_r1704278599), if simplification is the goal, arrow::Scalar already supports all necessary types.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45639: [C++][Statistics] Add ARROW:average_byte_width:exact and ARROW:average_byte_width:approximate statistics attributes to arrow::ArrayStatistics [arrow]
Posted by "andishgar (via GitHub)" <gi...@apache.org>.
andishgar commented on code in PR #46385:
URL: https://github.com/apache/arrow/pull/46385#discussion_r2189281143
##########
cpp/src/arrow/array/statistics_test.cc:
##########
@@ -152,4 +163,19 @@ TEST_F(TestArrayStatisticsEqualityDoubleValue, ApproximateEquals) {
ASSERT_TRUE(statistics1_.Equals(statistics2_, options_.atol(1e-3).use_atol(true)));
}
+TEST_F(TestArrayStatisticsEqualityDoubleValue, DoubleAttributeCombination) {
+ statistics1_.max = 0.5;
+ ASSERT_FALSE(statistics1_.Equals(statistics2_));
+ statistics2_.max = 0.5;
+ ASSERT_TRUE(statistics1_.Equals(statistics2_));
+ statistics1_.min = 0.5;
+ ASSERT_FALSE(statistics1_.Equals(statistics2_));
+ statistics2_.min = 0.5;
Review Comment:
Actually, this test is intended to cover [this code path](https://github.com/apache/arrow/blob/be212d7b1162f74992d7fbc31369e5eca33f88a1/cpp/src/arrow/compare.cc#L1567-L1570).
My initial implementation was the code below, but the test revealed that it was incorrect.
```c++
return left.null_count == right.null_count &&
left.distinct_count == right.distinct_count &&
left.is_min_exact == right.is_min_exact &&
left.is_max_exact == right.is_max_exact &&
left.is_average_byte_width_exact == right.is_average_byte_width_exact &&
ArrayStatisticsValueTypeEquals(left.min, right.min, equal_options) &&
ArrayStatisticsValueTypeEquals(left.max, right.max, equal_options);
ArrayStatisticsValueTypeEquals(left.average_byte_width, right.average_byte_width,
equal_options) ;
```
Aside from that, since we plan to add double attributes based on the statistics specification, I suggest keeping this test case to also validate those additions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45639: [C++][Statistics] Add support for ARROW:average_byte_width:{exac,approximate} [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #46385:
URL: https://github.com/apache/arrow/pull/46385#discussion_r2194351333
##########
cpp/src/arrow/array/statistics_test.cc:
##########
@@ -81,6 +92,14 @@ TEST(TestArrayStatistics, EqualityNonDoulbeValue) {
statistics2.distinct_count = 2929;
ASSERT_EQ(statistics1, statistics2);
+ // Although this assertion is meaningless without average_byte_width,
+ // the purpose here is simply to check whether is_average_byte_width_exact
+ // is included in the Equality.
+ statistics1.is_average_byte_width_exact = true;
+ ASSERT_NE(statistics1, statistics2);
+ statistics2.is_average_byte_width_exact = true;
+ ASSERT_EQ(statistics1, statistics2);
+
Review Comment:
Ah, I understand your intention.
I thought that `TestArrayStatisticsEqualityDoubleValue` is for equality logic special double values such as NaN and infinity (and approximate equality).
I think that we can use `TestArrayStatistics` for equality of `average_byte_width` if we don't use special double values such as NaN and infinity.
How about this?
```diff
diff --git a/cpp/src/arrow/array/statistics_test.cc b/cpp/src/arrow/array/statistics_test.cc
index 250c4bb437..22355ce434 100644
--- a/cpp/src/arrow/array/statistics_test.cc
+++ b/cpp/src/arrow/array/statistics_test.cc
@@ -65,7 +65,7 @@ TEST(TestArrayStatistics, Max) {
ASSERT_FALSE(statistics.is_max_exact);
}
-TEST(TestArrayStatistics, EqualityNonDoulbeValue) {
+TEST(TestArrayStatistics, Equals) {
ArrayStatistics statistics1;
ArrayStatistics statistics2;
@@ -81,6 +81,16 @@ TEST(TestArrayStatistics, EqualityNonDoulbeValue) {
statistics2.distinct_count = 2929;
ASSERT_EQ(statistics1, statistics2);
+ statistics1.average_byte_width = 2.9;
+ ASSERT_NE(statistics1, statistics2);
+ statistics2.average_byte_width = 2.9;
+ ASSERT_EQ(statistics1, statistics2);
+
+ statistics1.is_average_byte_width_exact = true;
+ ASSERT_NE(statistics1, statistics2);
+ statistics2.is_average_byte_width_exact = true;
+ ASSERT_EQ(statistics1, statistics2);
+
statistics1.min = std::string("world");
ASSERT_NE(statistics1, statistics2);
statistics2.min = std::string("world");
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45639: [C++][Statistics] Add ARROW:average_byte_width:exact and ARROW:average_byte_width:approximate statistics attributes to arrow::ArrayStatistics [arrow]
Posted by "andishgar (via GitHub)" <gi...@apache.org>.
andishgar commented on code in PR #46385:
URL: https://github.com/apache/arrow/pull/46385#discussion_r2083469161
##########
cpp/src/arrow/array/statistics.h:
##########
@@ -131,7 +137,9 @@ struct ARROW_EXPORT ArrayStatistics {
bool Equals(const ArrayStatistics& other) const {
return null_count == other.null_count && distinct_count == other.distinct_count &&
min == other.min && is_min_exact == other.is_min_exact && max == other.max &&
- is_max_exact == other.is_max_exact;
+ is_max_exact == other.is_max_exact &&
+ average_byte_width == other.average_byte_width &&
Review Comment:
I have the following questions regarding the implementation:
1-Should I first create a separate issue to address the `min` and `max` member variables, given that they can be `double` values?
2- I plan to add `arrow::EqualOptions` to the `Equal` method. This would make the` operator==` redundant. Is it acceptable to remove `operator== ` in this case?
3-When comparing two `arrow::Array` objects for equality, should their `arrow::ArrayStatistics` also be checked?( the current implementation does not check ArrayStatistics)
4-Should I implement a separate `ApproximateEqual` method to handle `EqualOptions::atol,` or should this logic be integrated into the `Equal` method?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45639: [C++][Statistics] Add ARROW:average_byte_width:exact and ARROW:average_byte_width:approximate statistics attributes to arrow::ArrayStatistics [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #46385:
URL: https://github.com/apache/arrow/pull/46385#discussion_r2083294008
##########
cpp/src/arrow/array/array_test.cc:
##########
@@ -3899,6 +3903,10 @@ TEST_F(TestArrayDataStatistics, MoveConstructor) {
ASSERT_TRUE(std::holds_alternative<int64_t>(moved_data.statistics->max.value()));
ASSERT_EQ(max_, std::get<int64_t>(moved_data.statistics->max.value()));
ASSERT_TRUE(moved_data.statistics->is_max_exact);
+
+ ASSERT_TRUE(moved_data.statistics->average_byte_width.has_value());
+ ASSERT_EQ(average_byte_width_, moved_data.statistics->average_byte_width.value());
Review Comment:
We should not use `ASSERT_EQ()` for floating point numbers.
We should use `ASSERT_FLOAT_EQ()`/`ASSERT_DOUBLE_EQ()` for them. See also: https://google.github.io/googletest/reference/assertions.html#floating-point
##########
cpp/src/arrow/array/statistics.h:
##########
@@ -77,6 +77,12 @@ struct ARROW_EXPORT ArrayStatistics {
/// \brief The number of distinct values, may not be set
std::optional<int64_t> distinct_count = std::nullopt;
+ /// \brief The average size in bytes of a row in an array, may not be set
+ std::optional<double> average_byte_width = std::nullopt;
+
+ /// \brief Whether the average_byte_width value is exact or not
Review Comment:
```suggestion
/// \brief Whether the average size in bytes is exact or not
```
##########
cpp/src/arrow/array/statistics.h:
##########
@@ -131,7 +137,9 @@ struct ARROW_EXPORT ArrayStatistics {
bool Equals(const ArrayStatistics& other) const {
return null_count == other.null_count && distinct_count == other.distinct_count &&
min == other.min && is_min_exact == other.is_min_exact && max == other.max &&
- is_max_exact == other.is_max_exact;
+ is_max_exact == other.is_max_exact &&
+ average_byte_width == other.average_byte_width &&
Review Comment:
We should not use `==` for floating point numbers.
We need something like https://github.com/apache/arrow/blob/598938711a8376cbfdceaf5c77ab0fd5057e6c02/cpp/src/arrow/compare.cc#L85-L94 instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45639: [C++][Statistics] Add ARROW:average_byte_width:exact and ARROW:average_byte_width:approximate statistics attributes to arrow::ArrayStatistics [arrow]
Posted by "andishgar (via GitHub)" <gi...@apache.org>.
andishgar commented on code in PR #46385:
URL: https://github.com/apache/arrow/pull/46385#discussion_r2191589556
##########
cpp/src/arrow/array/statistics_test.cc:
##########
@@ -152,4 +163,19 @@ TEST_F(TestArrayStatisticsEqualityDoubleValue, ApproximateEquals) {
ASSERT_TRUE(statistics1_.Equals(statistics2_, options_.atol(1e-3).use_atol(true)));
}
+TEST_F(TestArrayStatisticsEqualityDoubleValue, DoubleAttributeCombination) {
+ statistics1_.max = 0.5;
+ ASSERT_FALSE(statistics1_.Equals(statistics2_));
+ statistics2_.max = 0.5;
+ ASSERT_TRUE(statistics1_.Equals(statistics2_));
+ statistics1_.min = 0.5;
+ ASSERT_FALSE(statistics1_.Equals(statistics2_));
+ statistics2_.min = 0.5;
Review Comment:
Sure, I'll go ahead and remove them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45639: [C++][Statistics] Add ARROW:average_byte_width:exact and ARROW:average_byte_width:approximate statistics attributes to arrow::ArrayStatistics [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #46385:
URL: https://github.com/apache/arrow/pull/46385#discussion_r2192280827
##########
cpp/src/arrow/array/statistics_test.cc:
##########
@@ -152,4 +171,11 @@ TEST_F(TestArrayStatisticsEqualityDoubleValue, ApproximateEquals) {
ASSERT_TRUE(statistics1_.Equals(statistics2_, options_.atol(1e-3).use_atol(true)));
}
+TEST_F(TestArrayStatisticsEqualityDoubleValue, DoubleAttributesCombination) {
Review Comment:
```suggestion
TEST_F(TestArrayStatisticsEqualityDoubleValue, AverageByteWidth) {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org