Skip to content

Commit 908cf91

Browse files
committed
Apply suggestion from PR, and increase the recursion limit thanks to reduction on Err type
1 parent 004d978 commit 908cf91

File tree

12 files changed

+113
-84
lines changed

12 files changed

+113
-84
lines changed

crates/core/src/error.rs

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -82,8 +82,6 @@ pub enum DatabaseError {
8282
DatabasedOpened(PathBuf, anyhow::Error),
8383
}
8484

85-
// FIXME: reduce type size
86-
#[expect(clippy::large_enum_variant)]
8785
#[derive(Error, Debug, EnumAsInner)]
8886
pub enum DBError {
8987
#[error("LibError: {0}")]

crates/core/src/host/module_host.rs

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -539,8 +539,6 @@ pub enum InitDatabaseError {
539539
Other(anyhow::Error),
540540
}
541541

542-
// FIXME: reduce type size
543-
#[expect(clippy::large_enum_variant)]
544542
#[derive(thiserror::Error, Debug)]
545543
pub enum ClientConnectedError {
546544
#[error(transparent)]

crates/core/src/sql/execute.rs

Lines changed: 11 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1126,9 +1126,11 @@ pub(crate) mod tests {
11261126
Ok(())
11271127
}
11281128

1129-
/// Test we are protected against recursion when:
1130-
/// 1. The query is too large
1129+
/// Test we are protected against stack overflows when:
1130+
/// 1. The query is too large (too many characters)
11311131
/// 2. The AST is too deep
1132+
///
1133+
/// Exercise the limit [`recursion::MAX_RECURSION_EXPR`]
11321134
#[test]
11331135
fn test_large_query_no_panic() -> ResultTest<()> {
11341136
let db = TestDB::durable()?;
@@ -1143,13 +1145,11 @@ pub(crate) mod tests {
11431145

11441146
let build_query = |total| {
11451147
let mut sql = "select * from test where ".to_string();
1146-
for x in 0..total {
1147-
for y in 0..total {
1148-
let fragment = format!("((x = {x}) and (y = {y})) or ");
1149-
sql.push_str(&fragment);
1150-
}
1148+
for x in 1..total {
1149+
let fragment = format!("x = {x} or ");
1150+
sql.push_str(&fragment.repeat((total - 1) as usize));
11511151
}
1152-
sql.push_str("((x = 1000) and (y = 1000))");
1152+
sql.push_str("(y = 0)");
11531153
sql
11541154
};
11551155
let run = |db: &RelationalDB, sep: char, sql_text: &str| {
@@ -1161,16 +1161,15 @@ pub(crate) mod tests {
11611161
Err("SQL query exceeds maximum allowed length".to_string())
11621162
);
11631163

1164-
// Exercise the limit [recursion::MAX_RECURSION_EXPR] && [recursion::MAX_RECURSION_TYP_EXPR]
1165-
let sql = build_query(8);
1164+
let sql = build_query(41); // This causes stack overflow without the limit
11661165
assert_eq!(run(&db, ',', &sql), Err("Recursion limit exceeded".to_string()));
11671166

1168-
let sql = build_query(7);
1167+
let sql = build_query(40); // The max we can with the current limit
11691168
assert!(run(&db, ',', &sql).is_ok(), "Expected query to run without panic");
11701169

11711170
// Check no overflow with lot of joins
11721171
let mut sql = "SELECT test.* FROM test ".to_string();
1173-
// We could pust up to 700 joins without overflow as long we don't have any conditions,
1172+
// We could push up to 700 joins without overflow as long we don't have any conditions,
11741173
// but here execution become too slow.
11751174
// TODO: Move this test to the `Plan`
11761175
for i in 0..200 {

crates/expr/src/check.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ pub trait TypeChecker {
9999
vars.insert(rhs.alias.clone(), rhs.schema.clone());
100100

101101
if let Some(on) = on {
102-
if let Expr::BinOp(BinOp::Eq, a, b) = type_expr(vars, on, Some(&AlgebraicType::Bool), &mut 0)? {
102+
if let Expr::BinOp(BinOp::Eq, a, b) = type_expr(vars, on, Some(&AlgebraicType::Bool))? {
103103
if let (Expr::Field(a), Expr::Field(b)) = (*a, *b) {
104104
join = RelExpr::EqJoin(LeftDeepJoin { lhs, rhs }, a, b);
105105
continue;

crates/expr/src/errors.rs

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -122,8 +122,6 @@ pub struct DuplicateName(pub String);
122122
#[error("`filter!` does not support column projections; Must return table rows")]
123123
pub struct FilterReturnType;
124124

125-
// FIXME: reduce type size
126-
#[expect(clippy::large_enum_variant)]
127125
#[derive(Error, Debug)]
128126
pub enum TypingError {
129127
#[error(transparent)]

crates/expr/src/lib.rs

Lines changed: 19 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ pub mod statement;
3131
pub(crate) fn type_select(input: RelExpr, expr: SqlExpr, vars: &Relvars) -> TypingResult<RelExpr> {
3232
Ok(RelExpr::Select(
3333
Box::new(input),
34-
type_expr(vars, expr, Some(&AlgebraicType::Bool), &mut 0)?,
34+
type_expr(vars, expr, Some(&AlgebraicType::Bool))?,
3535
))
3636
}
3737

@@ -69,7 +69,7 @@ pub(crate) fn type_proj(input: RelExpr, proj: ast::Project, vars: &Relvars) -> T
6969
return Err(DuplicateName(alias.into_string()).into());
7070
}
7171

72-
if let Expr::Field(p) = type_expr(vars, expr.into(), None, &mut 0)? {
72+
if let Expr::Field(p) = type_expr(vars, expr.into(), None)? {
7373
projections.push((alias, p));
7474
}
7575
}
@@ -79,13 +79,12 @@ pub(crate) fn type_proj(input: RelExpr, proj: ast::Project, vars: &Relvars) -> T
7979
}
8080
}
8181

82-
/// Type check and lower a [SqlExpr] into a logical [Expr].
83-
pub(crate) fn type_expr(
84-
vars: &Relvars,
85-
expr: SqlExpr,
86-
expected: Option<&AlgebraicType>,
87-
depth: &mut usize,
88-
) -> TypingResult<Expr> {
82+
// These types determine the size of each stack frame during type checking.
83+
// Changing their sizes will require updating the recursion limit to avoid stack overflows.
84+
const _: () = assert!(size_of::<TypingResult<Expr>>() == 64);
85+
const _: () = assert!(size_of::<SqlExpr>() == 40);
86+
87+
fn _type_expr(vars: &Relvars, expr: SqlExpr, expected: Option<&AlgebraicType>, depth: usize) -> TypingResult<Expr> {
8988
recursion::guard(depth, recursion::MAX_RECURSION_TYP_EXPR, "expr::type_expr")?;
9089

9190
match (expr, expected) {
@@ -125,21 +124,21 @@ pub(crate) fn type_expr(
125124
}))
126125
}
127126
(SqlExpr::Log(a, b, op), None | Some(AlgebraicType::Bool)) => {
128-
let a = type_expr(vars, *a, Some(&AlgebraicType::Bool), depth)?;
129-
let b = type_expr(vars, *b, Some(&AlgebraicType::Bool), depth)?;
127+
let a = _type_expr(vars, *a, Some(&AlgebraicType::Bool), depth + 1)?;
128+
let b = _type_expr(vars, *b, Some(&AlgebraicType::Bool), depth + 1)?;
130129
Ok(Expr::LogOp(op, Box::new(a), Box::new(b)))
131130
}
132131
(SqlExpr::Bin(a, b, op), None | Some(AlgebraicType::Bool)) if matches!(&*a, SqlExpr::Lit(_)) => {
133-
let b = type_expr(vars, *b, None, depth)?;
134-
let a = type_expr(vars, *a, Some(b.ty()), depth)?;
132+
let b = _type_expr(vars, *b, None, depth + 1)?;
133+
let a = _type_expr(vars, *a, Some(b.ty()), depth + 1)?;
135134
if !op_supports_type(op, a.ty()) {
136135
return Err(InvalidOp::new(op, a.ty()).into());
137136
}
138137
Ok(Expr::BinOp(op, Box::new(a), Box::new(b)))
139138
}
140139
(SqlExpr::Bin(a, b, op), None | Some(AlgebraicType::Bool)) => {
141-
let a = type_expr(vars, *a, None, depth)?;
142-
let b = type_expr(vars, *b, Some(a.ty()), depth)?;
140+
let a = _type_expr(vars, *a, None, depth + 1)?;
141+
let b = _type_expr(vars, *b, Some(a.ty()), depth + 1)?;
143142
if !op_supports_type(op, a.ty()) {
144143
return Err(InvalidOp::new(op, a.ty()).into());
145144
}
@@ -152,6 +151,11 @@ pub(crate) fn type_expr(
152151
}
153152
}
154153

154+
/// Type check and lower a [SqlExpr] into a logical [Expr].
155+
pub(crate) fn type_expr(vars: &Relvars, expr: SqlExpr, expected: Option<&AlgebraicType>) -> TypingResult<Expr> {
156+
_type_expr(vars, expr, expected, 0)
157+
}
158+
155159
/// Is this type compatible with this binary operator?
156160
fn op_supports_type(_op: BinOp, t: &AlgebraicType) -> bool {
157161
t.is_bool()

crates/expr/src/statement.rs

Lines changed: 32 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -162,7 +162,7 @@ pub fn type_delete(delete: SqlDelete, tx: &impl SchemaView) -> TypingResult<Tabl
162162
let mut vars = Relvars::default();
163163
vars.insert(table_name.clone(), from.clone());
164164
let expr = filter
165-
.map(|expr| type_expr(&vars, expr, Some(&AlgebraicType::Bool), &mut 0))
165+
.map(|expr| type_expr(&vars, expr, Some(&AlgebraicType::Bool)))
166166
.transpose()?;
167167
Ok(TableDelete {
168168
table: from,
@@ -216,7 +216,7 @@ pub fn type_update(update: SqlUpdate, tx: &impl SchemaView) -> TypingResult<Tabl
216216
vars.insert(table_name.clone(), schema.clone());
217217
let values = values.into_boxed_slice();
218218
let filter = filter
219-
.map(|expr| type_expr(&vars, expr, Some(&AlgebraicType::Bool), &mut 0))
219+
.map(|expr| type_expr(&vars, expr, Some(&AlgebraicType::Bool)))
220220
.transpose()?;
221221
Ok(TableUpdate {
222222
table: schema,
@@ -450,15 +450,16 @@ pub fn compile_sql_stmt<'a>(sql: &'a str, tx: &impl SchemaView, auth: &AuthCtx)
450450

451451
#[cfg(test)]
452452
mod tests {
453-
use spacetimedb_lib::{identity::AuthCtx, AlgebraicType, ProductType};
454-
use spacetimedb_schema::def::ModuleDef;
455-
453+
use super::Statement;
454+
use crate::ast::LogOp;
456455
use crate::check::{
457456
test_utils::{build_module_def, SchemaViewer},
458-
SchemaView, TypingResult,
457+
Relvars, SchemaView, TypingResult,
459458
};
460-
461-
use super::Statement;
459+
use crate::type_expr;
460+
use spacetimedb_lib::{identity::AuthCtx, AlgebraicType, ProductType};
461+
use spacetimedb_schema::def::ModuleDef;
462+
use spacetimedb_sql_parser::ast::{SqlExpr, SqlLiteral};
462463

463464
fn module_def() -> ModuleDef {
464465
build_module_def(vec![
@@ -519,4 +520,27 @@ mod tests {
519520
assert!(result.is_err());
520521
}
521522
}
523+
524+
/// Manually build the AST for a recursive query,
525+
/// because we limit the length of the query to prevent stack overflow on parsing.
526+
/// Exercise the limit [`recursion::MAX_RECURSION_TYP_EXPR`]
527+
#[test]
528+
fn typing_recursion() {
529+
let build_query = |total, sep: char| {
530+
let mut expr = SqlExpr::Lit(SqlLiteral::Bool(true));
531+
for _ in 1..total {
532+
let next = SqlExpr::Log(
533+
Box::new(SqlExpr::Lit(SqlLiteral::Bool(true))),
534+
Box::new(SqlExpr::Lit(SqlLiteral::Bool(false))),
535+
LogOp::And,
536+
);
537+
expr = SqlExpr::Log(Box::new(expr), Box::new(next), LogOp::And);
538+
}
539+
type_expr(&Relvars::default(), expr, Some(&AlgebraicType::Bool))
540+
.map_err(|e| e.to_string().split(sep).next().unwrap_or_default().to_string())
541+
};
542+
assert_eq!(build_query(2_501, ','), Err("Recursion limit exceeded".to_string()));
543+
544+
assert!(build_query(2_500, ',').is_ok());
545+
}
522546
}

crates/sql-parser/src/parser/errors.rs

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ pub enum SqlUnsupported {
5050
#[error("Unsupported FROM expression: {0}")]
5151
From(TableFactor),
5252
#[error("Unsupported set operation: {0}")]
53-
SetOp(SetExpr),
53+
SetOp(Box<SetExpr>),
5454
#[error("Unsupported INSERT expression: {0}")]
5555
Insert(Query),
5656
#[error("Unsupported INSERT value: {0}")]
@@ -94,22 +94,33 @@ pub enum SqlRequired {
9494
}
9595

9696
#[derive(Error, Debug)]
97-
#[error("Recursion limit exceeded, `{message}` limit: {limit}")]
97+
#[error("Recursion limit exceeded, `{source_}`")]
9898
pub struct RecursionError {
99-
pub(crate) limit: usize,
100-
pub(crate) message: String,
99+
pub(crate) source_: &'static str,
101100
}
102101

103102
#[derive(Error, Debug)]
104103
pub enum SqlParseError {
105104
#[error(transparent)]
106-
SqlUnsupported(#[from] SqlUnsupported),
105+
SqlUnsupported(#[from] Box<SqlUnsupported>),
107106
#[error(transparent)]
108-
SubscriptionUnsupported(#[from] SubscriptionUnsupported),
107+
SubscriptionUnsupported(#[from] Box<SubscriptionUnsupported>),
109108
#[error(transparent)]
110109
SqlRequired(#[from] SqlRequired),
111110
#[error(transparent)]
112111
ParserError(#[from] ParserError),
113112
#[error(transparent)]
114113
Recursion(#[from] RecursionError),
115114
}
115+
116+
impl From<SubscriptionUnsupported> for SqlParseError {
117+
fn from(value: SubscriptionUnsupported) -> Self {
118+
SqlParseError::SubscriptionUnsupported(Box::new(value))
119+
}
120+
}
121+
122+
impl From<SqlUnsupported> for SqlParseError {
123+
fn from(value: SqlUnsupported) -> Self {
124+
SqlParseError::SqlUnsupported(Box::new(value))
125+
}
126+
}

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

Lines changed: 18 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ trait RelParser {
6868
op: BinaryOperator::Eq,
6969
right,
7070
},
71-
&mut 0,
71+
0,
7272
)?),
7373
})
7474
}
@@ -207,17 +207,22 @@ pub(crate) fn parse_proj(expr: Expr) -> SqlParseResult<ProjectExpr> {
207207
}
208208
}
209209

210+
// These types determine the size of [`parse_expr`]'s stack frame.
211+
// Changing their sizes will require updating the recursion limit to avoid stack overflows.
212+
const _: () = assert!(size_of::<Expr>() == 168);
213+
const _: () = assert!(size_of::<SqlParseResult<SqlExpr>>() == 40);
214+
210215
/// Parse a scalar expression
211-
pub(crate) fn parse_expr(expr: Expr, depth: &mut usize) -> SqlParseResult<SqlExpr> {
212-
fn signed_num(sign: impl Into<String>, expr: Expr) -> Result<SqlExpr, SqlUnsupported> {
216+
fn parse_expr(expr: Expr, depth: usize) -> SqlParseResult<SqlExpr> {
217+
fn signed_num(sign: impl Into<String>, expr: Expr) -> Result<SqlExpr, Box<SqlUnsupported>> {
213218
match expr {
214219
Expr::Value(Value::Number(n, _)) => Ok(SqlExpr::Lit(SqlLiteral::Num((sign.into() + &n).into_boxed_str()))),
215-
expr => Err(SqlUnsupported::Expr(expr)),
220+
expr => Err(SqlUnsupported::Expr(expr).into()),
216221
}
217222
}
218223
recursion::guard(depth, recursion::MAX_RECURSION_EXPR, "sql-parser::parse_expr")?;
219224
match expr {
220-
Expr::Nested(expr) => parse_expr(*expr, depth),
225+
Expr::Nested(expr) => parse_expr(*expr, depth + 1),
221226
Expr::Value(Value::Placeholder(param)) if &param == ":sender" => Ok(SqlExpr::Param(Parameter::Sender)),
222227
Expr::Value(v) => Ok(SqlExpr::Lit(parse_literal(v)?)),
223228
Expr::UnaryOp {
@@ -243,31 +248,31 @@ pub(crate) fn parse_expr(expr: Expr, depth: &mut usize) -> SqlParseResult<SqlExp
243248
op: BinaryOperator::And,
244249
right,
245250
} => {
246-
let l = parse_expr(*left, depth)?;
247-
let r = parse_expr(*right, depth)?;
251+
let l = parse_expr(*left, depth + 1)?;
252+
let r = parse_expr(*right, depth + 1)?;
248253
Ok(SqlExpr::Log(Box::new(l), Box::new(r), LogOp::And))
249254
}
250255
Expr::BinaryOp {
251256
left,
252257
op: BinaryOperator::Or,
253258
right,
254259
} => {
255-
let l = parse_expr(*left, depth)?;
256-
let r = parse_expr(*right, depth)?;
260+
let l = parse_expr(*left, depth + 1)?;
261+
let r = parse_expr(*right, depth + 1)?;
257262
Ok(SqlExpr::Log(Box::new(l), Box::new(r), LogOp::Or))
258263
}
259264
Expr::BinaryOp { left, op, right } => {
260-
let l = parse_expr(*left, depth)?;
261-
let r = parse_expr(*right, depth)?;
265+
let l = parse_expr(*left, depth + 1)?;
266+
let r = parse_expr(*right, depth + 1)?;
262267
Ok(SqlExpr::Bin(Box::new(l), Box::new(r), parse_binop(op)?))
263268
}
264269
_ => Err(SqlUnsupported::Expr(expr).into()),
265270
}
266271
}
267272

268273
/// Parse an optional scalar expression
269-
pub(crate) fn parse_expr_opt(opt: Option<Expr>, depth: &mut usize) -> SqlParseResult<Option<SqlExpr>> {
270-
opt.map(|expr| parse_expr(expr, depth)).transpose()
274+
pub(crate) fn parse_expr_opt(opt: Option<Expr>) -> SqlParseResult<Option<SqlExpr>> {
275+
opt.map(|expr| parse_expr(expr, 0)).transpose()
271276
}
272277

273278
/// Parse a scalar binary operator

0 commit comments

Comments
 (0)