Skip to content

Minor: simplify DataSource statistics code #8172

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

Closed
wants to merge 1 commit into from
Closed

Minor: simplify DataSource statistics code #8172

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
68 changes: 5 additions & 63 deletions datafusion/core/src/datasource/statistics.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,10 +83,9 @@ pub async fn get_statistics_with_limit(
// counts across all the files in question. If any file does not
// provide any information or provides an inexact value, we demote
// the statistic precision to inexact.
num_rows = add_row_stats(file_stats.num_rows, num_rows);
num_rows = num_rows.add(&file_stats.num_rows);

total_byte_size =
add_row_stats(file_stats.total_byte_size, total_byte_size);
total_byte_size = total_byte_size.add(&file_stats.total_byte_size);

(null_counts, max_values, min_values) = multiunzip(
izip!(
Expand All @@ -108,9 +107,9 @@ pub async fn get_statistics_with_limit(
min_value,
)| {
(
add_row_stats(file_nc, null_count),
set_max_if_greater(file_max, max_value),
set_min_if_lesser(file_min, min_value),
file_nc.add(&null_count),
file_max.max(&max_value),
file_min.min(&min_value),
)
},
),
Expand Down Expand Up @@ -160,17 +159,6 @@ pub(crate) fn create_max_min_accs(
(max_values, min_values)
}

fn add_row_stats(
Copy link
Contributor Author

@alamb alamb Nov 14, 2023

Choose a reason for hiding this comment

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

This is the same as https://github.com/apache/arrow-datafusion/blob/eef654c3b0c22b1f845b1441320b8bb718ddd605/datafusion/common/src/stats.rs#L119-L126

Copy link
Contributor

@berkaysynnada berkaysynnada Nov 15, 2023

Choose a reason for hiding this comment

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

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)).

Copy link
Contributor Author

@alamb alamb Nov 15, 2023

Choose a reason for hiding this comment

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

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 🤔

Copy link
Contributor

@berkaysynnada berkaysynnada Nov 16, 2023

Choose a reason for hiding this comment

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

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()

file_num_rows: Precision<usize>,
num_rows: Precision<usize>,
) -> Precision<usize> {
match (file_num_rows, &num_rows) {
(Precision::Absent, _) => num_rows.to_inexact(),
(lhs, Precision::Absent) => lhs.to_inexact(),
(lhs, rhs) => lhs.add(rhs),
}
}

pub(crate) fn get_col_stats_vec(
null_counts: Vec<Precision<usize>>,
max_values: Vec<Precision<ScalarValue>>,
Expand Down Expand Up @@ -211,49 +199,3 @@ pub(crate) fn get_col_stats(
})
.collect()
}

/// 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(
Copy link
Contributor Author

@alamb alamb Nov 14, 2023

Choose a reason for hiding this comment

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

This is the same as https://github.com/apache/arrow-datafusion/blob/eef654c3b0c22b1f845b1441320b8bb718ddd605/datafusion/common/src/stats.rs#L75-L87

Copy link
Contributor

@berkaysynnada berkaysynnada Nov 15, 2023

Choose a reason for hiding this comment

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

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.

max_nominee: Precision<ScalarValue>,
max_values: Precision<ScalarValue>,
) -> Precision<ScalarValue> {
match (&max_values, &max_nominee) {
(Precision::Exact(val1), Precision::Exact(val2)) if val1 < val2 => max_nominee,
(Precision::Exact(val1), Precision::Inexact(val2))
| (Precision::Inexact(val1), Precision::Inexact(val2))
| (Precision::Inexact(val1), Precision::Exact(val2))
if val1 < val2 =>
{
max_nominee.to_inexact()
}
(Precision::Exact(_), Precision::Absent) => max_values.to_inexact(),
(Precision::Absent, Precision::Exact(_)) => max_nominee.to_inexact(),
(Precision::Absent, Precision::Inexact(_)) => max_nominee,
(Precision::Absent, Precision::Absent) => Precision::Absent,
_ => max_values,
}
}

/// If the given value is numerically lesser than the original minimum value,
Copy link
Contributor Author

@alamb alamb Nov 14, 2023

Choose a reason for hiding this comment

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

This is the same as https://github.com/apache/arrow-datafusion/blob/eef654c3b0c22b1f845b1441320b8bb718ddd605/datafusion/common/src/stats.rs#L92-L103

Copy link
Contributor

@berkaysynnada berkaysynnada Nov 15, 2023

Choose a reason for hiding this comment

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

Same here

/// return the new minimum value with appropriate exactness information.
fn set_min_if_lesser(
min_nominee: Precision<ScalarValue>,
min_values: Precision<ScalarValue>,
) -> Precision<ScalarValue> {
match (&min_values, &min_nominee) {
(Precision::Exact(val1), Precision::Exact(val2)) if val1 > val2 => min_nominee,
(Precision::Exact(val1), Precision::Inexact(val2))
| (Precision::Inexact(val1), Precision::Inexact(val2))
| (Precision::Inexact(val1), Precision::Exact(val2))
if val1 > val2 =>
{
min_nominee.to_inexact()
}
(Precision::Exact(_), Precision::Absent) => min_values.to_inexact(),
(Precision::Absent, Precision::Exact(_)) => min_nominee.to_inexact(),
(Precision::Absent, Precision::Inexact(_)) => min_nominee,
(Precision::Absent, Precision::Absent) => Precision::Absent,
_ => min_values,
}
}