fix null hangling in mvOverlap and mvContains (#306)

implydata · Jan 29, 2024 · f892b5d · f892b5d
1 parent c320207
commit f892b5d
Show file tree

Hide file tree

Showing 6 changed files with 98 additions and 18 deletions.
diff --git a/src/expressions/baseExpression.ts b/src/expressions/baseExpression.ts
@@ -248,7 +248,7 @@ export interface ExpressionValue {
   outputType?: PlyTypeSimple;
   tuning?: string;
   sql?: string;
-  mvArray?: string[];
+  mvArray?: (string | null)[];
   ipToSearch?: Ip;
   ipSearchType?: string;
   bounds?: string;

diff --git a/src/expressions/isExpression.ts b/src/expressions/isExpression.ts
@@ -16,6 +16,7 @@
 
 import { NumberRange, PlywoodValue, Set, TimeRange } from '../datatypes';
 import { SQLDialect } from '../dialect/baseDialect';
+import { handleNullCheckIfNeeded } from '../helper';
 
 import {
   ChainableUnaryExpression,
@@ -56,23 +57,22 @@ export class IsExpression extends ChainableUnaryExpression {
     operandSQL: string,
     expressionSQL: string,
   ): string {
-    let expressionSet = this.expression.getLiteralValue();
+    const expressionSet = this.expression.getLiteralValue();
     if (expressionSet instanceof Set) {
       if (expressionSet.empty()) return 'FALSE';
 
       switch (this.expression.type) {
         case 'SET/STRING':
         case 'SET/NUMBER': {
-          let nullCheck: string = null;
-          if (expressionSet.has(null)) {
-            nullCheck = `(${operandSQL} IS NULL)`;
-            expressionSet = expressionSet.remove(null);
-          }
-
-          const inCheck = `${operandSQL} IN (${expressionSet.elements
-            .map((v: any) => (typeof v === 'number' ? v : dialect.escapeLiteral(v)))
-            .join(',')})`;
-          return nullCheck ? `(${nullCheck} OR ${inCheck})` : inCheck;
+          return handleNullCheckIfNeeded(
+            expressionSet.elements,
+            `${operandSQL} IS NULL`,
+            'OR',
+            withoutNull =>
+              `${operandSQL} IN (${withoutNull
+                .map((v: any) => (typeof v === 'number' ? v : dialect.escapeLiteral(v)))
+                .join(',')})`,
+          );
         }
 
         default:

diff --git a/src/expressions/mvContainsExpression.ts b/src/expressions/mvContainsExpression.ts
@@ -18,6 +18,7 @@ import { generalArraysEqual } from 'immutable-class';
 
 import { PlywoodValue } from '../datatypes';
 import { SQLDialect } from '../dialect/baseDialect';
+import { handleNullCheckIfNeeded } from '../helper';
 
 import { ChainableExpression, Expression, ExpressionJS, ExpressionValue } from './baseExpression';
 
@@ -29,7 +30,7 @@ export class MvContainsExpression extends ChainableExpression {
     return new MvContainsExpression(value);
   }
 
-  public mvArray: string[];
+  public mvArray: (string | null)[];
 
   constructor(parameters: ExpressionValue) {
     super(parameters, dummyObject);
@@ -70,7 +71,9 @@ export class MvContainsExpression extends ChainableExpression {
   }
 
   protected _getSQLChainableHelper(dialect: SQLDialect, operandSQL: string): string {
-    return dialect.mvContainsExpression(operandSQL, this.mvArray);
+    return handleNullCheckIfNeeded(this.mvArray, `${operandSQL} IS NULL`, 'AND', withoutNull =>
+      dialect.mvContainsExpression(operandSQL, withoutNull),
+    );
   }
 }
 

diff --git a/src/expressions/mvOverlapExpression.ts b/src/expressions/mvOverlapExpression.ts
@@ -18,6 +18,7 @@ import { generalArraysEqual } from 'immutable-class';
 
 import { PlywoodValue } from '../datatypes';
 import { SQLDialect } from '../dialect/baseDialect';
+import { handleNullCheckIfNeeded } from '../helper';
 
 import { ChainableExpression, Expression, ExpressionJS, ExpressionValue } from './baseExpression';
 
@@ -29,7 +30,7 @@ export class MvOverlapExpression extends ChainableExpression {
     return new MvOverlapExpression(value);
   }
 
-  public mvArray: string[];
+  public mvArray: (string | null)[];
 
   constructor(parameters: ExpressionValue) {
     super(parameters, dummyObject);
@@ -70,7 +71,9 @@ export class MvOverlapExpression extends ChainableExpression {
   }
 
   protected _getSQLChainableHelper(dialect: SQLDialect, operandSQL: string): string {
-    return dialect.mvOverlapExpression(operandSQL, this.mvArray);
+    return handleNullCheckIfNeeded(this.mvArray, `${operandSQL} IS NULL`, 'OR', withoutNull =>
+      dialect.mvOverlapExpression(operandSQL, withoutNull),
+    );
   }
 }
 

diff --git a/src/helper/utils.ts b/src/helper/utils.ts
@@ -163,3 +163,17 @@ export function pipeWithError(src: ReadableStream, dest: WritableStream): any {
   src.on('error', (e: Error) => dest.emit('error', e));
   return dest;
 }
+
+export function handleNullCheckIfNeeded<T>(
+  xs: T[],
+  nullCheck: string,
+  orAnd: 'OR' | 'AND',
+  fn: (withoutNull: T[]) => string,
+): string {
+  const withoutNull = xs.filter(x => x != null);
+  if (withoutNull.length === xs.length) {
+    return fn(xs);
+  } else {
+    return `(${nullCheck} ${orAnd} ${fn(withoutNull)})`;
+  }
+}
diff --git a/test/simulate/simulateDruidSql.mocha.js b/test/simulate/simulateDruidSql.mocha.js
@@ -223,7 +223,67 @@ describe('simulate DruidSql', () => {
             sqlTimeZone: 'Etc/UTC',
           },
           query:
-            'SELECT\n"tags" AS "Tag"\nFROM "dia.monds" AS t\nWHERE (((("pugs" IS NULL) OR "pugs" IN (\'pugA\',\'pugB\',\'null\'))) IS NOT TRUE AND (("tags" IS NULL) OR "tags" IN (\'tagA\',\'tagB\',\'null\')))\nGROUP BY 1',
+            'SELECT\n"tags" AS "Tag"\nFROM "dia.monds" AS t\nWHERE ((("pugs" IS NULL OR "pugs" IN (\'pugA\',\'pugB\',\'null\'))) IS NOT TRUE AND ("tags" IS NULL OR "tags" IN (\'tagA\',\'tagB\',\'null\')))\nGROUP BY 1',
+        },
+      ],
+    ]);
+  });
+
+  it('works with null and null string are both included in a filter expression (mvOverlap)', () => {
+    const ex = ply()
+      .apply('diamonds', $('diamonds').filter($('tags').mvOverlap(['tagA', 'tagB', null, 'null'])))
+      .apply('Tags', $('diamonds').split('$tags', 'Tag'));
+
+    const queryPlan = ex.simulateQueryPlan({
+      diamonds: External.fromJS({
+        engine: 'druidsql',
+        version: '0.20.0',
+        source: 'dia.monds',
+        timeAttribute: 'time',
+        attributes,
+        allowSelectQueries: true,
+        filter: $('pugs').mvOverlap(['pugA', 'pugB', null, 'null']).not(),
+      }),
+    });
+    expect(queryPlan.length).to.equal(1);
+    expect(queryPlan).to.deep.equal([
+      [
+        {
+          context: {
+            sqlTimeZone: 'Etc/UTC',
+          },
+          query:
+            'SELECT\n"tags" AS "Tag"\nFROM "dia.monds" AS t\nWHERE ((("pugs" IS NULL OR MV_OVERLAP("pugs", ARRAY[\'pugA\',\'pugB\',\'null\']))) IS NOT TRUE AND ("tags" IS NULL OR MV_OVERLAP("tags", ARRAY[\'tagA\',\'tagB\',\'null\'])))\nGROUP BY 1',
+        },
+      ],
+    ]);
+  });
+
+  it('works with null and null string are both included in a filter expression (mvContains)', () => {
+    const ex = ply()
+      .apply('diamonds', $('diamonds').filter($('tags').mvContains(['tagA', 'tagB', null, 'null'])))
+      .apply('Tags', $('diamonds').split('$tags', 'Tag'));
+
+    const queryPlan = ex.simulateQueryPlan({
+      diamonds: External.fromJS({
+        engine: 'druidsql',
+        version: '0.20.0',
+        source: 'dia.monds',
+        timeAttribute: 'time',
+        attributes,
+        allowSelectQueries: true,
+        filter: $('pugs').mvContains(['pugA', 'pugB', null, 'null']).not(),
+      }),
+    });
+    expect(queryPlan.length).to.equal(1);
+    expect(queryPlan).to.deep.equal([
+      [
+        {
+          context: {
+            sqlTimeZone: 'Etc/UTC',
+          },
+          query:
+            'SELECT\n"tags" AS "Tag"\nFROM "dia.monds" AS t\nWHERE ((("pugs" IS NULL AND MV_CONTAINS("pugs", ARRAY[\'pugA\',\'pugB\',\'null\']))) IS NOT TRUE AND ("tags" IS NULL AND MV_CONTAINS("tags", ARRAY[\'tagA\',\'tagB\',\'null\'])))\nGROUP BY 1',
         },
       ],
     ]);
@@ -256,7 +316,7 @@ describe('simulate DruidSql', () => {
             sqlTimeZone: 'Etc/UTC',
           },
           query:
-            'SELECT\n"tags" AS "Tag"\nFROM "dia.monds" AS t\nWHERE (((("pugs" IS NULL) OR "pugs" IN (\'pugA\',\'pugB\',\'\'))) IS NOT TRUE AND (("tags" IS NULL) OR "tags" IN (\'tagA\',\'tagB\',\'null\',\'\')))\nGROUP BY 1',
+            'SELECT\n"tags" AS "Tag"\nFROM "dia.monds" AS t\nWHERE ((("pugs" IS NULL OR "pugs" IN (\'pugA\',\'pugB\',\'\'))) IS NOT TRUE AND ("tags" IS NULL OR "tags" IN (\'tagA\',\'tagB\',\'null\',\'\')))\nGROUP BY 1',
         },
       ],
     ]);