Skip to content

Commit ebd2a93

Browse files
zzylolclaude
andcommitted
fix(ir): keep ambiguous columns and untyped-equal literals out of selections
From an independent review of the derivation: - a column whose (table, name) is not unique in the agg child's schema cannot be named in a selection, so its conjuncts stay in the definition; - value sets compare literals by typed order, so 1 and 1.0 (or NaN) are never proven different; - a partly lifted Scan predicate is rebuilt with only its residual. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW
1 parent adbe786 commit ebd2a93

2 files changed

Lines changed: 116 additions & 16 deletions

File tree

‎crates/types/src/ir/summary_coverage.rs‎

Lines changed: 50 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ use super::node::{Operator, OperatorNode};
2222
use super::non_asap::{NonASAPOp, TimeRangeKind};
2323
use super::scalar::{Predicate, ScalarExpr};
2424
use crate::pre_asap::expr_ir::{CompareOpKind, ScalarValue};
25-
use crate::pre_asap::schema::ColumnId;
25+
use crate::pre_asap::schema::{ColumnId, Schema};
2626

2727
#[derive(Debug, Clone, PartialEq)]
2828
pub struct SummaryCoverage {
@@ -173,25 +173,28 @@ fn of_summary_agg(node: &OperatorNode) -> SummaryCoverage {
173173
.count()
174174
== 1;
175175

176+
let top = &child.schema;
176177
// Top-down: decide which conjuncts are lifted. `kept[d]` is the residual
177178
// of `chain[d]` when it is a `Filter`.
178179
let mut lifted = BTreeMap::new();
179180
let agg_filter = filter
180181
.as_ref()
181-
.and_then(|pred| residual(pred, &[], &mut lifted));
182+
.and_then(|pred| residual(pred, top, &[], &mut lifted));
182183
let kept: Vec<Option<Predicate>> = chain
183184
.iter()
184185
.enumerate()
185186
.map(|(depth, link)| match link.non_asap() {
186-
Some(NonASAPOp::Filter { pred, .. }) => residual(pred, &chain[..depth], &mut lifted),
187+
Some(NonASAPOp::Filter { pred, .. }) => {
188+
residual(pred, top, &chain[..depth], &mut lifted)
189+
}
187190
_ => None,
188191
})
189192
.collect();
190193
let scan_kept = match base.non_asap() {
191194
Some(NonASAPOp::Scan { predicates, .. }) => Some(
192195
predicates
193196
.iter()
194-
.filter_map(|pred| residual(pred, &chain, &mut lifted))
197+
.filter_map(|pred| residual(pred, top, &chain, &mut lifted))
195198
.collect::<Vec<_>>(),
196199
),
197200
_ => None,
@@ -209,7 +212,7 @@ fn of_summary_agg(node: &OperatorNode) -> SummaryCoverage {
209212
}),
210213
) = (scan_kept, base.non_asap())
211214
{
212-
if kept.len() != predicates.len() {
215+
if kept != *predicates {
213216
rebuilt = rebuild(
214217
base,
215218
NonASAPOp::Scan {
@@ -279,7 +282,6 @@ fn of_summary_agg(node: &OperatorNode) -> SummaryCoverage {
279282
},
280283
);
281284

282-
let top = &child.schema;
283285
let selection = SelectionBox {
284286
columns: lifted
285287
.into_iter()
@@ -319,18 +321,30 @@ fn rebuild_asap(original: &OperatorNode, op: ASAPOp) -> Rc<OperatorNode> {
319321
))
320322
}
321323

322-
/// Lift what `pred`'s conjuncts can into `lifted` (keyed by the agg child's
323-
/// column) and return the rest. `above` are the chain nodes between the
324-
/// predicate and the `SummaryAgg`, top to bottom.
324+
/// Lift what `pred`'s conjuncts can into `lifted` (keyed by the column of
325+
/// `top`, the agg child's schema) and return the rest. `above` are the chain
326+
/// nodes between the predicate and the `SummaryAgg`, top to bottom. A column
327+
/// whose `(table, name)` is not unique in `top` cannot be named in a
328+
/// selection, so its conjuncts stay.
325329
fn residual(
326330
pred: &Predicate,
331+
top: &Schema,
327332
above: &[&Rc<OperatorNode>],
328333
lifted: &mut BTreeMap<ColumnId, Constraint>,
329334
) -> Option<Predicate> {
330335
let mut rest: Vec<ScalarExpr> = Vec::new();
331336
for conjunct in pred.0.conjuncts() {
332337
let lift = constraint_of(conjunct).and_then(|(column, constraint)| {
333338
let column = column_at_top(column, above)?;
339+
let field = &top.fields[column];
340+
let namesakes = top
341+
.fields
342+
.iter()
343+
.filter(|f| f.name == field.name && f.table == field.table)
344+
.count();
345+
if namesakes != 1 {
346+
return None;
347+
}
334348
let combined = match lifted.get(&column) {
335349
Some(existing) => existing.intersect(&constraint)?,
336350
None => constraint,
@@ -454,6 +468,29 @@ fn dedup(values: Vec<ScalarValue>) -> Vec<ScalarValue> {
454468
out
455469
}
456470

471+
/// Whether `v` equals one of `values`; `None` when some pair cannot be
472+
/// compared (different types, NaN), since `1` and `1.0` may select the same
473+
/// rows.
474+
fn among(v: &ScalarValue, values: &[ScalarValue]) -> Option<bool> {
475+
for w in values {
476+
if compare(v, w)? == Ordering::Equal {
477+
return Some(true);
478+
}
479+
}
480+
Some(false)
481+
}
482+
483+
/// The values of `a` that are (`keep = true`) or are not in `b`.
484+
fn filter_values(a: &[ScalarValue], b: &[ScalarValue], keep: bool) -> Option<Vec<ScalarValue>> {
485+
let mut out = Vec::new();
486+
for v in a {
487+
if among(v, b)? == keep {
488+
out.push(v.clone());
489+
}
490+
}
491+
Some(out)
492+
}
493+
457494
/// Order of two literals of the same type; `None` across types or for NaN.
458495
fn compare(a: &ScalarValue, b: &ScalarValue) -> Option<Ordering> {
459496
match (a, b) {
@@ -535,10 +572,8 @@ impl Constraint {
535572
fn intersect(&self, other: &Self) -> Option<Self> {
536573
use Constraint::*;
537574
Some(match (self, other) {
538-
(In(a), In(b)) => In(a.iter().filter(|v| b.contains(v)).cloned().collect()),
539-
(In(a), NotIn(b)) | (NotIn(b), In(a)) => {
540-
In(a.iter().filter(|v| !b.contains(v)).cloned().collect())
541-
}
575+
(In(a), In(b)) => In(filter_values(a, b, true)?),
576+
(In(a), NotIn(b)) | (NotIn(b), In(a)) => In(filter_values(a, b, false)?),
542577
(NotIn(a), NotIn(b)) => NotIn(dedup(a.iter().chain(b).cloned().collect())),
543578
(In(a), Interval { lower, upper }) | (Interval { lower, upper }, In(a)) => {
544579
let mut kept = Vec::new();
@@ -570,8 +605,8 @@ impl Constraint {
570605
fn disjoint(&self, other: &Self) -> bool {
571606
use Constraint::*;
572607
match (self, other) {
573-
(In(a), In(b)) => !a.iter().any(|v| b.contains(v)),
574-
(In(a), NotIn(b)) | (NotIn(b), In(a)) => a.iter().all(|v| b.contains(v)),
608+
(In(a), In(b)) => a.iter().all(|v| among(v, b) == Some(false)),
609+
(In(a), NotIn(b)) | (NotIn(b), In(a)) => a.iter().all(|v| among(v, b) == Some(true)),
575610
(In(a), Interval { lower, upper }) | (Interval { lower, upper }, In(a)) => a
576611
.iter()
577612
.all(|v| inside(v, (lower, upper), compare) == Some(false)),

‎crates/types/tests/summary_coverage.rs‎

Lines changed: 66 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ use std::time::Duration;
88
use asap_types::ir::operator_properties::{Reduction, Source};
99
use asap_types::ir::summary_coverage::{ColumnIdentity, Constraint, CoverageError, SelectionBox};
1010
use asap_types::ir::{
11-
ASAPOp, ExprSemantics, NonASAPOp, Operator, OperatorNode, Predicate, ScalarExpr,
11+
ASAPOp, ExprSemantics, NonASAPOp, Operator, OperatorNode, Predicate, ProjectItem, ScalarExpr,
1212
SchemaDerivationError, TimeRangeKind,
1313
};
1414
use asap_types::post_asap::{
@@ -410,3 +410,68 @@ fn non_summary_nodes_have_no_coverage() {
410410
assert!(table().coverage().is_none());
411411
assert!(filter(table(), eq(REGION, "us")).coverage().is_none());
412412
}
413+
414+
/// Two output columns with the same `(table, name)` cannot be told apart in
415+
/// a selection, so restrictions on them stay in the definition.
416+
#[test]
417+
fn ambiguous_column_names_do_not_lift() {
418+
let renamed = |restricted: usize, value: &str| {
419+
let item = |column: usize, alias: Option<&str>| ProjectItem {
420+
alias: alias.map(Into::into),
421+
expr: ScalarExpr::Column(column),
422+
};
423+
let project = OperatorNode::new_shared(Operator::NonASAP(NonASAPOp::Project {
424+
cols: vec![
425+
item(JOB, None),
426+
item(REGION, Some("k")),
427+
item(LATENCY, Some("k")),
428+
item(LATENCY, None),
429+
],
430+
qualifier: None,
431+
child: table(),
432+
}))
433+
.unwrap();
434+
kll(filter(project, eq(restricted, value)), "latency")
435+
};
436+
assert_eq!(
437+
rejected(vec![renamed(1, "us"), renamed(2, "eu")]),
438+
CoverageError::DefinitionMismatch
439+
);
440+
}
441+
442+
/// `latency = 1` and `latency = 1.0` select the same rows of a Float64 column.
443+
#[test]
444+
fn literals_of_different_types_are_not_proven_different() {
445+
let one = |value| latency_where(compare(LATENCY, CompareOpKind::Eq, value));
446+
assert!(merge(vec![
447+
one(ScalarValue::Int64(1)),
448+
one(ScalarValue::Float64(1.0))
449+
])
450+
.is_err());
451+
}
452+
453+
/// A scan predicate whose conjuncts only partly lift keeps just the rest.
454+
#[test]
455+
fn partly_lifted_scan_predicates_keep_only_the_rest() {
456+
let doubled_positive = ScalarExpr::Compare {
457+
left: Box::new(ScalarExpr::Arithmetic {
458+
op: ArithmeticOpKind::Mul,
459+
left: Box::new(ScalarExpr::Column(1)),
460+
right: Box::new(ScalarExpr::Literal(ScalarValue::Float64(2.0))),
461+
semantics: ExprSemantics::Promql,
462+
}),
463+
op: CompareOpKind::Gt,
464+
right: Box::new(ScalarExpr::Literal(ScalarValue::Float64(0.0))),
465+
semantics: ExprSemantics::Promql,
466+
};
467+
let job = |name: &str| {
468+
pane(
469+
0,
470+
vec![Predicate(ScalarExpr::BoolAnd(vec![
471+
eq(2, name),
472+
doubled_positive.clone(),
473+
]))],
474+
)
475+
};
476+
merge(vec![job("api"), job("web")]).unwrap();
477+
}

0 commit comments

Comments
 (0)