fix reviewer comments

databendlabs · Oct 8, 2023 · 67f887d · 67f887d
1 parent a2fe357
commit 67f887d
Show file tree

Hide file tree

Showing 2 changed files with 17 additions and 24 deletions.
diff --git a/src/query/expression/src/evaluator.rs b/src/query/expression/src/evaluator.rs
@@ -104,23 +104,11 @@ impl<'a> Evaluator<'a> {
         }
     }
 
-    pub fn run_exprs(&self, exprs: &[Expr]) -> Result<Vec<BlockEntry>> {
-        let mut columns: Vec<BlockEntry> = Vec::with_capacity(exprs.len());
-        for expr in exprs {
-            // try get result from cache
-            if let Some(cached_values) = &self.cached_values {
-                if let Some(cached_result) = cached_values.borrow().get(expr) {
-                    let col = BlockEntry::new(expr.data_type().clone(), cached_result.clone());
-                    columns.push(col);
-                    continue;
-                }
-            }
-
-            let result = self.run(expr)?;
-            let col = BlockEntry::new(expr.data_type().clone(), result);
-            columns.push(col);
-        }
-        Ok(columns)
+    pub fn run_exprs(&self, exprs: &[Expr]) -> Result<Vec<Value<AnyType>>> {
+        exprs
+            .iter()
+            .map(|expr| self.run(expr))
+            .collect::<Result<Vec<_>>>()
     }
 
     pub fn run(&self, expr: &Expr) -> Result<Value<AnyType>> {
@@ -138,7 +126,7 @@ impl<'a> Evaluator<'a> {
         self.check_expr(expr);
 
         // try get result from cache
-        if let Some(cached_values) = &self.cached_values {
+        if !matches!(expr, Expr::ColumnRef {..} | Expr::Constant{..}) && let Some(cached_values) = &self.cached_values {
             if let Some(cached_result) = cached_values.borrow().get(expr) {
                 return Ok(cached_result.clone());
             }
@@ -235,7 +223,7 @@ impl<'a> Evaluator<'a> {
                             .enumerate()
                             .collect(),
                         self.func_ctx,
-                        self.fn_registry
+                        self.fn_registry,
                     )
                     .1,
                     None,

diff --git a/src/query/sql/src/evaluator/block_operator.rs b/src/query/sql/src/evaluator/block_operator.rs
@@ -91,14 +91,19 @@ impl BlockOperator {
                         None => Ok(input),
                     }
                 } else {
-                    // Local variable run in single thread, so `Rc` and `RefCell` are enough.
                     let evaluator =
                         Evaluator::new(&input, func_ctx, &BUILTIN_FUNCTIONS).with_cache();
-                    let result = evaluator.run_exprs(exprs)?;
-                    let block = DataBlock::new(result, input.num_rows());
+                    let results = evaluator.run_exprs(exprs)?;
+                    results
+                        .into_iter()
+                        .zip(exprs.iter())
+                        .for_each(|(result, expr)| {
+                            let col = BlockEntry::new(expr.data_type().clone(), result);
+                            input.add_column(col);
+                        });
                     match projections {
-                        Some(projections) => Ok(block.project(projections)),
-                        None => Ok(block),
+                        Some(projections) => Ok(input.project(projections)),
+                        None => Ok(input),
                     }
                 }
             }