diff --git a/src/expressions/baseExpression.ts b/src/expressions/baseExpression.ts index 330e4da7..bc97c42d 100644 --- 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 index 0cf884d8..e5351b90 100644 --- 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 index a960854b..6ca710d8 100644 --- 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 index 886a893b..9a78860f 100644 --- 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 index c806d608..606345c1 100644 --- 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( + 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 index 0700045c..21fc9773 100644 --- 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', }, ], ]);