Skip to content

Commit a28b4ab

Browse files
KKouldKKould
authored
test: improve serde optimizer binder coverage (#371)
* test: improve serde optimizer binder coverage * test: improve implementation rule coverage
1 parent 60c5d5a commit a28b4ab

9 files changed

Lines changed: 901 additions & 295 deletions

File tree

‎Makefile‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ CODECOV_OUTPUT ?= lcov.info
1414
COVERAGE_PROFILE_DIR ?= target/grcov/profraw
1515
COVERAGE_HTML_DIR ?= target/grcov/html
1616
COVERAGE_RUSTFLAGS ?= -Cinstrument-coverage
17-
COVERAGE_TEST_FEATURES ?= decimal,orm
17+
COVERAGE_TEST_FEATURES ?= copy,decimal,orm
1818
COVERAGE_REPORT_ARGS ?= --llvm --ignore-not-existing --keep-only 'src/**' --ignore 'src/**/tests/**' --ignore 'tests/**' --ignore 'tpcc/**' --excl-start 'GRCOV_EXCL_START' --excl-stop 'GRCOV_EXCL_STOP'
1919

2020
.PHONY: test test-python test-wasm test-slt test-all codecov codecov-html wasm-build check tpcc tpcc-kitesql-rocksdb tpcc-kitesql-lmdb tpcc-lmdb-flamegraph tpcc-lmdb-heaptrack tpcc-sqlite tpcc-sqlite-practical tpcc-sqlite-balanced tpcc-dual cargo-check build wasm-examples native-examples fmt clippy

‎src/binder/aggregate.rs‎

Lines changed: 109 additions & 134 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ use std::collections::HashSet;
1717
use super::{Binder, QueryBindStep};
1818
use crate::errors::DatabaseError;
1919
use crate::expression::function::scala::ScalarFunction;
20+
use crate::expression::visitor::{walk_expr, Visitor};
2021
use crate::expression::visitor_mut::{walk_mut_expr, VisitorMut};
2122
use crate::planner::LogicalPlan;
2223
use crate::storage::Transaction;
@@ -321,148 +322,57 @@ impl<T: Transaction, A: AsRef<[(&'static str, DataValue)]>> Binder<'_, '_, T, A>
321322
return Ok(());
322323
}
323324

324-
match expr {
325-
ScalarExpression::AggCall { .. } => {
326-
if self.context.group_by_exprs.contains(expr)
327-
|| self.context.agg_calls.contains(expr)
328-
{
329-
return Ok(());
330-
}
325+
HavingOrderByValidator::new(&self.context.group_by_exprs, &self.context.agg_calls)
326+
.visit(expr)
327+
}
328+
}
331329

332-
Err(DatabaseError::AggMiss(format!(
333-
"expression '{expr}' must appear in the GROUP BY clause or be used in an aggregate function"
334-
)))
335-
}
336-
ScalarExpression::ColumnRef { .. } | ScalarExpression::Alias { .. } => {
337-
if self.context.group_by_exprs.contains(expr) {
338-
return Ok(());
339-
}
340-
if matches!(expr, ScalarExpression::Alias { .. }) {
341-
return self.validate_having_orderby(expr.unpack_alias_ref());
342-
}
330+
struct HavingOrderByValidator<'a> {
331+
group_by_exprs: &'a [ScalarExpression],
332+
agg_calls: &'a [ScalarExpression],
333+
}
343334

344-
Err(DatabaseError::AggMiss(format!(
345-
"expression '{expr}' must appear in the GROUP BY clause or be used in an aggregate function"
346-
)))
347-
}
335+
impl<'a> HavingOrderByValidator<'a> {
336+
fn new(group_by_exprs: &'a [ScalarExpression], agg_calls: &'a [ScalarExpression]) -> Self {
337+
Self {
338+
group_by_exprs,
339+
agg_calls,
340+
}
341+
}
348342

349-
ScalarExpression::TypeCast { expr, .. } => self.validate_having_orderby(expr),
350-
ScalarExpression::IsNull { expr, .. } => self.validate_having_orderby(expr),
351-
ScalarExpression::Unary { expr, .. } => self.validate_having_orderby(expr),
352-
ScalarExpression::In { expr, args, .. } => {
353-
self.validate_having_orderby(expr)?;
354-
for arg in args {
355-
self.validate_having_orderby(arg)?;
356-
}
357-
Ok(())
358-
}
359-
ScalarExpression::Binary {
360-
left_expr,
361-
right_expr,
362-
..
363-
} => {
364-
self.validate_having_orderby(left_expr)?;
365-
self.validate_having_orderby(right_expr)?;
366-
Ok(())
367-
}
368-
ScalarExpression::Between {
369-
expr,
370-
left_expr,
371-
right_expr,
372-
..
373-
} => {
374-
self.validate_having_orderby(expr)?;
375-
self.validate_having_orderby(left_expr)?;
376-
self.validate_having_orderby(right_expr)?;
377-
Ok(())
378-
}
379-
ScalarExpression::SubString {
380-
expr,
381-
for_expr,
382-
from_expr,
383-
} => {
384-
self.validate_having_orderby(expr)?;
385-
if let Some(expr) = for_expr {
386-
self.validate_having_orderby(expr)?;
387-
}
388-
if let Some(expr) = from_expr {
389-
self.validate_having_orderby(expr)?;
390-
}
391-
Ok(())
392-
}
393-
ScalarExpression::Position { expr, in_expr } => {
394-
self.validate_having_orderby(expr)?;
395-
self.validate_having_orderby(in_expr)?;
396-
Ok(())
397-
}
398-
ScalarExpression::Trim {
399-
expr,
400-
trim_what_expr,
401-
..
402-
} => {
403-
self.validate_having_orderby(expr)?;
404-
if let Some(trim_what_expr) = trim_what_expr {
405-
self.validate_having_orderby(trim_what_expr)?;
343+
fn agg_miss(expr: &ScalarExpression) -> DatabaseError {
344+
DatabaseError::AggMiss(format!(
345+
"expression '{expr}' must appear in the GROUP BY clause or be used in an aggregate function"
346+
))
347+
}
348+
}
349+
350+
impl<'expr> Visitor<'expr> for HavingOrderByValidator<'_> {
351+
fn visit(&mut self, expr: &'expr ScalarExpression) -> Result<(), DatabaseError> {
352+
match expr {
353+
ScalarExpression::AggCall { .. } => {
354+
if self.group_by_exprs.contains(expr) || self.agg_calls.contains(expr) {
355+
Ok(())
356+
} else {
357+
Err(Self::agg_miss(expr))
406358
}
407-
Ok(())
408359
}
409-
ScalarExpression::Constant(_) => Ok(()),
410-
ScalarExpression::Empty => unreachable!(),
411-
ScalarExpression::Tuple(args)
412-
| ScalarExpression::ScalaFunction(ScalarFunction { args, .. })
413-
| ScalarExpression::Coalesce { exprs: args, .. } => {
414-
for expr in args {
415-
self.validate_having_orderby(expr)?;
360+
ScalarExpression::ColumnRef { .. } => {
361+
if self.group_by_exprs.contains(expr) {
362+
Ok(())
363+
} else {
364+
Err(Self::agg_miss(expr))
416365
}
417-
Ok(())
418-
}
419-
ScalarExpression::If {
420-
condition,
421-
left_expr,
422-
right_expr,
423-
..
424-
} => {
425-
self.validate_having_orderby(condition)?;
426-
self.validate_having_orderby(left_expr)?;
427-
self.validate_having_orderby(right_expr)?;
428-
429-
Ok(())
430-
}
431-
ScalarExpression::IfNull {
432-
left_expr,
433-
right_expr,
434-
..
435-
}
436-
| ScalarExpression::NullIf {
437-
left_expr,
438-
right_expr,
439-
..
440-
} => {
441-
self.validate_having_orderby(left_expr)?;
442-
self.validate_having_orderby(right_expr)?;
443-
444-
Ok(())
445366
}
446-
ScalarExpression::CaseWhen {
447-
operand_expr,
448-
expr_pairs,
449-
else_expr,
450-
..
451-
} => {
452-
if let Some(expr) = operand_expr {
453-
self.validate_having_orderby(expr)?;
454-
}
455-
for (expr_1, expr_2) in expr_pairs {
456-
self.validate_having_orderby(expr_1)?;
457-
self.validate_having_orderby(expr_2)?;
458-
}
459-
if let Some(expr) = else_expr {
460-
self.validate_having_orderby(expr)?;
367+
ScalarExpression::Alias { .. } => {
368+
if self.group_by_exprs.contains(expr) {
369+
Ok(())
370+
} else {
371+
self.visit(expr.unpack_alias_ref())
461372
}
462-
463-
Ok(())
464373
}
465-
ScalarExpression::TableFunction(_) => unreachable!(),
374+
ScalarExpression::Empty | ScalarExpression::TableFunction(_) => unreachable!(),
375+
_ => walk_expr(self, expr),
466376
}
467377
}
468378
}
@@ -538,12 +448,16 @@ impl<'a> VisitorMut<'a> for AggregateOutputBinder<'_, '_> {
538448
#[cfg(all(test, not(target_arch = "wasm32")))]
539449
mod tests {
540450
use super::AggregateOutputBinder;
451+
use crate::binder::test::build_t1_table;
452+
use crate::binder::{Binder, BinderContext};
541453
use crate::catalog::{ColumnCatalog, ColumnDesc, ColumnRef};
542454
use crate::errors::DatabaseError;
543455
use crate::expression::agg::AggKind;
544456
use crate::expression::visitor_mut::VisitorMut;
545-
use crate::expression::{AliasType, ScalarExpression};
457+
use crate::expression::{AliasType, BinaryOperator, ScalarExpression};
546458
use crate::planner::PlanArena;
459+
use crate::storage::Storage;
460+
use crate::types::value::DataValue;
547461
use crate::types::LogicalType;
548462

549463
fn test_column(arena: &mut PlanArena, name: &str, ty: LogicalType) -> ColumnRef {
@@ -638,4 +552,65 @@ mod tests {
638552

639553
Ok(())
640554
}
555+
556+
#[test]
557+
fn test_validate_having_orderby_rejects_missing_group_expr() -> Result<(), DatabaseError> {
558+
let tables = build_t1_table()?;
559+
let scala_functions = Default::default();
560+
let table_functions = Default::default();
561+
let transaction = tables.storage.transaction()?;
562+
let args: [(&'static str, DataValue); 0] = [];
563+
let mut binder = Binder::new(
564+
BinderContext::new(
565+
&tables.table_cache,
566+
&tables.view_cache,
567+
&transaction,
568+
&scala_functions,
569+
&table_functions,
570+
),
571+
&args,
572+
None,
573+
);
574+
let table_arena = crate::planner::TableArenaCell::default();
575+
let mut arena = PlanArena::new(&table_arena);
576+
let group_column = test_column(&mut arena, "c1", LogicalType::Integer);
577+
let missing_column = test_column(&mut arena, "c2", LogicalType::Integer);
578+
let group_expr = ScalarExpression::column_expr(group_column, 0);
579+
let missing_expr = ScalarExpression::column_expr(missing_column, 1);
580+
binder.context.group_by_exprs.push(group_expr.clone());
581+
582+
binder.validate_having_orderby(&group_expr)?;
583+
let group_alias = ScalarExpression::Alias {
584+
expr: Box::new(group_expr.clone()),
585+
alias: AliasType::Name("group_alias".to_string()),
586+
};
587+
binder.validate_having_orderby(&group_alias)?;
588+
589+
assert!(matches!(
590+
binder.validate_having_orderby(&missing_expr),
591+
Err(DatabaseError::AggMiss(_))
592+
));
593+
594+
let registered_agg = test_count(missing_expr.clone());
595+
binder.context.agg_calls.push(registered_agg.clone());
596+
binder.validate_having_orderby(&registered_agg)?;
597+
assert!(matches!(
598+
binder.validate_having_orderby(&test_count(ScalarExpression::Constant(1_i32.into()))),
599+
Err(DatabaseError::AggMiss(_))
600+
));
601+
602+
let invalid_binary = ScalarExpression::Binary {
603+
op: BinaryOperator::Eq,
604+
left_expr: Box::new(group_expr),
605+
right_expr: Box::new(missing_expr),
606+
evaluator: None,
607+
ty: LogicalType::Boolean,
608+
};
609+
assert!(matches!(
610+
binder.validate_having_orderby(&invalid_binary),
611+
Err(DatabaseError::AggMiss(_))
612+
));
613+
614+
Ok(())
615+
}
641616
}

0 commit comments

Comments
 (0)