From 865f1dac8dd7bf3ef9b790c7bd059af9cffdd5f8 Mon Sep 17 00:00:00 2001
From: Tpt <thomaspt@hotmail.fr>
Date: Sat, 29 Oct 2022 11:10:52 +0200
Subject: [PATCH] Uses the same key space inside and outside of aggregate
 evaluation

There is a project node just alongside aggregates so the key space is already bounded by it
---
 lib/src/sparql/eval.rs         | 20 ++++++-------
 lib/src/sparql/plan.rs         |  8 ++---
 lib/src/sparql/plan_builder.rs | 53 +++++++++++-----------------------
 3 files changed, 31 insertions(+), 50 deletions(-)

diff --git a/lib/src/sparql/eval.rs b/lib/src/sparql/eval.rs
index 87a803a5..6c76c055 100644
--- a/lib/src/sparql/eval.rs
+++ b/lib/src/sparql/eval.rs
@@ -561,11 +561,11 @@ impl SimpleEvaluator {
             }
             PlanNode::Aggregate {
                 child,
-                key_mapping,
+                key_variables,
                 aggregates,
             } => {
                 let child = self.plan_evaluator(child);
-                let key_mapping = key_mapping.clone();
+                let key_variables = key_variables.clone();
                 let aggregate_input_expressions: Vec<_> = aggregates
                     .iter()
                     .map(|(aggregate, _)| {
@@ -588,8 +588,8 @@ impl SimpleEvaluator {
                 let accumulator_variables: Vec<_> =
                     aggregates.iter().map(|(_, var)| *var).collect();
                 Rc::new(move |from| {
-                    let tuple_size = from.capacity(); //TODO: not nice
-                    let key_mapping = key_mapping.clone();
+                    let tuple_size = from.capacity();
+                    let key_variables = key_variables.clone();
                     let mut errors = Vec::default();
                     let mut accumulators_for_group =
                         HashMap::<Vec<Option<EncodedTerm>>, Vec<Box<dyn Accumulator>>>::default();
@@ -603,9 +603,9 @@ impl SimpleEvaluator {
                         })
                         .for_each(|tuple| {
                             //TODO avoid copy for key?
-                            let key = key_mapping
+                            let key = key_variables
                                 .iter()
-                                .map(|(v, _)| tuple.get(*v).cloned())
+                                .map(|v| tuple.get(*v).cloned())
                                 .collect();
 
                             let key_accumulators =
@@ -623,7 +623,7 @@ impl SimpleEvaluator {
                                 );
                             }
                         });
-                    if accumulators_for_group.is_empty() && key_mapping.is_empty() {
+                    if accumulators_for_group.is_empty() && key_variables.is_empty() {
                         // There is always a single group if there is no GROUP BY
                         accumulators_for_group.insert(Vec::new(), Vec::new());
                     }
@@ -635,9 +635,9 @@ impl SimpleEvaluator {
                             .chain(accumulators_for_group.into_iter().map(
                                 move |(key, accumulators)| {
                                     let mut result = EncodedTuple::with_capacity(tuple_size);
-                                    for (from_position, to_position) in key_mapping.iter() {
-                                        if let Some(value) = &key[*from_position] {
-                                            result.set(*to_position, value.clone());
+                                    for (variable, value) in key_variables.iter().zip(key) {
+                                        if let Some(value) = value {
+                                            result.set(*variable, value);
                                         }
                                     }
                                     for (accumulator, variable) in
diff --git a/lib/src/sparql/plan.rs b/lib/src/sparql/plan.rs
index 1d99af7b..b5fd578a 100644
--- a/lib/src/sparql/plan.rs
+++ b/lib/src/sparql/plan.rs
@@ -90,7 +90,7 @@ pub enum PlanNode {
     Aggregate {
         // By definition the group by key are the range 0..key_mapping.len()
         child: Box<Self>,
-        key_mapping: Rc<Vec<(usize, usize)>>, // aggregate key pairs of (variable key in child, variable key in output)
+        key_variables: Rc<Vec<usize>>,
         aggregates: Rc<Vec<(PlanAggregation, usize)>>,
     },
 }
@@ -200,12 +200,12 @@ impl PlanNode {
                 }
             }
             Self::Aggregate {
-                key_mapping,
+                key_variables,
                 aggregates,
                 ..
             } => {
-                for (_, o) in key_mapping.iter() {
-                    callback(*o);
+                for var in key_variables.iter() {
+                    callback(*var);
                 }
                 for (_, var) in aggregates.iter() {
                     callback(*var);
diff --git a/lib/src/sparql/plan_builder.rs b/lib/src/sparql/plan_builder.rs
index 93fcd36f..fd96b0af 100644
--- a/lib/src/sparql/plan_builder.rs
+++ b/lib/src/sparql/plan_builder.rs
@@ -179,40 +179,21 @@ impl<'a> PlanBuilder<'a> {
                 inner,
                 variables: by,
                 aggregates,
-            } => {
-                let mut inner_variables = by.clone();
-                let inner_graph_name =
-                    Self::convert_pattern_value_id(graph_name, variables, &mut inner_variables);
-
-                PlanNode::Aggregate {
-                    child: Box::new(self.build_for_graph_pattern(
-                        inner,
-                        &mut inner_variables,
-                        &inner_graph_name,
-                    )?),
-                    key_mapping: Rc::new(
-                        by.iter()
-                            .map(|k| {
-                                (
-                                    variable_key(&mut inner_variables, k),
-                                    variable_key(variables, k),
-                                )
-                            })
-                            .collect(),
-                    ),
-                    aggregates: Rc::new(
-                        aggregates
-                            .iter()
-                            .map(|(v, a)| {
-                                Ok((
-                                    self.build_for_aggregate(a, &mut inner_variables, graph_name)?,
-                                    variable_key(variables, v),
-                                ))
-                            })
-                            .collect::<Result<Vec<_>, EvaluationError>>()?,
-                    ),
-                }
-            }
+            } => PlanNode::Aggregate {
+                child: Box::new(self.build_for_graph_pattern(inner, variables, graph_name)?),
+                key_variables: Rc::new(by.iter().map(|k| variable_key(variables, k)).collect()),
+                aggregates: Rc::new(
+                    aggregates
+                        .iter()
+                        .map(|(v, a)| {
+                            Ok((
+                                self.build_for_aggregate(a, variables, graph_name)?,
+                                variable_key(variables, v),
+                            ))
+                        })
+                        .collect::<Result<Vec<_>, EvaluationError>>()?,
+                ),
+            },
             GraphPattern::Values {
                 variables: table_variables,
                 bindings,
@@ -1136,11 +1117,11 @@ impl<'a> PlanBuilder<'a> {
                 }
             }
             PlanNode::Aggregate {
-                key_mapping,
+                key_variables,
                 aggregates,
                 ..
             } => {
-                set.extend(key_mapping.iter().map(|(_, o)| o));
+                set.extend(key_variables.iter());
                 //TODO: This is too harsh
                 for (_, var) in aggregates.iter() {
                     set.insert(*var);