Skip to content
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

aggregate_statistics should only optimize MIN/MAX when relation is not empty #8914

Merged
merged 2 commits into from
Jan 20, 2024
Merged

aggregate_statistics should only optimize MIN/MAX when relation is not empty #8914

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b805195
Fix aggregate_statistics
viirya Jan 19, 2024
6073f46
Add more test
viirya Jan 19, 2024
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
62 changes: 42 additions & 20 deletions datafusion/core/src/physical_optimizer/aggregate_statistics.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,17 +197,28 @@ fn take_optimizable_min(
agg_expr: &dyn AggregateExpr,
stats: &Statistics,
) -> Option<(ScalarValue, String)> {
let col_stats = &stats.column_statistics;
if let Some(casted_expr) = agg_expr.as_any().downcast_ref::<expressions::Min>() {
if casted_expr.expressions().len() == 1 {
// TODO optimize with exprs other than Column
if let Some(col_expr) = casted_expr.expressions()[0]
.as_any()
.downcast_ref::<expressions::Column>()
if let Precision::Exact(num_rows) = &stats.num_rows {
if *num_rows > 0 {
Copy link
Member Author

@viirya viirya Jan 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We could optimize MIN/MAX if the relation is empty (i.e., return NULL directly). Since it is not the issue itself, I will leave it as follow up.

let col_stats = &stats.column_statistics;
if let Some(casted_expr) =
agg_expr.as_any().downcast_ref::<expressions::Min>()
{
if let Precision::Exact(val) = &col_stats[col_expr.index()].min_value {
if !val.is_null() {
return Some((val.clone(), casted_expr.name().to_string()));
if casted_expr.expressions().len() == 1 {
// TODO optimize with exprs other than Column
if let Some(col_expr) = casted_expr.expressions()[0]
.as_any()
.downcast_ref::<expressions::Column>()
{
if let Precision::Exact(val) =
&col_stats[col_expr.index()].min_value
{
if !val.is_null() {
return Some((
val.clone(),
casted_expr.name().to_string(),
));
}
}
}
}
}
Expand All @@ -221,17 +232,28 @@ fn take_optimizable_max(
agg_expr: &dyn AggregateExpr,
stats: &Statistics,
) -> Option<(ScalarValue, String)> {
let col_stats = &stats.column_statistics;
if let Some(casted_expr) = agg_expr.as_any().downcast_ref::<expressions::Max>() {
if casted_expr.expressions().len() == 1 {
// TODO optimize with exprs other than Column
if let Some(col_expr) = casted_expr.expressions()[0]
.as_any()
.downcast_ref::<expressions::Column>()
if let Precision::Exact(num_rows) = &stats.num_rows {
if *num_rows > 0 {
let col_stats = &stats.column_statistics;
if let Some(casted_expr) =
agg_expr.as_any().downcast_ref::<expressions::Max>()
{
if let Precision::Exact(val) = &col_stats[col_expr.index()].max_value {
if !val.is_null() {
return Some((val.clone(), casted_expr.name().to_string()));
if casted_expr.expressions().len() == 1 {
// TODO optimize with exprs other than Column
if let Some(col_expr) = casted_expr.expressions()[0]
.as_any()
.downcast_ref::<expressions::Column>()
{
if let Precision::Exact(val) =
&col_stats[col_expr.index()].max_value
{
if !val.is_null() {
return Some((
val.clone(),
casted_expr.name().to_string(),
));
}
}
}
}
}
Expand Down
34 changes: 34 additions & 0 deletions datafusion/sqllogictest/test_files/aggregate.slt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3260,3 +3260,37 @@ query I
select count(*) from (select count(*) a, count(*) b from (select 1));
----
1

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also add another test where there is a single row that doesn't actually pass the predicate?

Like

CREATE TABLE t(col0 INTEGER) as VALUES(2);
SELECT MIN(col0) FROM empty WHERE col0=1;
SELECT MAX(col0) FROM empty WHERE col0=1;

So the table is not actually empty but the filter removes it all

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea. Added that one.

# rule `aggregate_statistics` should not optimize MIN/MAX to wrong values on empty relation

statement ok
CREATE TABLE empty(col0 INTEGER);

query I
SELECT MIN(col0) FROM empty WHERE col0=1;
----
NULL

query I
SELECT MAX(col0) FROM empty WHERE col0=1;
----
NULL

statement ok
DROP TABLE empty;

statement ok
CREATE TABLE t(col0 INTEGER) as VALUES(2);

query I
SELECT MIN(col0) FROM t WHERE col0=1;
----
NULL

query I
SELECT MAX(col0) FROM t WHERE col0=1;
----
NULL

statement ok
DROP TABLE t;