Skip to content

Commit 6499f66

Browse files
YoEightYoEight
authored
feat: implement static analysis on HAVING clause (#21)
1 parent 8ff9179 commit 6499f66

6 files changed

Lines changed: 249 additions & 3 deletions

‎src/analysis.rs‎

Lines changed: 48 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -651,7 +651,16 @@ impl<'a> Analysis<'a> {
651651
self.analyze_expr(&mut ctx, &group_by.expr, Type::Unspecified)?;
652652

653653
if let Some(expr) = &group_by.predicate {
654+
ctx.allow_agg_func = true;
655+
ctx.use_agg_funcs = true;
656+
654657
self.analyze_expr(&mut ctx, expr, Type::Bool)?;
658+
if !self.expect_agg_expr(expr)? {
659+
return Err(AnalysisError::ExpectAggExpr(
660+
expr.attrs.pos.line,
661+
expr.attrs.pos.col,
662+
));
663+
}
655664
}
656665

657666
ctx.allow_agg_func = true;
@@ -669,7 +678,7 @@ impl<'a> Analysis<'a> {
669678
order_by.expr.attrs.pos.col,
670679
));
671680
} else if query.group_by.is_some() {
672-
self.expect_agg_expr(&order_by.expr)?;
681+
self.expect_agg_func(&order_by.expr)?;
673682
}
674683
}
675684

@@ -903,7 +912,7 @@ impl<'a> Analysis<'a> {
903912
}
904913

905914
if *aggregate {
906-
return self.expect_agg_expr(expr);
915+
return self.expect_agg_func(expr);
907916
}
908917

909918
for arg in &app.args {
@@ -924,7 +933,7 @@ impl<'a> Analysis<'a> {
924933
}
925934
}
926935

927-
fn expect_agg_expr(&self, expr: &Expr) -> AnalysisResult<()> {
936+
fn expect_agg_func(&self, expr: &Expr) -> AnalysisResult<()> {
928937
if let Value::App(app) = &expr.value
929938
&& let Some(Type::App {
930939
aggregate: true, ..
@@ -944,6 +953,42 @@ impl<'a> Analysis<'a> {
944953
))
945954
}
946955

956+
fn expect_agg_expr(&self, expr: &Expr) -> AnalysisResult<bool> {
957+
match &expr.value {
958+
Value::Id(id) => {
959+
if self.scope.entries.contains_key(id.as_str()) {
960+
return Err(AnalysisError::UnallowedAggFuncUsageWithSrcField(
961+
expr.attrs.pos.line,
962+
expr.attrs.pos.col,
963+
));
964+
}
965+
966+
Ok(false)
967+
}
968+
Value::Group(expr) => self.expect_agg_expr(expr),
969+
Value::Binary(binary) => {
970+
let lhs = self.expect_agg_expr(&binary.lhs)?;
971+
let rhs = self.expect_agg_expr(&binary.rhs)?;
972+
973+
if !lhs && !rhs {
974+
return Err(AnalysisError::ExpectAggExpr(
975+
expr.attrs.pos.line,
976+
expr.attrs.pos.col,
977+
));
978+
}
979+
980+
Ok(true)
981+
}
982+
Value::Unary(unary) => self.expect_agg_expr(unary.expr.as_ref()),
983+
Value::App(_) => {
984+
self.expect_agg_func(expr)?;
985+
Ok(true)
986+
}
987+
988+
_ => Ok(false),
989+
}
990+
}
991+
947992
fn ensure_agg_param_is_source_bound(&self, expr: &Expr) -> AnalysisResult<()> {
948993
match &expr.value {
949994
Value::Id(id) if !self.options.default_scope.entries.contains_key(id.as_str()) => {

‎src/tests/analysis.rs‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -256,3 +256,15 @@ fn test_analyze_accept_group_by_with_agg_rec() {
256256
let query = parse_query(include_str!("./resources/accept_group_by_with_agg_rec.eql")).unwrap();
257257
insta::assert_yaml_snapshot!(query.run_static_analysis(&Default::default()));
258258
}
259+
260+
#[test]
261+
fn test_reject_invalid_having_clause() {
262+
let query = parse_query(include_str!("./resources/reject_invalid_having_clause.eql")).unwrap();
263+
insta::assert_yaml_snapshot!(query.run_static_analysis(&Default::default()));
264+
}
265+
266+
#[test]
267+
fn test_accept_valid_having_clause() {
268+
let query = parse_query(include_str!("./resources/valid_having_clause.eql")).unwrap();
269+
insta::assert_yaml_snapshot!(query.run_static_analysis(&Default::default()));
270+
}

‎src/tests/resources/reject_invalid_having_clause.eql‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
FROM e IN events
2+
GROUP BY e.data.department HAVING e.data.salary > 10000
3+
PROJECT INTO {
4+
department: UNIQUE(e.data.department),
5+
average: AVG(e.data.salary)
6+
}

‎src/tests/resources/valid_having_clause.eql‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
FROM e IN events
2+
GROUP BY e.data.department HAVING AVG(e.data.salary) > 10000
3+
PROJECT INTO {
4+
department: UNIQUE(e.data.department),
5+
average: AVG(e.data.salary)
6+
}

‎src/tests/snapshots/eventql_parser__tests__analysis__accept_valid_having_clause.snap‎

Lines changed: 168 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,168 @@
1+
---
2+
source: src/tests/analysis.rs
3+
expression: "query.run_static_analysis(&Default::default())"
4+
---
5+
Ok:
6+
attrs:
7+
pos:
8+
line: 1
9+
col: 1
10+
sources:
11+
- binding:
12+
name: e
13+
pos:
14+
line: 1
15+
col: 6
16+
kind:
17+
Name: events
18+
predicate: ~
19+
group_by:
20+
expr:
21+
attrs:
22+
pos:
23+
line: 2
24+
col: 10
25+
value:
26+
Access:
27+
target:
28+
attrs:
29+
pos:
30+
line: 2
31+
col: 10
32+
value:
33+
Access:
34+
target:
35+
attrs:
36+
pos:
37+
line: 2
38+
col: 10
39+
value:
40+
Id: e
41+
field: data
42+
field: department
43+
predicate:
44+
attrs:
45+
pos:
46+
line: 2
47+
col: 35
48+
value:
49+
Binary:
50+
lhs:
51+
attrs:
52+
pos:
53+
line: 2
54+
col: 35
55+
value:
56+
App:
57+
func: AVG
58+
args:
59+
- attrs:
60+
pos:
61+
line: 2
62+
col: 39
63+
value:
64+
Access:
65+
target:
66+
attrs:
67+
pos:
68+
line: 2
69+
col: 39
70+
value:
71+
Access:
72+
target:
73+
attrs:
74+
pos:
75+
line: 2
76+
col: 39
77+
value:
78+
Id: e
79+
field: data
80+
field: salary
81+
operator: Gt
82+
rhs:
83+
attrs:
84+
pos:
85+
line: 2
86+
col: 56
87+
value:
88+
Number: 10000
89+
order_by: ~
90+
limit: ~
91+
projection:
92+
attrs:
93+
pos:
94+
line: 3
95+
col: 14
96+
value:
97+
Record:
98+
- name: department
99+
value:
100+
attrs:
101+
pos:
102+
line: 4
103+
col: 17
104+
value:
105+
App:
106+
func: UNIQUE
107+
args:
108+
- attrs:
109+
pos:
110+
line: 4
111+
col: 24
112+
value:
113+
Access:
114+
target:
115+
attrs:
116+
pos:
117+
line: 4
118+
col: 24
119+
value:
120+
Access:
121+
target:
122+
attrs:
123+
pos:
124+
line: 4
125+
col: 24
126+
value:
127+
Id: e
128+
field: data
129+
field: department
130+
- name: average
131+
value:
132+
attrs:
133+
pos:
134+
line: 5
135+
col: 14
136+
value:
137+
App:
138+
func: AVG
139+
args:
140+
- attrs:
141+
pos:
142+
line: 5
143+
col: 18
144+
value:
145+
Access:
146+
target:
147+
attrs:
148+
pos:
149+
line: 5
150+
col: 18
151+
value:
152+
Access:
153+
target:
154+
attrs:
155+
pos:
156+
line: 5
157+
col: 18
158+
value:
159+
Id: e
160+
field: data
161+
field: salary
162+
distinct: false
163+
meta:
164+
project:
165+
Record:
166+
average: Number
167+
department: Unspecified
168+
aggregate: true

‎src/tests/snapshots/eventql_parser__tests__analysis__reject_invalid_having_clause.snap‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
---
2+
source: src/tests/analysis.rs
3+
expression: "query.run_static_analysis(&Default::default())"
4+
---
5+
Err:
6+
Analysis:
7+
ExpectAggExpr:
8+
- 2
9+
- 35

0 commit comments

Comments
 (0)