Finalize NULL related decisions (#2089)

* add test suite for null protected operations

* fix test for dbs with NULLS keyword

* fix mysql not and snowflake test

* make IS NULL and IS NOT NULL official

* make = NULL and != NULL warn

* fix tests IS NULL change broke

* allow partial null comparison

* add tests for partial is null

* add tests for warning on = null

Author: mtoy-googly-moogly

demo/malloy-duckdb-wasm/README.md

@@ -73,7 +73,7 @@ source: airports is duckdb,table('airports.parquet') extend {
     heliport_count is airport_count {where: fac_type = 'HELIPORT'}
 
   view: by_state is {
-    where: state != null
+    where: state is not null
     group_by: state
     aggregate: airport_count
   }

packages/malloy-db-snowflake/src/snowflake_connection.spec.ts

@@ -66,7 +66,7 @@ describe('db:Snowflake', () => {
       .loadModel("source: aircraft is snowflake.table('malloytest.aircraft')")
       .loadQuery(
         `run:  aircraft -> {
-        where: state != null
+        where: state is not null
         aggregate: cnt is count()
         group_by:  state}`
       )

packages/malloy/src/dialect/dialect.ts

@@ -173,13 +173,14 @@ export abstract class Dialect {
   // MYSQL doesn't have full join, maybe others.
   supportsFullJoin = true;
 
-  nativeBoolean = true;
-
   // Can have arrays of arrays
   nestedArrays = true;
   // An array or record will reveal type of contents on schema read
   compoundObjectInSchema = true;
 
+  // No true boolean type, e.g. true=1 and false=0, set this to true
+  booleanAsNumbers = false;
+
   abstract getDialectFunctionOverrides(): {
     [name: string]: DialectFunctionOverloadDef[];
   };

packages/malloy/src/dialect/mysql/mysql.ts

@@ -107,7 +107,7 @@ export class MySQLDialect extends Dialect {
   defaultDecimalType = 'DECIMAL';
   udfPrefix = 'ms_temp.__udf';
   hasFinalStage = false;
-  // TODO: this may not be enough for lager casts.
+  // TODO: this may not be enough for larger casts.
   stringTypeName = 'VARCHAR(255)';
   divisionIsInteger = true;
   supportsSumDistinctFunction = true;
@@ -121,13 +121,13 @@ export class MySQLDialect extends Dialect {
   supportsQualify = false;
   supportsNesting = true;
   experimental = false;
-  nativeBoolean = false;
   supportsFullJoin = false;
   supportsPipelinesInViews = false;
   readsNestedData = false;
   supportsComplexFilteredSources = false;
   supportsArraysInData = false;
   compoundObjectInSchema = false;
+  booleanAsNumbers = true;
 
   malloyTypeToSQLType(malloyType: AtomicTypeDef): string {
     switch (malloyType.type) {

packages/malloy/src/lang/ast/expressions/expr-not.ts

@@ -36,6 +36,11 @@ export class ExprNot extends Unary {
 
   getExpression(fs: FieldSpace): ExprValue {
     const notThis = this.expr.getExpression(fs);
+    if (fs.dialectObj()?.booleanAsNumbers) {
+      if (this.legalChildTypes.find(t => t.type === 'number') === undefined) {
+        this.legalChildTypes.push(TDU.numberT);
+      }
+    }
     const doNot = this.typeCheck(this.expr, notThis);
     return {
       ...notThis,

packages/malloy/src/lang/ast/expressions/expr-null.ts

@@ -21,15 +21,79 @@
  * SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
  */
 
+import {BinaryMalloyOperator, FieldSpace} from '..';
 import {ExprValue, literalExprValue} from '../types/expr-value';
-import {ExpressionDef} from '../types/expression-def';
+import {ATNodeType, ExpressionDef} from '../types/expression-def';
+
+function doIsNull(fs: FieldSpace, op: string, expr: ExpressionDef): ExprValue {
+  const nullCmp = expr.getExpression(fs);
+  nullCmp.type = 'boolean';
+  nullCmp.value = {
+    node: op === '=' ? 'is-null' : 'is-not-null',
+    e: nullCmp.value,
+  };
+  return nullCmp;
+}
 
 export class ExprNULL extends ExpressionDef {
   elementType = 'NULL';
+
   getExpression(): ExprValue {
     return literalExprValue({
       dataType: {type: 'null'},
       value: {node: 'null'},
     });
   }
+
+  apply(
+    fs: FieldSpace,
+    op: BinaryMalloyOperator,
+    left: ExpressionDef
+  ): ExprValue {
+    if (op === '!=' || op === '=') {
+      return doIsNull(fs, op, left);
+    }
+    return super.apply(fs, op, left, true);
+  }
+}
+
+export class PartialIsNull extends ExpressionDef {
+  elementType = '<=> NULL';
+  constructor(readonly op: '=' | '!=') {
+    super();
+  }
+
+  apply(fs: FieldSpace, op: string, expr: ExpressionDef): ExprValue {
+    return doIsNull(fs, this.op, expr);
+  }
+
+  requestExpression(_fs: FieldSpace): ExprValue | undefined {
+    return undefined;
+  }
+
+  getExpression(_fs: FieldSpace): ExprValue {
+    return this.loggedErrorExpr(
+      'partial-as-value',
+      'Partial null check does not have a value'
+    );
+  }
+
+  atNodeType(): ATNodeType {
+    return ATNodeType.Partial;
+  }
+}
+
+export class ExprIsNull extends ExpressionDef {
+  elementType = 'is null';
+  constructor(
+    readonly expr: ExpressionDef,
+    readonly op: '=' | '!='
+  ) {
+    super();
+    this.has({expr});
+  }
+
+  getExpression(fs: FieldSpace): ExprValue {
+    return doIsNull(fs, this.op, this.expr);
+  }
 }

packages/malloy/src/lang/ast/types/expression-def.ts

@@ -300,25 +300,6 @@ function regexEqual(left: ExprValue, right: ExprValue): Expr | undefined {
   return undefined;
 }
 
-function nullCompare(
-  left: ExprValue,
-  op: string,
-  right: ExprValue
-): Expr | undefined {
-  const not = op === '!=' || op === '!~';
-  const nullCmp = not ? 'is-not-null' : 'is-null';
-  if (left.type === 'null' || right.type === 'null') {
-    if (left.type !== 'null') {
-      return {node: nullCmp, e: left.value};
-    }
-    if (right.type !== 'null') {
-      return {node: nullCmp, e: right.value};
-    }
-    return {node: not ? 'false' : 'true'};
-  }
-  return undefined;
-}
-
 function equality(
   fs: FieldSpace,
   left: ExpressionDef,
@@ -349,30 +330,21 @@ function equality(
     kids: {left: lhs.value, right: rhs.value},
   };
 
-  if (lhs.type !== 'error' && rhs.type !== 'error') {
-    switch (op) {
-      case '~':
-      case '!~': {
-        if (lhs.type !== 'string' || rhs.type !== 'string') {
-          let regexCmp = regexEqual(lhs, rhs);
-          if (regexCmp) {
-            if (op[0] === '!') {
-              regexCmp = {node: 'not', e: {...regexCmp}};
-            }
-          } else {
-            throw new TypeMismatch(
-              "Incompatible types for match('~') operator"
-            );
-          }
-          value = regexCmp;
+  if (
+    lhs.type !== 'error' &&
+    rhs.type !== 'error' &&
+    (op === '~' || op === '!~')
+  ) {
+    if (lhs.type !== 'string' || rhs.type !== 'string') {
+      let regexCmp = regexEqual(lhs, rhs);
+      if (regexCmp) {
+        if (op[0] === '!') {
+          regexCmp = {node: 'not', e: {...regexCmp}};
         }
-        break;
-      }
-      case '=':
-      case '!=': {
-        value = nullCompare(lhs, op, rhs) ?? value;
-        break;
+      } else {
+        throw new TypeMismatch("Incompatible types for match('~') operator");
       }
+      value = regexCmp;
     }
   }
 

packages/malloy/src/lang/grammar/MalloyParser.g4

@@ -602,7 +602,7 @@ fieldExpr
   | fieldExpr BAR partialAllowedFieldExpr                  # exprOrTree
   | fieldExpr compareOp fieldExpr                          # exprCompare
   | fieldExpr NOT? LIKE fieldExpr                          # exprWarnLike
-  | fieldExpr IS NOT? NULL                                 # exprWarnNullCmp
+  | fieldExpr IS NOT? NULL                                 # exprNullCheck
   | fieldExpr NOT? IN OPAREN fieldExprList CPAREN          # exprWarnIn
   | fieldExpr QMARK partialAllowedFieldExpr                # exprApply
   | NOT fieldExpr                                          # exprNot
@@ -624,9 +624,19 @@ fieldExpr
   | ungroup OPAREN fieldExpr (COMMA fieldName)* CPAREN     # exprUngroup
   ;
 
+partialCompare
+  : compareOp fieldExpr
+  ;
+
+partialTest
+  : partialCompare
+  | IS NOT? NULL
+  ;
+
 partialAllowedFieldExpr
-  : OPAREN compareOp? fieldExpr CPAREN
-  | compareOp? fieldExpr
+  : partialTest
+  | OPAREN partialTest CPAREN
+  | fieldExpr
   ;
 
 fieldExprList

packages/malloy/src/lang/malloy-to-ast.ts

@@ -1273,21 +1273,40 @@ export class MalloyToAST
     return this.astAt(new ast.ExprCoalesce(left, right), pcx);
   }
 
+  visitPartialCompare(pcx: parse.PartialCompareContext): ast.PartialCompare {
+    const partialOp = pcx.compareOp().text;
+    if (ast.isComparison(partialOp)) {
+      return this.astAt(
+        new ast.PartialCompare(partialOp, this.getFieldExpr(pcx.fieldExpr())),
+        pcx
+      );
+    }
+    throw this.internalError(
+      pcx,
+      `partial comparison '${partialOp}' not recognized`
+    );
+  }
+
+  visitPartialTest(pcx: parse.PartialTestContext): ast.ExpressionDef {
+    const cmp = pcx.partialCompare();
+    if (cmp) {
+      return this.visitPartialCompare(cmp);
+    }
+    return this.astAt(new ast.PartialIsNull(pcx.NOT() ? '!=' : '='), pcx);
+  }
+
   visitPartialAllowedFieldExpr(
     pcx: parse.PartialAllowedFieldExprContext
   ): ast.ExpressionDef {
-    const fieldExpr = this.getFieldExpr(pcx.fieldExpr());
-    const partialOp = pcx.compareOp()?.text;
-    if (partialOp) {
-      if (ast.isComparison(partialOp)) {
-        return this.astAt(new ast.PartialCompare(partialOp, fieldExpr), pcx);
-      }
-      throw this.internalError(
-        pcx,
-        `partial comparison '${partialOp}' not recognized`
-      );
+    const exprCx = pcx.fieldExpr();
+    if (exprCx) {
+      return this.getFieldExpr(exprCx);
     }
-    return fieldExpr;
+    const partialCx = pcx.partialTest();
+    if (partialCx) {
+      return this.visitPartialTest(partialCx);
+    }
+    throw this.internalError(pcx, 'impossible partial');
   }
 
   visitExprString(pcx: parse.ExprStringContext): ast.ExprString {
@@ -1348,6 +1367,24 @@ export class MalloyToAST
     const left = this.getFieldExpr(pcx.fieldExpr(0));
     const right = this.getFieldExpr(pcx.fieldExpr(1));
     if (ast.isEquality(op)) {
+      const wholeRange = this.parseInfo.rangeFromContext(pcx);
+      if (right instanceof ast.ExprNULL) {
+        if (op === '=') {
+          this.warnWithReplacement(
+            'sql-is-null',
+            "Use 'is null' to check for NULL instead of '= null'",
+            wholeRange,
+            `${this.getSourceCode(pcx.fieldExpr(0))} is null`
+          );
+        } else if (op === '!=') {
+          this.warnWithReplacement(
+            'sql-is-not-null',
+            "Use 'is not null' to check for NULL instead of '!= null'",
+            wholeRange,
+            `${this.getSourceCode(pcx.fieldExpr(0))} is not null`
+          );
+        }
+      }
       return this.astAt(new ast.ExprEquality(left, op, right), pcx);
     } else if (ast.isComparison(op)) {
       return this.astAt(new ast.ExprCompare(left, op, right), pcx);
@@ -2124,29 +2161,10 @@ export class MalloyToAST
     );
   }
 
-  visitExprWarnNullCmp(pcx: parse.ExprWarnNullCmpContext): ast.ExprCompare {
-    let op: ast.CompareMalloyOperator = '=';
+  visitExprNullCheck(pcx: parse.ExprNullCheckContext): ast.ExprIsNull {
     const expr = pcx.fieldExpr();
-    const wholeRange = this.parseInfo.rangeFromContext(pcx);
-    if (pcx.NOT()) {
-      op = '!=';
-      this.warnWithReplacement(
-        'sql-is-not-null',
-        "Use '!= NULL' to check for NULL instead of 'IS NOT NULL'",
-        wholeRange,
-        `${this.getSourceCode(expr)} != null`
-      );
-    } else {
-      this.warnWithReplacement(
-        'sql-is-null',
-        "Use '= NULL' to check for NULL instead of 'IS NULL'",
-        wholeRange,
-        `${this.getSourceCode(expr)} = null`
-      );
-    }
-    const nullExpr = new ast.ExprNULL();
     return this.astAt(
-      new ast.ExprCompare(this.getFieldExpr(expr), op, nullExpr),
+      new ast.ExprIsNull(this.getFieldExpr(expr), pcx.NOT() ? '!=' : '='),
       pcx
     );
   }