Minor: simplify DataSource statistics code #8172
Conversation
| } | ||
| } | ||
|
|
||
| /// If the given value is numerically lesser than the original minimum value, |
There was a problem hiding this comment.
|
|
||
| /// If the given value is numerically greater than the original maximum value, | ||
| /// return the new maximum value with appropriate exactness information. | ||
| fn set_max_if_greater( |
There was a problem hiding this comment.
There was a problem hiding this comment.
There is a similar difference here as well. Max values are conserved by relaxing the exactness when an absent statistic is read from the file.
| @@ -160,17 +159,6 @@ pub(crate) fn create_max_min_accs( | |||
| (max_values, min_values) | |||
| } | |||
|
|
|||
| fn add_row_stats( | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
I think they don't share the same behavior, but it didn't show up due to lack of testing. The add() function operates in the safest way; if one of the operands is absent, the result will also be absent. On the other hand, the remove() function keeps the non-absent value by changing its exactness (absent + exact(value) => inexact(value)).
|
cc @berkaysynnada and @ozankabak |
alamb
changed the title
There was a problem hiding this comment.
The functions such as add(), min(), and max(), which I initially introduced in the precision API, were designed to operate in the safest way. However, I observe that the common usage of these functions in the code is trying to preserve statistical values across parameter updates by converting them to inexact values. What are your thoughts on modifying the functionalities of the precision methods instead? I mean these precision methods' logic could be replaced by the functions you removed.
Thanks for the simplifications
| @@ -160,17 +159,6 @@ pub(crate) fn create_max_min_accs( | |||
| (max_values, min_values) | |||
| } | |||
|
|
|||
| fn add_row_stats( | |||
There was a problem hiding this comment.
I think they don't share the same behavior, but it didn't show up due to lack of testing. The add() function operates in the safest way; if one of the operands is absent, the result will also be absent. On the other hand, the remove() function keeps the non-absent value by changing its exactness (absent + exact(value) => inexact(value)).
|
|
||
| /// If the given value is numerically greater than the original maximum value, | ||
| /// return the new maximum value with appropriate exactness information. | ||
| fn set_max_if_greater( |
There was a problem hiding this comment.
There is a similar difference here as well. Max values are conserved by relaxing the exactness when an absent statistic is read from the file.
| } | ||
| } | ||
|
|
||
| /// If the given value is numerically lesser than the original minimum value, |
|
@berkaysynnada, good catch! Let's consider changing the |
This is an excellent catch @berkaysynnada -- thank you. My thoughts are that I would like to initially not change any behavior unless it is clearly a bug. So in this case, I will:
The reason to move the code into
I can not. One test would be to try to change |
| @@ -160,17 +159,6 @@ pub(crate) fn create_max_min_accs( | |||
| (max_values, min_values) | |||
| } | |||
|
|
|||
| fn add_row_stats( | |||
There was a problem hiding this comment.
It is interesting that there is very similar code in https://github.com/apache/arrow-datafusion/blob/a892300a5a56c97b5b4ddc9aa4a421aaf412d0fe/datafusion/core/src/datasource/file_format/parquet.rs#L503-L525 that uses add 🤔
There was a problem hiding this comment.
It seems like it is always adding exact values, so there should be no difference on the results whether it is used add() or estimated_add()
There was a problem hiding this comment.
How embarassing -- I even noted the lack of unit tests in this file on https://github.com/apache/arrow-datafusion/pull/7793/files#r1358805175 🤦
Sadly, while I was working on the changes, I noticed that in many parts where we use statistics, we don't test them enough. You have done really well starting the Epic. It definitely requires quite a bit of work. |
|
I'll keep working on statistics under the aegis of a different issue |
Which issue does this PR close?
This is part of #8078
Rationale for this change
I am trying to improve the handling of statistics by changing the internal representation, so I would like less to change.
My longer term goal is to keep all the calculation and comparison code for
Statisticswithin theStatisticsclass itself, which will make testing and changing implementation detailsWhat changes are included in this PR?
Precisionand use pre-existing methodsmin,maxandaddAre these changes tested?
Covered by existing tests
Are there any user-facing changes?
No