From 26f7c12ffd3fcb46900dc3d5af74ee9b829c6676 Mon Sep 17 00:00:00 2001 From: Ruihang Xia Date: Wed, 4 Feb 2026 21:12:20 +0800 Subject: [PATCH] fix: drop rhs columns on promql filter join (#7665) * fix: drop rhs columns on promql filter join Signed-off-by: Ruihang Xia * choose instant vector to operate Signed-off-by: Ruihang Xia --------- Signed-off-by: Ruihang Xia --- src/query/src/promql/planner.rs | 90 ++++++++++++- .../common/promql/comparison_filter_or.result | 119 ++++++++++++++++++ .../common/promql/comparison_filter_or.sql | 73 +++++++++++ .../stats_schema_mismatch_regression.result | 12 +- .../standalone/common/promql/time_fn.result | 24 ++-- .../common/promql/timestamp_fn.result | 14 +-- 6 files changed, 304 insertions(+), 28 deletions(-) create mode 100644 tests/cases/standalone/common/promql/comparison_filter_or.result create mode 100644 tests/cases/standalone/common/promql/comparison_filter_or.sql diff --git a/src/query/src/promql/planner.rs b/src/query/src/promql/planner.rs index a2851c02f6..cba34a2db4 100644 --- a/src/query/src/promql/planner.rs +++ b/src/query/src/promql/planner.rs @@ -66,6 +66,7 @@ use promql::functions::{ }; use promql_parser::label::{METRIC_NAME, MatchOp, Matcher, Matchers}; use promql_parser::parser::token::TokenType; +use promql_parser::parser::value::ValueType; use promql_parser::parser::{ AggregateExpr, BinModifier, BinaryExpr as PromBinaryExpr, Call, EvalStmt, Expr as PromExpr, Function, FunctionArgs as PromFunctionArgs, LabelModifier, MatrixSelector, NumberLiteral, @@ -761,7 +762,21 @@ impl PromPlanner { Ok(binary_expr) }; if is_comparison_op && !should_return_bool { - self.filter_on_field_column(join_plan, bin_expr_builder) + // PromQL comparison operators without `bool` are filters: + // - keep the instant-vector side sample values + // - drop samples where the comparison is false + // + // So we filter on the join result and then project only the side that should + // be preserved according to PromQL semantics. + let filtered = self.filter_on_field_column(join_plan, bin_expr_builder)?; + let (project_table_ref, project_context) = + match (lhs.value_type(), rhs.value_type()) { + (ValueType::Scalar, ValueType::Vector) => { + (&right_table_ref, &right_context) + } + _ => (&left_table_ref, &left_context), + }; + self.project_binary_join_side(filtered, project_table_ref, project_context) } else { self.projection_for_each_field_column(join_plan, bin_expr_builder) } @@ -769,6 +784,68 @@ impl PromPlanner { } } + fn project_binary_join_side( + &mut self, + input: LogicalPlan, + table_ref: &TableReference, + context: &PromPlannerContext, + ) -> Result { + let schema = input.schema(); + + let mut project_exprs = + Vec::with_capacity(context.tag_columns.len() + context.field_columns.len() + 2); + + // Project time index from the chosen side. + if let Some(time_index_column) = &context.time_index_column { + let time_index_col = schema + .qualified_field_with_name(Some(table_ref), time_index_column) + .context(DataFusionPlanningSnafu)? + .into(); + project_exprs.push(DfExpr::Column(time_index_col)); + } + + // Project field columns from the chosen side. + for field_column in &context.field_columns { + let field_col = schema + .qualified_field_with_name(Some(table_ref), field_column) + .context(DataFusionPlanningSnafu)? + .into(); + project_exprs.push(DfExpr::Column(field_col)); + } + + // Project tag columns from the chosen side. + for tag_column in &context.tag_columns { + let tag_col = schema + .qualified_field_with_name(Some(table_ref), tag_column) + .context(DataFusionPlanningSnafu)? + .into(); + project_exprs.push(DfExpr::Column(tag_col)); + } + + // Preserve `__tsid` if present, so it can still be used internally downstream. It's + // stripped from the final output anyway. + if context.use_tsid + && let Ok(tsid_col) = + schema.qualified_field_with_name(Some(table_ref), DATA_SCHEMA_TSID_COLUMN_NAME) + { + project_exprs.push(DfExpr::Column(tsid_col.into())); + } + + let plan = LogicalPlanBuilder::from(input) + .project(project_exprs) + .context(DataFusionPlanningSnafu)? + .build() + .context(DataFusionPlanningSnafu)?; + + // Update context to reflect the projected schema. Don't keep a table qualifier since + // the result is a derived expression. + self.ctx = context.clone(); + self.ctx.table_name = None; + self.ctx.schema_name = None; + + Ok(plan) + } + fn prom_number_lit_to_plan(&mut self, number_literal: &NumberLiteral) -> Result { let NumberLiteral { val } = number_literal; self.ctx.time_index_column = Some(DEFAULT_TIME_INDEX_COLUMN.to_string()); @@ -2694,8 +2771,15 @@ impl PromPlanner { exprs.push(expr); } - conjunction(exprs).context(ValueNotFoundSnafu { - table: self.table_ref()?.to_quoted_string(), + // This error context should be computed lazily: the planner may set `ctx.table_name` to + // `None` for derived expressions (e.g. after projecting the LHS of a vector-vector + // comparison filter). Eagerly calling `table_ref()?` here can turn a valid plan into + // a `TableNameNotFound` error even when `conjunction(exprs)` succeeds. + conjunction(exprs).with_context(|| ValueNotFoundSnafu { + table: self + .table_ref() + .map(|t| t.to_quoted_string()) + .unwrap_or_else(|_| "unknown".to_string()), }) } diff --git a/tests/cases/standalone/common/promql/comparison_filter_or.result b/tests/cases/standalone/common/promql/comparison_filter_or.result new file mode 100644 index 0000000000..a828c54e0d --- /dev/null +++ b/tests/cases/standalone/common/promql/comparison_filter_or.result @@ -0,0 +1,119 @@ +-- Use metric engine to match real PromQL usage. +CREATE TABLE comparison_filter_or_physical ( + ts TIMESTAMP TIME INDEX, + greptime_value DOUBLE +) ENGINE=metric WITH ("physical_metric_table" = ""); + +Affected Rows: 0 + +CREATE TABLE a ( + k STRING NULL, + ts TIMESTAMP NOT NULL, + greptime_value DOUBLE NULL, + TIME INDEX (ts), + PRIMARY KEY (k) +) ENGINE=metric WITH (on_physical_table = 'comparison_filter_or_physical'); + +Affected Rows: 0 + +CREATE TABLE b ( + k STRING NULL, + ts TIMESTAMP NOT NULL, + greptime_value DOUBLE NULL, + TIME INDEX (ts), + PRIMARY KEY (k) +) ENGINE=metric WITH (on_physical_table = 'comparison_filter_or_physical'); + +Affected Rows: 0 + +INSERT INTO a (ts, k, greptime_value) +VALUES + (0, 'x', 3), + (0, 'y', 1), + (0, 'z', 5); + +Affected Rows: 3 + +INSERT INTO b (ts, k, greptime_value) +VALUES + (0, 'x', 1), + (0, 'y', 2); + +Affected Rows: 2 + +-- Regression: vector-vector comparison without `bool` is a filter that keeps LHS values. +-- It must not leak RHS columns (e.g. duplicated `ts`), otherwise set operators like `or` +-- can fail with ambiguous column references. +-- +-- Also cover the common PromQL pattern `(A > B) or B or A`, which uses `or` to fill missing +-- series from RHS (e.g. B missing for some labels, then fallback to A). +-- SQLNESS SORT_RESULT 2 1 +tql eval (0, 0, '1s') (a > b) or b or a; + ++---------------------+----------------+---+ +| ts | greptime_value | k | ++---------------------+----------------+---+ +| 1970-01-01T00:00:00 | 2.0 | y | +| 1970-01-01T00:00:00 | 3.0 | x | +| 1970-01-01T00:00:00 | 5.0 | z | ++---------------------+----------------+---+ + +-- Regression: derived vector expressions may not have a concrete `ctx.table_name`. +-- Function planning (which filters empty values) must not require a table name when the +-- input plan is valid. +-- SQLNESS SORT_RESULT 2 1 +tql eval (0, 0, '1s') abs(a > b); + ++---------------------+---------------------+---+ +| ts | abs(greptime_value) | k | ++---------------------+---------------------+---+ +| 1970-01-01T00:00:00 | 3.0 | x | ++---------------------+---------------------+---+ + +-- Regression: scalar-vector comparison (scalar on LHS) without `bool` is a filter that keeps +-- the vector side's samples/labels (not the scalar side). +CREATE TABLE m ( + k STRING NULL, + ts TIMESTAMP NOT NULL, + greptime_value DOUBLE NULL, + TIME INDEX (ts), + PRIMARY KEY (k) +) ENGINE=metric WITH (on_physical_table = 'comparison_filter_or_physical'); + +Affected Rows: 0 + +INSERT INTO m (ts, k, greptime_value) +VALUES + (1000, 'x', 3), + (1000, 'y', 0), + (2000, 'x', 4), + (2000, 'y', 0); + +Affected Rows: 4 + +-- SQLNESS SORT_RESULT 3 1 +tql eval (1, 2, '1s') time() < m; + ++---------------------+----------------+---+ +| ts | greptime_value | k | ++---------------------+----------------+---+ +| 1970-01-01T00:00:01 | 3.0 | x | +| 1970-01-01T00:00:02 | 4.0 | x | ++---------------------+----------------+---+ + +drop table m; + +Affected Rows: 0 + +drop table a; + +Affected Rows: 0 + +drop table b; + +Affected Rows: 0 + +drop table comparison_filter_or_physical; + +Affected Rows: 0 + diff --git a/tests/cases/standalone/common/promql/comparison_filter_or.sql b/tests/cases/standalone/common/promql/comparison_filter_or.sql new file mode 100644 index 0000000000..cb774ed37b --- /dev/null +++ b/tests/cases/standalone/common/promql/comparison_filter_or.sql @@ -0,0 +1,73 @@ +-- Use metric engine to match real PromQL usage. +CREATE TABLE comparison_filter_or_physical ( + ts TIMESTAMP TIME INDEX, + greptime_value DOUBLE +) ENGINE=metric WITH ("physical_metric_table" = ""); + +CREATE TABLE a ( + k STRING NULL, + ts TIMESTAMP NOT NULL, + greptime_value DOUBLE NULL, + TIME INDEX (ts), + PRIMARY KEY (k) +) ENGINE=metric WITH (on_physical_table = 'comparison_filter_or_physical'); + +CREATE TABLE b ( + k STRING NULL, + ts TIMESTAMP NOT NULL, + greptime_value DOUBLE NULL, + TIME INDEX (ts), + PRIMARY KEY (k) +) ENGINE=metric WITH (on_physical_table = 'comparison_filter_or_physical'); + +INSERT INTO a (ts, k, greptime_value) +VALUES + (0, 'x', 3), + (0, 'y', 1), + (0, 'z', 5); + +INSERT INTO b (ts, k, greptime_value) +VALUES + (0, 'x', 1), + (0, 'y', 2); + +-- Regression: vector-vector comparison without `bool` is a filter that keeps LHS values. +-- It must not leak RHS columns (e.g. duplicated `ts`), otherwise set operators like `or` +-- can fail with ambiguous column references. +-- +-- Also cover the common PromQL pattern `(A > B) or B or A`, which uses `or` to fill missing +-- series from RHS (e.g. B missing for some labels, then fallback to A). +-- SQLNESS SORT_RESULT 2 1 +tql eval (0, 0, '1s') (a > b) or b or a; + +-- Regression: derived vector expressions may not have a concrete `ctx.table_name`. +-- Function planning (which filters empty values) must not require a table name when the +-- input plan is valid. +-- SQLNESS SORT_RESULT 2 1 +tql eval (0, 0, '1s') abs(a > b); + +-- Regression: scalar-vector comparison (scalar on LHS) without `bool` is a filter that keeps +-- the vector side's samples/labels (not the scalar side). +CREATE TABLE m ( + k STRING NULL, + ts TIMESTAMP NOT NULL, + greptime_value DOUBLE NULL, + TIME INDEX (ts), + PRIMARY KEY (k) +) ENGINE=metric WITH (on_physical_table = 'comparison_filter_or_physical'); + +INSERT INTO m (ts, k, greptime_value) +VALUES + (1000, 'x', 3), + (1000, 'y', 0), + (2000, 'x', 4), + (2000, 'y', 0); + +-- SQLNESS SORT_RESULT 3 1 +tql eval (1, 2, '1s') time() < m; + +drop table m; + +drop table a; +drop table b; +drop table comparison_filter_or_physical; diff --git a/tests/cases/standalone/common/promql/stats_schema_mismatch_regression.result b/tests/cases/standalone/common/promql/stats_schema_mismatch_regression.result index 496da696f8..50efeed89a 100644 --- a/tests/cases/standalone/common/promql/stats_schema_mismatch_regression.result +++ b/tests/cases/standalone/common/promql/stats_schema_mismatch_regression.result @@ -141,12 +141,12 @@ Affected Rows: 2 -- SQLNESS SORT_RESULT 3 1 TQL EVAL (0, 1, '1s') promql_instant_mismatch_nested == promql_instant_mismatch_nested; -+---------------------+---+--------+---------------------+---+--------+ -| ts | k | v | ts | k | v | -+---------------------+---+--------+---------------------+---+--------+ -| 1970-01-01T00:00:00 | a | {x: 1} | 1970-01-01T00:00:00 | a | {x: 1} | -| 1970-01-01T00:00:01 | a | {x: 2} | 1970-01-01T00:00:01 | a | {x: 2} | -+---------------------+---+--------+---------------------+---+--------+ ++---------------------+--------+---+ +| ts | v | k | ++---------------------+--------+---+ +| 1970-01-01T00:00:00 | {x: 1} | a | +| 1970-01-01T00:00:01 | {x: 2} | a | ++---------------------+--------+---+ DROP TABLE promql_instant_mismatch_nested; diff --git a/tests/cases/standalone/common/promql/time_fn.result b/tests/cases/standalone/common/promql/time_fn.result index 104e90b7f8..92ee594150 100644 --- a/tests/cases/standalone/common/promql/time_fn.result +++ b/tests/cases/standalone/common/promql/time_fn.result @@ -129,12 +129,12 @@ tql eval (1, 2, '1s') time() + metrics; -- SQLNESS SORT_RESULT 3 1 tql eval (1, 2, '1s') time() == metrics; -+---------------------+----------------------+---------------------+-----+ -| time | time / Float64(1000) | ts | val | -+---------------------+----------------------+---------------------+-----+ -| 1970-01-01T00:00:01 | 1.0 | 1970-01-01T00:00:01 | 1.0 | -| 1970-01-01T00:00:02 | 2.0 | 1970-01-01T00:00:02 | 2.0 | -+---------------------+----------------------+---------------------+-----+ ++---------------------+-----+ +| ts | val | ++---------------------+-----+ +| 1970-01-01T00:00:01 | 1.0 | +| 1970-01-01T00:00:02 | 2.0 | ++---------------------+-----+ -- SQLNESS SORT_RESULT 3 1 tql eval (1, 2, '1s') time() == bool metrics; @@ -159,12 +159,12 @@ tql eval (1, 2, '1s') metrics + time(); -- SQLNESS SORT_RESULT 3 1 tql eval (1, 2, '1s') metrics == time(); -+---------------------+-----+---------------------+----------------------+ -| ts | val | time | time / Float64(1000) | -+---------------------+-----+---------------------+----------------------+ -| 1970-01-01T00:00:01 | 1.0 | 1970-01-01T00:00:01 | 1.0 | -| 1970-01-01T00:00:02 | 2.0 | 1970-01-01T00:00:02 | 2.0 | -+---------------------+-----+---------------------+----------------------+ ++---------------------+-----+ +| ts | val | ++---------------------+-----+ +| 1970-01-01T00:00:01 | 1.0 | +| 1970-01-01T00:00:02 | 2.0 | ++---------------------+-----+ -- SQLNESS SORT_RESULT 3 1 tql eval (1, 2, '1s') metrics == bool time(); diff --git a/tests/cases/standalone/common/promql/timestamp_fn.result b/tests/cases/standalone/common/promql/timestamp_fn.result index cb2f2306c2..3ae9dbbf47 100644 --- a/tests/cases/standalone/common/promql/timestamp_fn.result +++ b/tests/cases/standalone/common/promql/timestamp_fn.result @@ -154,13 +154,13 @@ tql eval (0, 60, '30s') timestamp(timestamp_test) + timestamp(timestamp_test2); -- SQLNESS SORT_RESULT 3 1 tql eval (0, 60, '30s') timestamp(timestamp_test) == timestamp(timestamp_test2); -+---------------------+-------+---------------------+-------+ -| ts | value | ts | value | -+---------------------+-------+---------------------+-------+ -| 1970-01-01T00:00:00 | 0.0 | 1970-01-01T00:00:00 | 0.0 | -| 1970-01-01T00:00:30 | 1.0 | 1970-01-01T00:00:30 | 1.0 | -| 1970-01-01T00:01:00 | 60.0 | 1970-01-01T00:01:00 | 60.0 | -+---------------------+-------+---------------------+-------+ ++---------------------+-------+ +| ts | value | ++---------------------+-------+ +| 1970-01-01T00:00:00 | 0.0 | +| 1970-01-01T00:00:30 | 1.0 | +| 1970-01-01T00:01:00 | 60.0 | ++---------------------+-------+ drop table timestamp_test;