-
Notifications
You must be signed in to change notification settings - Fork 852
Description
Summary
ScalarExpr::as_expr() lowers AsyncFunctionCall (e.g. nextval) to a dummy ColumnRef, which causes Expr::is_deterministic() to return true — a false positive. This is a lossy abstraction boundary: callers of as_expr() silently lose async function semantics.
Concrete bug this caused
In interpreter_table_modify_column.rs, the default-only modify path used matches!(&scalar_expr, ScalarExpr::AsyncFunctionCall(_)) to detect nextval before falling through to as_expr().is_deterministic(). But parse_and_bind() wraps type-mismatched results in CastExpr, so nextval(seq) (returns UInt64) on an INT column becomes CastExpr(AsyncFunctionCall) — the top-level matches! misses it entirely.
The workaround is to use default_value_evaluable() which recursively walks the tree. But this is still fragile.
Proposed improvements
ScalarExpr::is_deterministic(): Add a native method that handlesAsyncFunctionCallwithout relying on the lossyas_expr()lowering:
impl ScalarExpr {
fn is_deterministic(&self, registry: &FunctionRegistry) -> bool {
match self {
ScalarExpr::AsyncFunctionCall(_) => false,
_ => self.as_expr()
.map(|e| e.is_deterministic(registry))
.unwrap_or(false),
}
}
}- Rename
default_value_evaluable(): The current name is misleading — it's a generalScalarExprmethod, not specific to default values. The return type(bool, bool)lacks self-documentation. Consider:- Splitting into
contains_nextval()andis_compile_time_evaluable() - Or returning a named struct instead of a tuple
- Splitting into