Implement a temporary type check validation on sql compiling #1456

mamcx · 2024-06-21T17:16:24Z

Description of Changes

Stop early type mismatch on SQL queries to avoid panic on execution.

For now, it is only limited to SELECT...WHERE & JOIN clauses.

NOTE: This is a temporary step until the new query engine lands.

API and ABI breaking changes

It could reject queries that incorrectly passed compilation, but it is a desirable outcome.

Expected complexity level and risk

1

Testing

Check types on WHERE caluses
Check types on JOIN caluses

joshua-spacetime · 2024-06-21T20:34:40Z

crates/core/src/sql/compiler.rs

+
+        assert_eq!(
+            compile_sql(&db, &db.begin_tx(), sql).map_err(|e| e.to_string()),
+            Err("SqlError: Type Mismatch: `table#4097.col#0: Some(Builtin(U64))` != `\"161853\": Some(Builtin(String))`, executing: `SELECT * FROM PlayerState WHERE entity_id = '161853'`".into())


Do you want to assert on the error message, or would the error type suffice?

I mean to see how the error will look.

crates/sats/src/sum_type.rs

Centril · 2024-06-24T09:39:08Z

crates/core/src/sql/compiler.rs

+
+        assert_eq!(
+            compile_sql(&db, &db.begin_tx(), sql).map_err(|e| e.to_string()),
+            Err("SqlError: Type Mismatch: `table#4097.col#0: Some(Builtin(U64))` != `\"161853\": Some(Builtin(String))`, executing: `SELECT * FROM PlayerState WHERE entity_id = '161853'`".into())


Let's get rid of these Some(_)s and I believe we also have access to information sufficient to transform a FieldName into a {table_name}.{column_name} string.

crates/core/src/sql/type_check.rs

Centril · 2024-06-24T10:08:53Z

crates/core/src/sql/type_check.rs

+                // The `SumType` returns `None` on `type_of` so we need to check against the value
+                if let (Some(AlgebraicType::Sum(ty_lhs)), _) = (&ty_lhs, &ty_rhs) {
+                    // We can use in `sql` coercion from string to sum type: `tag = 'name'`
+                    if let FieldOp::Field(FieldExpr::Value(x)) = rhs.as_ref() {


Let's use a more descriptive name here than x, e.g., val_rhs

Centril · 2024-06-24T10:10:33Z

crates/core/src/sql/type_check.rs

+                    if let FieldOp::Field(FieldExpr::Value(x)) = rhs.as_ref() {
+                        if let Some(x) = x.as_string() {
+                            if ty_lhs.get_variant_simple(x).is_some() {
+                                return Ok(Some(AlgebraicType::Sum(ty_lhs.clone())));


The type of FieldOp::Cmp can only be Bool.

More exactly, the final will be. But here what I do is to extract which type each arm have.

In the case of a ill-typed query:

let sql = "SELECT * FROM PlayerState WHERE entity_id = '161853'"; [crates/core/src/sql/type_check.rs:61:17] &ty_lhs = Some( Builtin( U64, ), ) [crates/core/src/sql/type_check.rs:61:17] &ty_rhs = Some( Builtin( String, ), )

So when I check both sides are Eq then the result will be Bool. ie: This is a type check no a type unification step, that arguably should also be part of this, but I defer for the refactor.

More exactly, the final will be. But here what I do is to extract which type each arm have.

Not just the final result, but also at any logical operator anywhere in the AST. When you have e.g., WHERE a = x AND b = y AND c = z each col = val "arm" should be found to be typed at Bool. This is why you end up with a hole in the type checker at https://github.com/clockworklabs/SpacetimeDB/pull/1456/files#r1650781933 which we then have to a have a check for later in the compilation process.

So when I check both sides are Eq then the result will be Bool. ie: This is a type check no a type unification step, that arguably should also be part of this, but I defer for the refactor.

I'd like to get this right before a larger refactor, and have a sound type checker in a single place which we then can refactor with confidence.

Centril · 2024-06-24T10:33:17Z

crates/core/src/sql/type_check.rs

+                    Ok(ty_lhs)
+                }
+            }
+            OpQuery::Logic(_) => Ok(Some(AlgebraicType::Bool)),


This is not sound. You are not checking the leaves of the operands, e.g., the foo.bar = xyzs inside, and not ensuring that the leaves are actually typed at Bool.

Centril · 2024-06-24T10:39:57Z

crates/core/src/sql/type_check.rs

+}
+
+impl TypeCheck for QueryFragment<'_, Selection> {
+    fn type_check(&self) -> Result<(), PlanError> {


Type checking a Selection is exactly like type checking a OpQuery::Logic except that the latter can have many operands whereas the former only has one. In both cases, the expected type is Bool (and this knowledge should be used in error UX), and operands need to be checked.

The type_check module you are adding here should I think replace check_field_op and check_field_op_logics.

bfops · 2024-06-24T17:58:55Z

I'm labeling this as backwards-compatible, even though it's technically not quite. It's backwards-compatible given that bad queries crashed the server.

gefjon · 2024-06-25T04:38:21Z

I'm labeling this as backwards-compatible, even though it's technically not quite. It's backwards-compatible given that bad queries crashed the server.

Backwards compatibility is not the same as bug-for-bug compatibility.

crates/core/src/sql/ast.rs

crates/core/src/sql/compiler.rs

Centril · 2024-07-01T10:34:49Z

crates/core/src/sql/type_check.rs

+            }
+
+            _ => {
+                // TODO: Other options deferred for the new query engine


I think this should be done here, so that type checking is sound.
What remains is checking Insert, Update, and Delete in fairly simple ways.

The discussion with Joshua was to keep this as a temporal solution because the proposal for SQL APIs touches this in ways that will invalidate the changes.

crates/core/src/sql/type_check.rs

Centril · 2024-07-01T11:18:58Z

crates/core/src/sql/type_check.rs

+            patch_type(lhs, &mut ty_lhs, &ty_rhs)?;
+            patch_type(rhs, &mut ty_rhs, &ty_lhs)?;
+
+            // If both sides are the same type, then return `Bool` to indicate a logical comparison


A FieldOp::Cmp is unconditionally typed at Bool so I don't understand why this is worded as conditioned...

crates/core/src/sql/type_check.rs

crates/core/src/sql/compiler.rs

Centril · 2024-07-01T12:03:24Z

crates/core/src/sql/compiler.rs

+            &[("entity_id", AlgebraicType::U64)],
+            &[(0.into(), "entity_id")],
+        )?;
+        let sql = "SELECT * FROM PlayerState WHERE entity_id = '161853'";


Please also test the query "SELECT * FROM PlayerState WHERE entity_id"; which should fail but I think passes with your current code.

Co-authored-by: Mazdak Farrokhzad <[email protected]> Signed-off-by: Mario Montoya <[email protected]>

Centril

As a stop-gap measure, this works, but we should reimplement this on a logical plan before 1.0

mamcx added bugfix Fixes something that was expected to work differently release-0.11 labels Jun 21, 2024

mamcx requested a review from joshua-spacetime June 21, 2024 17:16

mamcx self-assigned this Jun 21, 2024

joshua-spacetime approved these changes Jun 21, 2024

View reviewed changes

Centril requested changes Jun 24, 2024

View reviewed changes

bfops added the backward-compatible label Jun 24, 2024

mamcx force-pushed the mamcx/fix_malformed_query branch 4 times, most recently from 8e64531 to d9730e2 Compare June 26, 2024 18:21

Centril requested changes Jul 1, 2024

View reviewed changes

mamcx marked this pull request as draft July 3, 2024 15:23

mamcx added the reassessing Taking a step back to reconsider the problem/scope label Jul 3, 2024

mamcx and others added 6 commits July 5, 2024 13:46

Implement q temporary type check validation on sql compiling

678e91a

Apply suggestions from code review

4af1d28

Co-authored-by: Mazdak Farrokhzad <[email protected]> Signed-off-by: Mario Montoya <[email protected]>

Make error messages more readable & fix unsound type check

9d55166

Add comment on ugly hack

8eff47b

Apply suggestions from code review

9ce11e1

Co-authored-by: Mazdak Farrokhzad <[email protected]> Signed-off-by: Mario Montoya <[email protected]>

Clippy

9d1c7fc

mamcx force-pushed the mamcx/fix_malformed_query branch from edd8bc3 to 9d1c7fc Compare July 5, 2024 18:46

Reduce the implementation to only cover the bug case

d33bdf4

mamcx marked this pull request as ready for review July 9, 2024 15:39

Centril approved these changes Jul 9, 2024

View reviewed changes

mamcx added this pull request to the merge queue Jul 9, 2024

Merged via the queue into master with commit 2ad91b6 Jul 9, 2024
6 checks passed

mamcx removed the reassessing Taking a step back to reconsider the problem/scope label Jul 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement a temporary type check validation on sql compiling #1456

Implement a temporary type check validation on sql compiling #1456

mamcx commented Jun 21, 2024

joshua-spacetime Jun 21, 2024

mamcx Jun 21, 2024

Centril Jun 24, 2024

Centril Jun 24, 2024

Centril Jun 24, 2024

mamcx Jun 24, 2024

Centril Jun 24, 2024

Centril Jun 24, 2024

Centril Jun 24, 2024

bfops commented Jun 24, 2024

gefjon commented Jun 25, 2024

Centril Jul 1, 2024

mamcx Jul 1, 2024

Centril Jul 1, 2024

Centril Jul 1, 2024

Centril left a comment

Implement a temporary type check validation on sql compiling #1456

Implement a temporary type check validation on sql compiling #1456

Conversation

mamcx commented Jun 21, 2024

Description of Changes

API and ABI breaking changes

Expected complexity level and risk

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bfops commented Jun 24, 2024

gefjon commented Jun 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Centril left a comment

Choose a reason for hiding this comment