Skip to content

GH-46395: [C++][Statistics] Use EqualOptions for min and max in arrow::ArrayStatistics::Equals() - #46422

Merged
kou merged 11 commits into
apache:mainfrom
andishgar:correct-statistics-equal-method
Jun 7, 2025
Merged

GH-46395: [C++][Statistics] Use EqualOptions for min and max in arrow::ArrayStatistics::Equals()#46422
kou merged 11 commits into
apache:mainfrom
andishgar:correct-statistics-equal-method

Conversation

@andishgar

@andishgarandishgar commented May 13, 2025•
edited by kou
Loading

Copy link
Copy Markdown
Contributor

Rationale for this change

arrow::ArrayStatistics::Equals does not handle double values for ArrayStatistics::ValueType correctly

What changes are included in this PR?

Add arrow::EqualOptions to arrow::ArrayStatistics::Eqauls()
Add arrow::ArrayStatisticsEqauls()
Add EqualOptions::use_atol_
Add EqualOptions::use_atol()
Add EqualOptions::use_atol(bool v)

Are these changes tested?

Yes, I ran the relevant unit tests.

Are there any user-facing changes?

Yes.
Add arrow::ArrayStatisticsEqauls()
Add EqualOptions::use_atol()
Add EqualOptions::use_atol(bool v)

@andishgar
andishgar marked this pull request as draft May 13, 2025 09:25
@andishgar
andishgar marked this pull request as ready for review May 13, 2025 12:44
@andishgar

andishgar commented May 13, 2025

Copy link
Copy Markdown
ContributorAuthor

@kou what is your view on arrow::ArrayStatistics::ApproximateEquals? If we aim for consistency with the Arrow C++ implementation, it seems this method should be part of it.

@koukou changed the title GH-46395: [C++][Statistics]: Correct the Equal method for min and max in arrow::ArrayStatisticsGH-46395: [C++][Statistics] Correct the Equal method for min and max in arrow::ArrayStatisticsMay 13, 2025
@kou

kou commented May 14, 2025

Copy link
Copy Markdown
Member

It may be better that we add is_approximate to EqualOptions than adding ApproximateEquals.

Could you start a discussion for this like #46396 ?

@andishgar

andishgar commented May 14, 2025

Copy link
Copy Markdown
ContributorAuthor

I will open an issue for discussion soon

@andishgarandishgar mentioned this pull request May 14, 2025
Closed
@andishgar
andishgarforce-pushed the correct-statistics-equal-method branch from afb535b to 663b645CompareMay 30, 2025 12:16
@andishgar
andishgar marked this pull request as draft May 30, 2025 12:48
@andishgar
andishgar marked this pull request as ready for review May 30, 2025 13:34
@andishgar
andishgar marked this pull request as draft May 30, 2025 13:57
@andishgar
andishgar marked this pull request as ready for review May 30, 2025 18:04
@andishgar

andishgar commented May 30, 2025

Copy link
Copy Markdown
ContributorAuthor

@kou
I remove ApproximateEqauls and the Pull Request is ready for review

kou
kou reviewed May 31, 2025
View reviewed changes
Comment threadcpp/src/arrow/array/statistics.h Outdated
Comment threadcpp/src/arrow/array/statistics.h Outdated
@github-actionsgithub-actionsBot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels May 31, 2025
@andishgar

andishgar commented May 31, 2025

Copy link
Copy Markdown
ContributorAuthor

Thank you for your feedback, @kou — I'll look into it

@andishgar
andishgarforce-pushed the correct-statistics-equal-method branch from 3dbecf1 to a362b94CompareJune 1, 2025 11:34
@github-actionsgithub-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jun 1, 2025
@andishgar
andishgarforce-pushed the correct-statistics-equal-method branch from a362b94 to 1d6812dCompareJune 1, 2025 11:47
@andishgar
andishgar marked this pull request as draft June 1, 2025 11:49
@andishgar
andishgar marked this pull request as ready for review June 1, 2025 13:57
@andishgar
andishgar requested a review from kouJune 1, 2025 13:58
kou
kou reviewed Jun 2, 2025
View reviewed changes
Comment threadcpp/src/arrow/array/statistics.cc Outdated
Comment threadcpp/src/arrow/array/statistics.h Outdated
Comment threadcpp/src/arrow/array/statistics.h Outdated
Comment threadcpp/src/arrow/array/statistics.h Outdated
Comment threadcpp/src/arrow/array/statistics.h Outdated
Comment threadcpp/src/arrow/array/statistics_test.cc Outdated
Comment threadcpp/src/arrow/array/statistics_test.cc Outdated
Comment threadcpp/src/arrow/compare.h Outdated
@github-actionsgithub-actionsBot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Jun 2, 2025
@andishgar

andishgar commented Jun 2, 2025

Copy link
Copy Markdown
ContributorAuthor

Thank you for your feedback, @kou — I'll look into it

@github-actionsgithub-actionsBot added the awaiting changes Awaiting changes label Jun 4, 2025
kou
kou reviewed Jun 5, 2025
View reviewed changes
Comment threadcpp/src/arrow/array/statistics_test.cc Outdated
Comment threadcpp/src/arrow/array/statistics_test.cc Outdated
Comment threadcpp/src/arrow/array/statistics_test.cc Outdated
Comment threadcpp/src/arrow/array/statistics_test.cc Outdated
Comment threadcpp/src/arrow/compare.cc Outdated
Comment threadcpp/src/arrow/compare.h Outdated
@andishgar

andishgar commented Jun 5, 2025

Copy link
Copy Markdown
ContributorAuthor

@kou I apologize for my last commit — it seems I forgot to apply some of the notes you mentioned.

@andishgar
andishgarforce-pushed the correct-statistics-equal-method branch from 3ff6117 to d661877CompareJune 6, 2025 13:54
@github-actionsgithub-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jun 6, 2025
andishgar added 11 commits June 7, 2025 01:47
@andishgar
add Approximate equal
3374ac3
@andishgar
use correct format for double values
168af2c
@andishgar
remove ApproximateEquals method
cd47933
@andishgar
Add is_approximate
3cc2f93
@andishgar
Correct comments
5560be3
@andishgar
Enable allow_atol in Equal options and add operator == and !=
292d2af
@andishgar
Apply Kou suggestion
8d26a40
replace allow_atol to use_atol with relevant changes
move type alias declaration for ValueType into anonymous namesapce
comment == and != for operator and remove default options
clear comments test and comment regarding different ValueType
breakdown TestEqualityDoubleValues test case into several test cases
@andishgar
Change the way that equality is implemented
fc5697c
@andishgar
Apply Kou suggestion
ffde3c5
@andishgar
fix space, apply unified naming for fixture,
2a0760f
@andishgar
fix minor linting
e06599f
@andishgar
andishgarforce-pushed the correct-statistics-equal-method branch from d661877 to e06599fCompareJune 6, 2025 22:18
@andishgar
andishgar requested a review from kouJune 7, 2025 06:41
kou
kou approved these changes Jun 7, 2025
View reviewed changes

@koukou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1

@koukou changed the title GH-46395: [C++][Statistics] Correct the Equal method for min and max in arrow::ArrayStatisticsGH-46395: [C++][Statistics] Use EqualOptions for min and max in arrow::ArrayStatistics::Equals()Jun 7, 2025
@github-actionsgithub-actionsBot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Jun 7, 2025
@kou
kou merged commit 0e5249b into apache:mainJun 7, 2025
@koukou removed the awaiting merge Awaiting merge label Jun 7, 2025
@koukou mentioned this pull request Jun 7, 2025
Closed
@andishgar
andishgar deleted the correct-statistics-equal-method branch June 7, 2025 23:04
@conbench-apache-arrow

conbench-apache-arrowBot commented Jun 8, 2025

Copy link
Copy Markdown

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 0e5249b.

There were 72 benchmark results with an error:

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 4 possible false positives for unstable benchmarks that are known to sometimes produce them.

@andishgarandishgar mentioned this pull request Jun 10, 2025
Closed
alinaliBQ pushed a commit to Bit-Quill/arrow that referenced this pull request Jun 17, 2025
@andishgar@alinaliBQ
apacheGH-46395: [C++][Statistics] Use EqualOptions for min and max in…
6350435
… arrow::ArrayStatistics::Equals() (apache#46422)
### Rationale for this change
`arrow::ArrayStatistics::Equals` does not handle double values for `ArrayStatistics::ValueType` correctly
### What changes are included in this PR?
Add `arrow::EqualOptions` to `arrow::ArrayStatistics::Eqauls()`
Add `arrow::ArrayStatisticsEqauls()`
Add `EqualOptions::use_atol_`
Add `EqualOptions::use_atol()`
Add `EqualOptions::use_atol(bool v)`
### Are these changes tested?
Yes, I ran the relevant unit tests.
### Are there any user-facing changes?
Yes.
Add `arrow::ArrayStatisticsEqauls()`
Add `EqualOptions::use_atol()`
Add `EqualOptions::use_atol(bool v)`
* GitHub Issue: apache#46395
Authored-by: Arash Andishgar <arashandishgar1@gmail.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@koukoukou approved these changes

Assignees

No one assigned

Labels

Component: C++

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@andishgar@kou