@panarch PTAL

Thanks for the thorough review. PTAL @panarch

I found an incompatible behavior with MySQL. In MySQL, the behavior regarding null is as follows:

Schema (MySQL v5.7)

CREATE TABLE Test (
    id INTEGER NULL,
    num INTEGER,
    name TEXT
)

Load Data

INSERT INTO Test (id, num, name) VALUES (NULL, 2, "Hello");
INSERT INTO Test (id, num, name) VALUES (1, 9, "World");
INSERT INTO Test (id, num, name) VALUES (3, 4, "Great");

Query 1

SELECT id, num FROM Test WHERE id + 1 IS NULL;

Query 2

SELECT id + 1 FROM Test;

Query 3

SELECT id, num FROM Test WHERE id = NULL;

No results.

Query 4

SELECT id, num FROM Test WHERE id IS NULL;

But in GlueSQL, NULL + 1 = 1 and NULL = NULL is true. Do we need to change the behavior on null to be consistent with MySQL?

Wow, NULL has lots of stuff to think about, thanks for taking this carefully.
Existing code looks very good, and if you are ok then can I request small changes for nullable test codes?
Now tests/nullable.rs has lots of test cases so it would be good to simplify little bit, like this

• src/tests/blend.rs

And your second comment about MySQL compatible issue looks also very interesting.
My opinion is this.

1. NULL IS NULL, NULL = NULL is ok.
   I think it's better to deal NULL as a type, not something systemic value.
   Nowadays SQL is not only for programmers but also marketers and many others.
   So, forcing them to understand something systemic issue is not affordable.
   Without that, GlueSQL itself already handles NULL as a type, not something special else.
   So naturally it's ok to see NULL IS NULL as true.

2. NULL + 1 should be NULL.
   Good point, I think it should be fixed and current implementation I made is wrong.
   Existing implementation was simply because I didn't care about this case, I made that work during I'm working on aggregate task.
   This one is related to gluesql::data::Value and sqlparser::ast::Value as AstValue.
   I will simply divide using value for gluesql::data::Value and literal for sqlparser::ast::Value as AstValue.
   There exists 3 possible scenarios.

• literal + literal
• value + value
• value + literal

But the last one, we don't need to care value + literal because it starts by converting literal to value. so it becomes value + value case.

Two things required to be changed.

• literal + literal
  Those functions are defined in src/executor/evaluate/evaluated.rs
  literal_add, literal_subtract, literal_multiply and literal_divide.
  Existings only take AstValue::Number(_) type but it should also handle AstValue::Null.

• value + value
  It's defined in src/data/value.rs.
  In impl Value, there exists 4 functions add, subtract, multiply and divide.
  These have wrong implementations, so you can simply fix it to show NULL + 1 to NULL.
  But the problem is, after you fix that, integration tests will fail. It is because of SUM aggregation.
  In src/executor/aggregate.rs,

"SUM" => {
    let value_to_sum = get_first_value(args)?;

    match aggregated.get(func) {
        Some((current, value)) => {
            if &index <= current {
                return Ok(aggregated);
            }

            Ok(aggregated.update(func, (index, value.add(value_to_sum)?)))
        }
        None => Ok(aggregated.update(func, (index, value_to_sum.clone()))),
    }
}

This uses value.add(value_to_sum)?, but after changing NULL + 1 behavior, it should be also fixed.
You may be able to adapt similar approach from COUNT implementation.

So my opinion is..

• NULL = NULL is true => OK.
• NULL + 1 should be NULL => Change required.

How do you think of this?

I agree with this:

• NULL = NULL is true => OK.
• NULL + 1 should be NULL => Change required.

On the mysql compatibility issue, I think I can submit another PR to fix it.
In this PR, I will continue to refine the test cases, but will not modify the null related behavior

That sounds good, I also agree that it's good to divide NULL compatibility issue using another PR.
After you refine the test cases, then I'll merge it.

@panarch PTAL

Test cases looks perfect!
And sorry for the late comment but here is something that I'd like to get your opinion.

pub fn is_some(&self) -> bool {
    match self {
        Evaluated::ValueRef(v) => v.is_some(),
        Evaluated::Value(v) => v.is_some(),
        Evaluated::Literal(v) => *v != AstValue::Null,
        Evaluated::LiteralRef(v) => **v != AstValue::Null,
        Evaluated::StringRef(_v) => true,
    }
}

here, *v != AstValue::Null and **v != AstValue::Null uses dereferencing, and I usually prefer using & than *.
The reason is...
In this code we cannot check whether v in Evaluated::Literal(v) implements Copy trait or not.
So for making sure that *v doesn't call unnecessary clone(), we should check the code of AstValue.

• sqlparser-rs - src/ast/value.rs

/// Primitive SQL values such as number and string
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
pub enum Value {
    /// Numeric literal
    #[cfg(not(feature = "bigdecimal"))]
    Number(String),
   ...
...

In this case, it's ok. that sqlparser Value does not implement Copy trait.

But when we use v != &AstValue::Null, we don't need to worry about unnecessary clone() may happen or not, because it will not happen.
So, I think using referencing is better choice, how do you think?

@panarch I agree with you, and I've made the relevant changes.

Thanks! merging 👍