Merge pull request #291 from ProjectASAP/feat/concat-discriminator-unique-keys-228

feat(query-expr): add opt-in Concat discriminator key

Author: zzylol

crates/asap-aware-mapping/src/explanation.rs

@@ -438,7 +438,7 @@ fn visit_children(
         | SQLWindowFunc { child, .. }
         | Sort { child, .. }
         | Limit { child, .. } => visit(child, format!("{label} > child"), locations),
-        Concat { children } => {
+        Concat { children, .. } => {
             for (i, c) in children.iter().enumerate() {
                 visit_children(c, &format!("{label} > concat[{i}]"), locations);
             }

crates/asap-aware-mapping/src/query_physical_lowering.rs

@@ -693,7 +693,7 @@ pub fn lower_query_physical_dag(
                     }
                     self.lower_unary(query, occurrence, PhysicalOperator::PassThrough, child)
                 }
-                QueryExpr::Concat { children } => {
+                QueryExpr::Concat { children, .. } => {
                     let child_ids = children
                         .iter()
                         .map(|child| self.lower(child))
@@ -1979,6 +1979,7 @@ mod tests {
 
         let concat = Rc::new(QueryExpr::Concat {
             children: vec![scan("a"), scan("b")],
+            discriminator_unique_key: None,
         });
         let dag = lower_query_physical_dag(&concat, &scope, &scripted(&provided)).unwrap();
         assert_eq!(dag.nodes.last().unwrap().operator, PhysicalOperator::Concat);

crates/asap-aware-mapping/src/replacement.rs

@@ -3597,6 +3597,7 @@ fn direct_child_counts(node: &QueryExpr) -> Vec<(*const QueryExpr, usize)> {
             }
             Concat {
                 children: concat_children,
+                ..
             } => {
                 for c in concat_children {
                     collect(c, children);
@@ -4183,7 +4184,7 @@ fn walk_children(
         | SQLWindowFunc { child, .. }
         | Sort { child, .. }
         | Limit { child, .. } => walk(child, order, nodes, counts),
-        Concat { children } => {
+        Concat { children, .. } => {
             for c in children {
                 walk_children(c, order, nodes, counts);
             }
@@ -5124,7 +5125,7 @@ mod tests {
                 | SQLWindowFunc { child, .. }
                 | Sort { child, .. }
                 | Limit { child, .. } => walk(child, counts),
-                Concat { children } => {
+                Concat { children, .. } => {
                     for c in children {
                         walk_children(c, counts);
                     }

crates/asap-aware-mapping/src/summary_maintenance_cost/model.rs

@@ -87,7 +87,7 @@ pub(super) fn query_source_selections(
         | PromqlSeriesSample { child, .. }
         | Sort { child, .. }
         | Limit { child, .. } => query_source_selections(child, out)?,
-        Concat { children } => {
+        Concat { children, .. } => {
             for child in children {
                 query_source_selections(child, out)?;
             }

crates/devtools/src/bin/variant_coverage.rs

@@ -88,7 +88,7 @@ fn walk(e: &QueryExpr, seen: &mut BTreeSet<&'static str>) {
             seen.insert("Dedup");
             walk(child, seen);
         }
-        QueryExpr::Concat { children } => {
+        QueryExpr::Concat { children, .. } => {
             seen.insert("Concat");
             children.iter().for_each(|c| walk(c, seen));
         }

crates/frontend-promql/src/promql.rs

@@ -742,7 +742,13 @@ fn walk_histogram_quantiles(call: &Call) -> Result<Unresolved> {
             })
         })
         .collect::<Result<Vec<_>>>()?;
-    Ok(Unresolved::Concat { children: branches })
+    // No discriminator asserted here today (issue #228): the φ value each
+    // branch carries via `PromqlRelabel` *is* structurally a distinct
+    // per-branch discriminator, but nothing downstream currently needs the
+    // resulting compound unique key — see
+    // `docs/design_docs/concat-unique-keys-decision.md`. `Unresolved::concat`
+    // keeps `output_schema`'s default (drop `unique_keys` entirely).
+    Ok(Unresolved::concat(branches))
 }
 
 /// Prometheus's `labels.FormatOpenMetricsFloat` — how `histogram_quantiles`

crates/frontend-promql/tests/observability/awesome_prometheus_alerts.rs

@@ -83,7 +83,7 @@ fn intents(e: &QueryExpr) -> Vec<AggIntent> {
                 go(lhs, out);
                 go(rhs, out);
             }
-            QueryExpr::Concat { children } => children.iter().for_each(|c| go(c, out)),
+            QueryExpr::Concat { children, .. } => children.iter().for_each(|c| go(c, out)),
             QueryExpr::PromqlVectorFromScalar(inner) | QueryExpr::PromqlScalarFromVector(inner) => {
                 go(inner, out)
             }

crates/frontend-promql/tests/promql_conformance.rs

@@ -92,7 +92,7 @@ fn collect(e: &QueryExpr, out: &mut Vec<AggIntent>) {
             collect(left, out);
             collect(right, out);
         }
-        QueryExpr::Concat { children } => children.iter().for_each(|c| collect(c, out)),
+        QueryExpr::Concat { children, .. } => children.iter().for_each(|c| collect(c, out)),
         QueryExpr::PromqlVectorFromScalar(inner) | QueryExpr::PromqlScalarFromVector(inner) => {
             collect(inner, out)
         }

crates/frontend-promql/tests/promql_lowering.rs

@@ -920,7 +920,7 @@ fn topk_over_bare_selector_ranks_raw_samples() {
 
 /// The `(label value, intent)` of each `histogram_quantiles` branch.
 fn quantile_branches(q: &QueryExpr) -> Vec<(String, AggIntent)> {
-    let QueryExpr::Concat { children } = q else {
+    let QueryExpr::Concat { children, .. } = q else {
         panic!("expected a Concat at the root, got {q:?}");
     };
     children
@@ -978,7 +978,7 @@ fn histogram_quantiles_branches_are_union_compatible() {
     // `Concat` derives its schema from the first child, so every branch must
     // agree on column names — the φ lives in the label, not the column name.
     let q = lower(r#"histogram_quantiles(testhistogram3, "q", 0.5, 0.9)"#);
-    let QueryExpr::Concat { children } = &q else {
+    let QueryExpr::Concat { children, .. } = &q else {
         panic!("expected Concat");
     };
     let shapes: Vec<Vec<String>> = children
@@ -1004,7 +1004,7 @@ fn histogram_quantiles_branches_are_union_compatible() {
 #[test]
 fn histogram_quantiles_uses_the_given_label_name() {
     let q = lower(r#"histogram_quantiles(h, "phi", 0.5)"#);
-    let QueryExpr::Concat { children } = &q else {
+    let QueryExpr::Concat { children, .. } = &q else {
         panic!("expected Concat");
     };
     let QueryExpr::PromqlRelabel { dst, .. } = &children[0] else {

crates/frontend-sql/src/sql/mod.rs

@@ -865,7 +865,14 @@ impl<'a> SqlLowerer<'a> {
             })
             .collect::<Result<Vec<_>, LoweringError>>()?;
 
-        Ok(Unresolved::Concat { children: branches })
+        // No discriminator asserted here today (issue #228): DataFusion's own
+        // `__grouping_id` would be the natural one, but this front end
+        // already discards it (see above — `GROUPING()` itself is rejected),
+        // so there is no distinct-per-branch column available to name yet.
+        // `Unresolved::concat` keeps `output_schema`'s default (drop
+        // `unique_keys` entirely). See
+        // `docs/design_docs/concat-unique-keys-decision.md`.
+        Ok(Unresolved::concat(branches))
     }
 
     fn lower_sort(&self, sort: &logical_expr::Sort) -> Result<Unresolved, LoweringError> {