fix(query): preserve IGNORE NULLS semantics in distributed queries (#9601)

* fix(query): skip LastRow pushdown for IGNORE NULLS

Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>

* fix(query): keep IGNORE NULLS aggregates above remote scans

Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>

* test(query): record IGNORE NULLS results before and after flush

Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>

* test(query): share IGNORE NULLS coverage across deployments

Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>

* test(query): refresh last-value plans after rebase

Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>

---------

Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>
This commit is contained in:
discord9
2026-10-26 10:25:25 +00:00
committed by GitHub
parent df8f6848cc
commit 0dab3aef21
7 changed files with 395 additions and 6 deletions
+70 -1
View File
@@ -32,7 +32,7 @@ use datafusion::functions_nested::expr_fn::make_array;
use datafusion::prelude::SessionContext;
use datafusion_common::tree_node::TreeNodeRecursion;
use datafusion_common::{ExprSchema, JoinType, ScalarValue, TableReference};
use datafusion_expr::expr::{Exists, ScalarFunction};
use datafusion_expr::expr::{Exists, NullTreatment, ScalarFunction};
use datafusion_expr::utils::split_conjunction;
use datafusion_expr::{
AggregateUDF, Expr, ExprSchemable as _, Extension, LogicalPlanBuilder, Operator, Subquery,
@@ -2484,6 +2484,75 @@ fn date_bin_ts_group_by() {
assert_eq!(expected, result.to_string());
}
#[test]
fn ignore_nulls_aggregate_stays_on_frontend() {
use datafusion::functions_aggregate::first_last::last_value;
let table = TestTable::table_with_name(0, "t".to_string());
let source = Arc::new(DefaultTableSource::new(Arc::new(
DfTableProviderAdapter::new(table),
)));
let input = LogicalPlanBuilder::scan_with_filters("t", source, None, vec![])
.unwrap()
.build()
.unwrap();
for (group_by, null_treatment) in [
(
vec![col("pk1"), col("pk2")],
Some(NullTreatment::IgnoreNulls),
),
(vec![col("number")], Some(NullTreatment::IgnoreNulls)),
(vec![col("pk1"), col("pk2")], None),
(
vec![col("pk1"), col("pk2")],
Some(NullTreatment::RespectNulls),
),
] {
let mut ignored = last_value(col("number"), vec![]);
let Expr::AggregateFunction(func) = &mut ignored else {
panic!("expected aggregate expression");
};
func.params.null_treatment = null_treatment;
let aggregate_exprs = if null_treatment == Some(NullTreatment::IgnoreNulls) {
let mut ordinary = last_value(col("number"), vec![]);
let Expr::AggregateFunction(func) = &mut ordinary else {
panic!("expected aggregate expression");
};
func.params.null_treatment = None;
vec![ordinary.alias("ordinary"), ignored.alias("ignored")]
} else {
vec![ignored]
};
let plan = LogicalPlanBuilder::from(input.clone())
.aggregate(group_by, aggregate_exprs)
.unwrap()
.build()
.unwrap();
let analyzed = DistPlannerAnalyzer {}
.analyze(plan.clone(), &ConfigOptions::default())
.unwrap();
if null_treatment == Some(NullTreatment::IgnoreNulls) {
let LogicalPlan::Aggregate(aggregate) = &analyzed else {
panic!("IGNORE NULLS aggregate must remain on the frontend: {analyzed}");
};
let remote_input = find_merge_scan(&analyzed).unwrap().input();
assert!(
!matches!(remote_input, LogicalPlan::Aggregate(_)),
"remote input contains aggregate: {remote_input}"
);
let Some(func) =
common_function::aggrs::aggr_wrapper::get_aggr_func(&aggregate.aggr_expr[1])
else {
unreachable!();
};
assert_eq!(Some(NullTreatment::IgnoreNulls), func.params.null_treatment);
} else {
let remote_input = find_merge_scan(&analyzed).unwrap().input();
assert!(matches!(remote_input, LogicalPlan::Aggregate(_)));
}
}
}
#[test]
fn test_last_value_order_by() {
init_default_ut_logging();
+12 -1
View File
@@ -15,10 +15,13 @@
use std::collections::HashSet;
use std::sync::Arc;
use common_function::aggrs::aggr_wrapper::{StateMergeHelper, is_all_aggr_exprs_steppable};
use common_function::aggrs::aggr_wrapper::{
StateMergeHelper, get_aggr_func, is_all_aggr_exprs_steppable,
};
use common_telemetry::debug;
use datafusion::error::Result as DfResult;
use datafusion_common::tree_node::{TreeNode, TreeNodeRecursion};
use datafusion_expr::expr::NullTreatment;
use datafusion_expr::{Expr, LogicalPlan, UserDefinedLogicalNode};
use promql::extension_plan::{
EmptyMetric, InstantManipulate, RangeManipulate, SeriesDivide, SeriesNormalize,
@@ -116,6 +119,14 @@ impl Categorizer {
LogicalPlan::Filter(filter) => Self::check_expr(&filter.predicate),
LogicalPlan::Window(_) => Commutativity::Unimplemented,
LogicalPlan::Aggregate(aggr) => {
// The pinned Substrait codec drops IGNORE NULLS, so keep these aggregates on the frontend.
if aggr.aggr_expr.iter().any(|expr| {
get_aggr_func(expr).is_some_and(|func| {
func.params.null_treatment == Some(NullTreatment::IgnoreNulls)
})
}) {
return Ok(Commutativity::NonCommutative);
}
// The state/merge split maps each group expression to one output column,
// which doesn't hold for grouping sets.
let has_grouping_set = aggr
+50 -4
View File
@@ -22,7 +22,7 @@ use common_recordbatch::filter::SimpleFilterEvaluator;
use datafusion::datasource::DefaultTableSource;
use datafusion_common::tree_node::{Transformed, TreeNodeRewriter};
use datafusion_common::{Column, Result};
use datafusion_expr::expr::Sort;
use datafusion_expr::expr::{NullTreatment, Sort};
use datafusion_expr::{Expr, LogicalPlan, utils};
use datafusion_optimizer::{OptimizerConfig, OptimizerRule};
use datatypes::arrow::datatypes::{DataType, TimeUnit as ArrowTimeUnit};
@@ -413,6 +413,8 @@ impl ScanHintRewriter {
&& func.func.name() != aggr_state_func_name("last_value"))
|| func.params.filter.is_some()
|| func.params.distinct
// A shared final-row selector would discard earlier nonnull values.
|| func.params.null_treatment == Some(NullTreatment::IgnoreNulls)
{
return None;
}
@@ -455,7 +457,7 @@ mod test {
use datafusion::prelude::JoinType;
use datafusion_common::tree_node::TreeNodeRecursion;
use datafusion_expr::expr::{
AggregateFunction, AggregateFunctionParams, Cast, WindowFunctionParams,
AggregateFunction, AggregateFunctionParams, Cast, NullTreatment, WindowFunctionParams,
};
use datafusion_expr::expr_fn::scalar_subquery;
use datafusion_expr::{Extension, LogicalPlan, LogicalPlanBuilder, col, lit};
@@ -561,6 +563,13 @@ mod test {
}
fn last_value_aggregate(input: LogicalPlan) -> LogicalPlan {
last_value_aggregate_with_null_treatments(input, [None, None])
}
fn last_value_aggregate_with_null_treatments(
input: LogicalPlan,
null_treatments: [Option<NullTreatment>; 2],
) -> LogicalPlan {
let aggregate = LogicalPlanBuilder::from(input)
.aggregate(
vec![col("k0")],
@@ -576,7 +585,7 @@ mod test {
asc: true,
nulls_first: true,
}],
null_treatment: None,
null_treatment: null_treatments[0],
},
}),
Expr::AggregateFunction(AggregateFunction {
@@ -590,7 +599,7 @@ mod test {
asc: true,
nulls_first: true,
}],
null_treatment: None,
null_treatment: null_treatments[1],
},
}),
],
@@ -901,6 +910,43 @@ mod test {
}
}
#[test]
fn last_value_null_treatment_controls_selector() {
let provider = Arc::new(mock_table_provider(RegionId::new(1, 1)));
let context = OptimizerContext::default();
for treatment in [None, Some(NullTreatment::RespectNulls)] {
let rewritten = ScanHintRule
.rewrite(
last_value_aggregate_with_null_treatments(
scan_plan(provider.clone(), "t"),
[treatment, treatment],
),
&context,
)
.unwrap()
.data;
assert_eq!(
scan_requests(&rewritten)[0].series_row_selector,
Some(TimeSeriesRowSelector::LastRow { after_merge: false })
);
}
// One IGNORE NULLS member prevents the shared scan selector for every
// aggregate in the group, including the member that respects nulls.
let rewritten = ScanHintRule
.rewrite(
last_value_aggregate_with_null_treatments(
scan_plan(provider, "t"),
[None, Some(NullTreatment::IgnoreNulls)],
),
&context,
)
.unwrap()
.data;
assert_eq!(scan_requests(&rewritten)[0].series_row_selector, None);
}
#[test]
fn single_evaluation_last_value_aggregate_keeps_legacy_selector() {
let provider = Arc::new(mock_table_provider(RegionId::new(1, 1)));
@@ -0,0 +1,142 @@
create table last_value_ignore_nulls (
ts timestamp time index,
host string primary key,
v1 double,
v2 double,
);
Affected Rows: 0
insert into last_value_ignore_nulls values
(1000, 'a', 42.0, null),
(2000, 'a', null, 20.0),
(1000, 'b', null, null),
(2000, 'b', null, null);
Affected Rows: 4
select
host,
last_value(v1 order by ts) ignore nulls as v1_ignore,
last_value(v2 order by ts) ignore nulls as v2_ignore,
last_value(v1 order by ts) as v1_default,
last_value(v1 order by ts) respect nulls as v1_respect
from last_value_ignore_nulls
group by host
order by host;
+------+-----------+-----------+------------+------------+
| host | v1_ignore | v2_ignore | v1_default | v1_respect |
+------+-----------+-----------+------------+------------+
| a | 42.0 | 20.0 | | |
| b | | | | |
+------+-----------+-----------+------------+------------+
-- Default and RESPECT NULLS return the same results in both modes.
select
host,
last_value(v1 order by ts) as v1_default,
last_value(v1 order by ts) respect nulls as v1_respect
from last_value_ignore_nulls
group by host
order by host;
+------+------------+------------+
| host | v1_default | v1_respect |
+------+------------+------------+
| a | | |
| b | | |
+------+------------+------------+
-- The IGNORE NULLS member rejects the shared selector, including when it is second.
-- SQLNESS REPLACE (-+) -
-- SQLNESS REPLACE (\s\s+) _
-- SQLNESS REPLACE (elapsed_compute.*) REDACTED
-- SQLNESS REPLACE (peers.*) REDACTED
-- SQLNESS REPLACE (RoundRobinBatch.*) REDACTED
-- SQLNESS REPLACE (metrics.*) REDACTED
-- SQLNESS REPLACE (partitioning.*) REDACTED
-- SQLNESS REPLACE region=\d+\(\d+,\s+\d+\) region=REDACTED
-- SQLNESS REPLACE \\?"(dyn_filters|time_range|file_summary|max_part_range)\\?":.* "$1":REDACTED
explain analyze select
last_value(v1 order by ts),
last_value(v2 order by ts) ignore nulls
from last_value_ignore_nulls
group by host;
+-+-+-+
| stage | node | plan_|
+-+-+-+
| 0_| 0_|_ProjectionExec: expr=[last_value(last_value_ignore_nulls.v1) ORDER BY [last_value_ignore_nulls.ts ASC NULLS LAST]@1 as last_value(last_value_ignore_nulls.v1) ORDER BY [last_value_ignore_nulls.ts ASC NULLS LAST], last_value(last_value_ignore_nulls.v2) IGNORE NULLS ORDER BY [last_value_ignore_nulls.ts ASC NULLS LAST]@2 as last_value(last_value_ignore_nulls.v2) IGNORE NULLS ORDER BY [last_value_ignore_nulls.ts ASC NULLS LAST]] REDACTED
|_|_|_AggregateExec: mode=FinalPartitioned, gby=[host@0 as host], aggr=[last_value(last_value_ignore_nulls.v1) ORDER BY [last_value_ignore_nulls.ts ASC NULLS LAST], last_value(last_value_ignore_nulls.v2) IGNORE NULLS ORDER BY [last_value_ignore_nulls.ts ASC NULLS LAST]] REDACTED
|_|_|_RepartitionExec: REDACTED
|_|_|_AggregateExec: mode=Partial, gby=[host@1 as host], aggr=[last_value(last_value_ignore_nulls.v1) ORDER BY [last_value_ignore_nulls.ts ASC NULLS LAST], last_value(last_value_ignore_nulls.v2) IGNORE NULLS ORDER BY [last_value_ignore_nulls.ts ASC NULLS LAST]] REDACTED
|_|_|_RepartitionExec: REDACTED
|_|_|_MergeScanExec: REDACTED
|_|_|_|
| 1_| 0_|_CooperativeExec REDACTED
|_|_|_SeqScan: region=REDACTED, "partition_count":{"count":1, "mem_ranges":1, "files":0, "file_ranges":0}, "projection": ["ts", "host", "v1", "v2"], "time_range":REDACTED
|_|_|_|
|_|_| Total rows: 2_|
+-+-+-+
admin flush_table('last_value_ignore_nulls');
+----------------------------------------------+
| ADMIN flush_table('last_value_ignore_nulls') |
+----------------------------------------------+
| 0 |
+----------------------------------------------+
select
host,
last_value(v1 order by ts) ignore nulls as v1_ignore,
last_value(v2 order by ts) ignore nulls as v2_ignore,
last_value(v1 order by ts) as v1_default,
last_value(v1 order by ts) respect nulls as v1_respect
from last_value_ignore_nulls
group by host
order by host;
+------+-----------+-----------+------------+------------+
| host | v1_ignore | v2_ignore | v1_default | v1_respect |
+------+-----------+-----------+------------+------------+
| a | 42.0 | 20.0 | | |
| b | | | | |
+------+-----------+-----------+------------+------------+
-- SQLNESS REPLACE (-+) -
-- SQLNESS REPLACE (\s\s+) _
-- SQLNESS REPLACE (elapsed_compute.*) REDACTED
-- SQLNESS REPLACE (peers.*) REDACTED
-- SQLNESS REPLACE (RoundRobinBatch.*) REDACTED
-- SQLNESS REPLACE (metrics.*) REDACTED
-- SQLNESS REPLACE (partitioning.*) REDACTED
-- SQLNESS REPLACE region=\d+\(\d+,\s+\d+\) region=REDACTED
-- SQLNESS REPLACE \\?"(dyn_filters|time_range|file_summary|max_part_range)\\?":.* "$1":REDACTED
explain analyze select
last_value(v1 order by ts),
last_value(v2 order by ts) ignore nulls
from last_value_ignore_nulls
group by host;
+-+-+-+
| stage | node | plan_|
+-+-+-+
| 0_| 0_|_ProjectionExec: expr=[last_value(last_value_ignore_nulls.v1) ORDER BY [last_value_ignore_nulls.ts ASC NULLS LAST]@1 as last_value(last_value_ignore_nulls.v1) ORDER BY [last_value_ignore_nulls.ts ASC NULLS LAST], last_value(last_value_ignore_nulls.v2) IGNORE NULLS ORDER BY [last_value_ignore_nulls.ts ASC NULLS LAST]@2 as last_value(last_value_ignore_nulls.v2) IGNORE NULLS ORDER BY [last_value_ignore_nulls.ts ASC NULLS LAST]] REDACTED
|_|_|_AggregateExec: mode=FinalPartitioned, gby=[host@0 as host], aggr=[last_value(last_value_ignore_nulls.v1) ORDER BY [last_value_ignore_nulls.ts ASC NULLS LAST], last_value(last_value_ignore_nulls.v2) IGNORE NULLS ORDER BY [last_value_ignore_nulls.ts ASC NULLS LAST]] REDACTED
|_|_|_RepartitionExec: REDACTED
|_|_|_AggregateExec: mode=Partial, gby=[host@1 as host], aggr=[last_value(last_value_ignore_nulls.v1) ORDER BY [last_value_ignore_nulls.ts ASC NULLS LAST], last_value(last_value_ignore_nulls.v2) IGNORE NULLS ORDER BY [last_value_ignore_nulls.ts ASC NULLS LAST]] REDACTED
|_|_|_RepartitionExec: REDACTED
|_|_|_MergeScanExec: REDACTED
|_|_|_|
| 1_| 0_|_CooperativeExec REDACTED
|_|_|_SeqScan: region=REDACTED, "partition_count":{"count":1, "mem_ranges":0, "files":1, "file_ranges":1}, "projection": ["ts", "host", "v1", "v2"], "time_range":REDACTED
|_|_|_|
|_|_| Total rows: 2_|
+-+-+-+
drop table last_value_ignore_nulls;
Affected Rows: 0
@@ -0,0 +1,76 @@
create table last_value_ignore_nulls (
ts timestamp time index,
host string primary key,
v1 double,
v2 double,
);
insert into last_value_ignore_nulls values
(1000, 'a', 42.0, null),
(2000, 'a', null, 20.0),
(1000, 'b', null, null),
(2000, 'b', null, null);
select
host,
last_value(v1 order by ts) ignore nulls as v1_ignore,
last_value(v2 order by ts) ignore nulls as v2_ignore,
last_value(v1 order by ts) as v1_default,
last_value(v1 order by ts) respect nulls as v1_respect
from last_value_ignore_nulls
group by host
order by host;
-- Default and RESPECT NULLS return the same results in both modes.
select
host,
last_value(v1 order by ts) as v1_default,
last_value(v1 order by ts) respect nulls as v1_respect
from last_value_ignore_nulls
group by host
order by host;
-- The IGNORE NULLS member rejects the shared selector, including when it is second.
-- SQLNESS REPLACE (-+) -
-- SQLNESS REPLACE (\s\s+) _
-- SQLNESS REPLACE (elapsed_compute.*) REDACTED
-- SQLNESS REPLACE (peers.*) REDACTED
-- SQLNESS REPLACE (RoundRobinBatch.*) REDACTED
-- SQLNESS REPLACE (metrics.*) REDACTED
-- SQLNESS REPLACE (partitioning.*) REDACTED
-- SQLNESS REPLACE region=\d+\(\d+,\s+\d+\) region=REDACTED
-- SQLNESS REPLACE \\?"(dyn_filters|time_range|file_summary|max_part_range)\\?":.* "$1":REDACTED
explain analyze select
last_value(v1 order by ts),
last_value(v2 order by ts) ignore nulls
from last_value_ignore_nulls
group by host;
admin flush_table('last_value_ignore_nulls');
select
host,
last_value(v1 order by ts) ignore nulls as v1_ignore,
last_value(v2 order by ts) ignore nulls as v2_ignore,
last_value(v1 order by ts) as v1_default,
last_value(v1 order by ts) respect nulls as v1_respect
from last_value_ignore_nulls
group by host
order by host;
-- SQLNESS REPLACE (-+) -
-- SQLNESS REPLACE (\s\s+) _
-- SQLNESS REPLACE (elapsed_compute.*) REDACTED
-- SQLNESS REPLACE (peers.*) REDACTED
-- SQLNESS REPLACE (RoundRobinBatch.*) REDACTED
-- SQLNESS REPLACE (metrics.*) REDACTED
-- SQLNESS REPLACE (partitioning.*) REDACTED
-- SQLNESS REPLACE region=\d+\(\d+,\s+\d+\) region=REDACTED
-- SQLNESS REPLACE \\?"(dyn_filters|time_range|file_summary|max_part_range)\\?":.* "$1":REDACTED
explain analyze select
last_value(v1 order by ts),
last_value(v2 order by ts) ignore nulls
from last_value_ignore_nulls
group by host;
drop table last_value_ignore_nulls;
@@ -146,6 +146,36 @@ explain analyze
|_|_| Total rows: 1_|
+-+-+-+
-- SQLNESS REPLACE (-+) -
-- SQLNESS REPLACE (\s\s+) _
-- SQLNESS REPLACE (elapsed_compute.*) REDACTED
-- SQLNESS REPLACE (peers.*) REDACTED
-- SQLNESS REPLACE (RoundRobinBatch.*) REDACTED
-- SQLNESS REPLACE (metrics.*) REDACTED
-- SQLNESS REPLACE (partitioning.*) REDACTED
-- SQLNESS REPLACE region=\d+\(\d+,\s+\d+\) region=REDACTED
-- SQLNESS REPLACE \\?"(dyn_filters|time_range|file_summary|max_part_range)\\?":.* "$1":REDACTED
explain analyze select
last_value(val order by ts),
last_value(val order by ts) respect nulls
from t
group by host;
+-+-+-+
| stage | node | plan_|
+-+-+-+
| 0_| 0_|_CooperativeExec REDACTED
|_|_|_MergeScanExec: REDACTED
|_|_|_|
| 1_| 0_|_ProjectionExec: expr=[last_value(t.val) ORDER BY [t.ts ASC NULLS LAST]@1 as last_value(t.val) ORDER BY [t.ts ASC NULLS LAST], last_value(t.val) RESPECT NULLS ORDER BY [t.ts ASC NULLS LAST]@2 as last_value(t.val) RESPECT NULLS ORDER BY [t.ts ASC NULLS LAST]] REDACTED
|_|_|_AggregateExec: mode=FinalPartitioned, gby=[host@0 as host], aggr=[last_value(t.val) ORDER BY [t.ts ASC NULLS LAST], last_value(t.val) RESPECT NULLS ORDER BY [t.ts ASC NULLS LAST]] REDACTED
|_|_|_RepartitionExec: REDACTED
|_|_|_AggregateExec: mode=Partial, gby=[host@1 as host], aggr=[last_value(t.val) ORDER BY [t.ts ASC NULLS LAST], last_value(t.val) RESPECT NULLS ORDER BY [t.ts ASC NULLS LAST]] REDACTED
|_|_|_SeqScan: region=REDACTED, "partition_count":{"count":1, "mem_ranges":1, "files":0, "file_ranges":0}, "selector":"LastRow { after_merge: false }", "projection": ["ts", "host", "val"], "time_range":REDACTED
|_|_|_|
|_|_| Total rows: 4_|
+-+-+-+
drop table t;
Affected Rows: 0
@@ -72,4 +72,19 @@ explain select last_value(ts order by ts) from t;
explain analyze
select last_value(ts order by ts) from t;
-- SQLNESS REPLACE (-+) -
-- SQLNESS REPLACE (\s\s+) _
-- SQLNESS REPLACE (elapsed_compute.*) REDACTED
-- SQLNESS REPLACE (peers.*) REDACTED
-- SQLNESS REPLACE (RoundRobinBatch.*) REDACTED
-- SQLNESS REPLACE (metrics.*) REDACTED
-- SQLNESS REPLACE (partitioning.*) REDACTED
-- SQLNESS REPLACE region=\d+\(\d+,\s+\d+\) region=REDACTED
-- SQLNESS REPLACE \\?"(dyn_filters|time_range|file_summary|max_part_range)\\?":.* "$1":REDACTED
explain analyze select
last_value(val order by ts),
last_value(val order by ts) respect nulls
from t
group by host;
drop table t;