From 17d356b9ef6041c4410bb90dd885a0d530016efe Mon Sep 17 00:00:00 2001 From: James Estes Date: Wed, 14 Oct 2026 19:16:22 -0600 Subject: [PATCH 01/15] fix(compiler): a filter's constant boolean is spelled by the dialect The `none` filter, the empty filter and a contradictory string filter compile to a constant, and so does `in` over an empty or null given. The constants were the keywords true and false written as strings, which a dialect without boolean values cannot run; the literals true and false already go through sqlBoolean, and now these do too. Co-Authored-By: Claude Fable 5.1 Signed-off-by: James Estes --- .../malloy/src/model/expression_compiler.ts | 4 ++-- packages/malloy/src/model/filter_compilers.ts | 20 +++++++++---------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/packages/malloy/src/model/expression_compiler.ts b/packages/malloy/src/model/expression_compiler.ts index a4afc760b..c81ed6f2e 100644 --- a/packages/malloy/src/model/expression_compiler.ts +++ b/packages/malloy/src/model/expression_compiler.ts @@ -242,7 +242,7 @@ function compileExpr( // null binding collapses to empty-set semantics — not the SQL // `IN (NULL)` shape, which has confusing NULL-membership rules. if (bound.node === 'null') { - return expr.not ? 'TRUE' : 'FALSE'; + return context.dialect.sqlBoolean(expr.not); } if (bound.node !== 'arrayLiteral') { throw new Error( @@ -250,7 +250,7 @@ function compileExpr( ); } if (bound.kids.values.length === 0) { - return expr.not ? 'TRUE' : 'FALSE'; + return context.dialect.sqlBoolean(expr.not); } const elemSqls = bound.kids.values.map(v => exprToSQL(resultSet, context, v, state) diff --git a/packages/malloy/src/model/filter_compilers.ts b/packages/malloy/src/model/filter_compilers.ts index 41a57d488..beb030148 100644 --- a/packages/malloy/src/model/filter_compilers.ts +++ b/packages/malloy/src/model/filter_compilers.ts @@ -64,8 +64,8 @@ function unlike(disLiked: string[], x: string) { // The `none` filter ("always false") compiles to a constant, independent of // column type or value; `not none` is "always true". Shared by the top-level // guard and every per-type compiler so the constant lives in one place. -function noneToSQL(not: boolean | undefined): string { - return not ? 'true' : 'false'; +function noneToSQL(not: boolean | undefined, d: Dialect): string { + return d.sqlBoolean(not === true); } export const FilterCompilers = { @@ -77,7 +77,7 @@ export const FilterCompilers = { qi: QueryInfo = {} ) { if (c === null) { - return 'true'; + return d.sqlBoolean(true); } // `none` is the always-false filter (companion to the empty/`null` filter, // which is always-true above). It compiles to a constant independent of @@ -85,7 +85,7 @@ export const FilterCompilers = { // also the primary path for boolean, whose compiler has no switch and // would otherwise route `none` through the null/false truth-table. if (c.operator === 'none') { - return noneToSQL(c.not); + return noneToSQL(c.not, d); } if (t === 'string' && isStringFilter(c)) { return FilterCompilers.stringCompile(c, x, d); @@ -135,7 +135,7 @@ export const FilterCompilers = { case 'null': return nc.not ? `${x} IS NOT NULL` : `${x} IS NULL`; case 'none': - return noneToSQL(nc.not); + return noneToSQL(nc.not, d); case '()': { const wrapped = '(' + FilterCompilers.numberCompile(nc.expr, x, d) + ')'; @@ -170,7 +170,7 @@ export const FilterCompilers = { // Normally intercepted by compile()'s top-level guard; handled here too so // booleanCompile is self-safe (this if-chain has no default, and `none` // must not fall through to the null/false truth-table below). - return noneToSQL(bc.not); + return noneToSQL(bc.not, d); } else if (bc.operator === '=true') { return bc.not ? `NOT ${px}` : x; } else if (bc.operator === '=false') { @@ -206,7 +206,7 @@ export const FilterCompilers = { case 'null': return sc.not ? `${x} IS NOT NULL` : `${x} IS NULL`; case 'none': - return noneToSQL(sc.not); + return noneToSQL(sc.not, d); case 'empty': return sc.not ? `COALESCE(${x},'') != ''` : `COALESCE(${x},'') = ''`; case '=': { @@ -317,7 +317,7 @@ export const FilterCompilers = { } } if ((includeEmpty && excludeEmpty) || (includeNull && excludeNull)) { - return 'false'; + return d.sqlBoolean(false); } let includeSQL = ''; if (includes.length > 0 || includeNull || includeEmpty) { @@ -352,7 +352,7 @@ export const FilterCompilers = { ? `(${includeSQL}) AND (${excludeSQL})` : includeSQL; } - return excludeSQL !== '' ? excludeSQL : 'true'; + return excludeSQL !== '' ? excludeSQL : d.sqlBoolean(true); } } }, @@ -522,7 +522,7 @@ export class TemporalFilterCompiler { case 'null': return tc.not ? `${x} IS NOT NULL` : `${x} IS NULL`; case 'none': - return noneToSQL(tc.not); + return noneToSQL(tc.not, this.d); case '()': { const wrapped = '(' + this.compile(tc.expr) + ')'; return tc.not ? `NOT ${wrapped}` : wrapped; From cf4c08fd1403594d3e50f0986e10664012a822f9 Mon Sep 17 00:00:00 2001 From: James Estes Date: Wed, 14 Oct 2026 22:06:58 -0600 Subject: [PATCH 02/15] fix(compiler): a boolean is written as the condition or the value SQL wants SQL has two places for a boolean, a condition (WHERE, ON, CASE WHEN, the operands of AND, OR and NOT) and a value, and on a dialect with no boolean type they do not mix: a comparison is legal only as a condition, a bit only as a value. The compiler carries the wanted form down the expression tree and crosses the boundary through the dialect, spells a literal in the wanted form, and writes Malloy's NULL rules for not, != and !~ through the dialect too. The translator no longer wraps a declared boolean in CASE. On every dialect with a boolean type the generated SQL is unchanged. Co-Authored-By: Claude Fable 5.1 Signed-off-by: James Estes --- packages/malloy-db-sqlserver/README.md | 5 +- packages/malloy/src/dialect/dialect.spec.ts | 41 +++++- packages/malloy/src/dialect/dialect.ts | 59 ++++++++- packages/malloy/src/dialect/index.ts | 1 + .../dialect/sqlserver/function_overrides.ts | 16 +-- .../src/lang/ast/expressions/expr-func.ts | 6 +- .../lang/ast/query-items/field-declaration.ts | 18 +-- packages/malloy/src/model/CONTEXT.md | 14 ++ .../malloy/src/model/expression_compiler.ts | 125 +++++++++++++++--- packages/malloy/src/model/field_instance.ts | 12 +- packages/malloy/src/model/filter_compilers.ts | 24 ++-- packages/malloy/src/model/malloy_types.ts | 3 + packages/malloy/src/model/query_query.ts | 8 +- .../src/databases/sqlserver/sqlserver.spec.ts | 80 ++++++++++- 14 files changed, 347 insertions(+), 65 deletions(-) diff --git a/packages/malloy-db-sqlserver/README.md b/packages/malloy-db-sqlserver/README.md index 10381fa93..acfa19f15 100644 --- a/packages/malloy-db-sqlserver/README.md +++ b/packages/malloy-db-sqlserver/README.md @@ -49,7 +49,7 @@ Cancelling a query uses the driver, so no `VIEW SERVER STATE` or `ALTER ANY CONN ## What works -Sources from tables (`table('schema.table')`, `[bracketed]` names, three-part `db.schema.table`) and SQL blocks; `select`, `group_by`, `aggregate`, `where`, `having`, `order_by`, `limit`; expressions as dimensions; joins with symmetric aggregates across a fan-out; multi-stage pipelines and a query used as a source; dates, timestamps, truncation, extraction, intervals, and `timezone:` with IANA names through `AT TIME ZONE`; the standard function library in its T-SQL spellings. +Sources from tables (`table('schema.table')`, `[bracketed]` names, three-part `db.schema.table`) and SQL blocks; `select`, `group_by`, `aggregate`, `where`, `having`, `order_by`, `limit`; expressions as dimensions; joins with symmetric aggregates across a fan-out; multi-stage pipelines and a query used as a source; dates, timestamps, truncation, extraction, intervals, and `timezone:` with IANA names through `AT TIME ZONE`; the standard function library in its T-SQL spellings; booleans in both positions T-SQL has for them, a comparison, a declared boolean, `not`, `!=`, `pick` or a boolean function written as a condition in `where:`, `having:` and `pick … when`, and as a `1`/`0` value in `select:`, `group_by:` and a function argument. ## What does not work yet @@ -59,9 +59,7 @@ Each of these is a later change, not a promise. |---|---| | `nest:` | Refused at translation (`supportsNesting` is false) | | Arrays and records, in table data or as literals | Declared unsupported; a JSON column is a string | -| A boolean where SQL expects a condition: a boolean dimension in `where:`, `having:` or `pick … when`; a `bit` column in `where:` | Server error (4145). A comparison, `starts_with`, `ends_with`, `is_nan` or `is_inf` written there directly works, and a boolean **declared** as a dimension or measure is a `1`/`0` value (the translator's CASE for `booleanType: 'none'`) that groups and selects but is not a condition | | `x::boolean` | Compile error: there is no boolean to cast to | -| `not x` on any boolean, `x !~ 'pattern'` | Server error (4145): the compiler writes `COALESCE(NOT x, TRUE)` and `COALESCE(x LIKE p, true)`, and a condition is not a value `COALESCE` can take. `x != y` works; `not (x = y)` does not | | `all()`, `exclude()` | Compile error: ungrouped aggregates share the nest machinery | | `x ~ r'...'` | Error when SQL is generated: no regular expressions before SQL Server 2025 | | `regexp_extract`, `replace` with a regular expression | Server error: the functions arrive in SQL Server 2025 | @@ -81,6 +79,7 @@ Every Malloy query is one T-SQL batch: the session settings `SET DATEFIRST 7`, ` - A `datetimeoffset` is a Malloy `timestamptz`, an instant. Its clock is read at UTC, or in the query's `timezone:`, not at the offset the value was written in, so `t.day` is the same for `12:34:56 -05:00` as for `17:34:56 +00:00`. - Every string literal is written `N'...'`, so text outside the database's code page compares as itself. - The default collation compares case-insensitively; Malloy does not change that. +- A boolean is a `1`/`0` value in a result, in a `group_by:` and as a function argument, and a condition in `where:`, `having:` and `pick … when`; the compiler writes whichever T-SQL wants where the expression is, and a NULL boolean stays NULL. - `bit` is an integer, read as 0 or 1. `uniqueidentifier`, `text`, `ntext`, `time`, `binary`, `varbinary`, `xml`, `geography` and the other types the map does not name are `sql native`, to be passed through or cast in a `sql()` dimension. - A `bigint` arrives as the driver's text and is read as a number, so a value above 2^53 loses precision. - The connection's digest, which names the tables it may persist and the cache it may share, is built from the server, port, instance, database, authentication kind and the configured principal: the `user`, `domain\user`, or `clientId`. An ambient identity, `azure-default` or `azure-msi` with no `clientId`, is unknown until the first token request, so two such connections to one database share a digest. To tell them apart, set `clientId` (a user-assigned identity) or use `azure-service-principal`. diff --git a/packages/malloy/src/dialect/dialect.spec.ts b/packages/malloy/src/dialect/dialect.spec.ts index 077e22769..abe8ffa9c 100644 --- a/packages/malloy/src/dialect/dialect.spec.ts +++ b/packages/malloy/src/dialect/dialect.spec.ts @@ -5,13 +5,52 @@ import {MySQLDialect} from './mysql'; import {SQLServerDialect} from './sqlserver'; +import {StandardSQLDialect} from './standardsql'; describe('Dialect booleans', () => { + const noBooleans = new SQLServerDialect(); + const booleans = new StandardSQLDialect(); + test('a dialect with no boolean type spells both literals as conditions', () => { - const noBooleans = new SQLServerDialect(); expect(noBooleans.sqlBoolean(true)).toBe('(1=1)'); expect(noBooleans.sqlBoolean(false)).toBe('(1=0)'); }); + + test('a dialect with no boolean type spells a literal value as 1 or 0', () => { + expect(noBooleans.sqlBooleanValue(true)).toBe('1'); + expect(noBooleans.sqlBooleanValue(false)).toBe('0'); + }); + + test('a dialect with booleans spells a literal the same in either form', () => { + expect(booleans.sqlBoolean(true)).toBe('true'); + expect(booleans.sqlBooleanValue(true)).toBe('true'); + expect(booleans.sqlBoolean(false)).toBe('false'); + expect(booleans.sqlBooleanValue(false)).toBe('false'); + }); + + test('a condition becomes a value which is NULL when the condition is', () => { + expect(noBooleans.sqlConditionAsValue('a > b')).toBe( + 'CASE WHEN a > b THEN 1 WHEN NOT (a > b) THEN 0 END' + ); + expect(booleans.sqlConditionAsValue('a > b')).toBe('a > b'); + }); + + test('a value becomes a condition by comparing it with 1', () => { + expect(noBooleans.sqlValueAsCondition('x')).toBe('(x = 1)'); + expect(booleans.sqlValueAsCondition('x')).toBe('x'); + }); + + test('not, and a comparison true on NULL, keep COALESCE where there are booleans', () => { + expect(booleans.sqlNot('c')).toBe('COALESCE(NOT c,TRUE)'); + expect(booleans.sqlNullIsTrue('c')).toBe('COALESCE(c,true)'); + }); + + test('not, and a comparison true on NULL, are written without a boolean value', () => { + expect(noBooleans.sqlNot('c')).toBe('(CASE WHEN c THEN 1 ELSE 0 END = 0)'); + expect(noBooleans.sqlNullIsTrue('c')).toBe( + '(CASE WHEN NOT (c) THEN 1 ELSE 0 END = 0)' + ); + }); }); describe('Dialect ORDER BY and row limit', () => { diff --git a/packages/malloy/src/dialect/dialect.ts b/packages/malloy/src/dialect/dialect.ts index bee65d1ce..c049516ba 100644 --- a/packages/malloy/src/dialect/dialect.ts +++ b/packages/malloy/src/dialect/dialect.ts @@ -180,6 +180,12 @@ export type OrderByClauseType = 'output_name' | 'ordinal' | 'expression'; export type GroupByClauseType = 'ordinal' | 'expression'; export type OrderByRequest = 'query' | 'turtle' | 'analytical'; export type BooleanTypeSupport = 'supported' | 'simulated' | 'none'; +/** + * Where SQL expects a boolean expression to be: a condition (WHERE, ON, + * CASE WHEN, the operands of AND, OR and NOT) or a value (everywhere else). + * A dialect with booleanType 'none' converts between the two. + */ +export type BooleanForm = 'condition' | 'value'; /** * Maps a range of integer values to a Malloy number type. @@ -1232,7 +1238,7 @@ export abstract class Dialect { } /** - * SQL to generate to get a boolean value for a boolean expression + * A boolean literal where a condition is expected */ sqlBoolean(bv: boolean): string { if (this.booleanType === 'none') { @@ -1241,6 +1247,57 @@ export abstract class Dialect { return bv ? 'true' : 'false'; } + /** + * A boolean literal where a value is expected + */ + sqlBooleanValue(bv: boolean): string { + if (this.booleanType === 'none') { + return bv ? '1' : '0'; + } + return bv ? 'true' : 'false'; + } + + /** + * On a dialect with booleanType 'none' a condition (a comparison, AND, + * IS NULL) is legal only where SQL expects a condition, and a value (a + * bit) only where SQL expects a value. The compiler crosses that boundary + * through these two; a NULL condition is a NULL value. On every other + * dialect each returns its argument. + */ + sqlConditionAsValue(condition: string): string { + if (this.booleanType === 'none') { + return `CASE WHEN ${condition} THEN 1 WHEN NOT (${condition}) THEN 0 END`; + } + return condition; + } + + sqlValueAsCondition(value: string): string { + if (this.booleanType === 'none') { + return `(${value} = 1)`; + } + return value; + } + + /** + * Malloy's `not`: of NULL, true + */ + sqlNot(condition: string): string { + if (this.booleanType === 'none') { + return `(CASE WHEN ${condition} THEN 1 ELSE 0 END = 0)`; + } + return `COALESCE(NOT ${condition},TRUE)`; + } + + /** + * A condition Malloy defines as true when SQL says NULL: `!=` and `!~` + */ + sqlNullIsTrue(condition: string): string { + if (this.booleanType === 'none') { + return `(CASE WHEN NOT (${condition}) THEN 1 ELSE 0 END = 0)`; + } + return `COALESCE(${condition},true)`; + } + /** * What a boolean value looks like in a query result */ diff --git a/packages/malloy/src/dialect/index.ts b/packages/malloy/src/dialect/index.ts index 52e391e1b..121c3ac0b 100644 --- a/packages/malloy/src/dialect/index.ts +++ b/packages/malloy/src/dialect/index.ts @@ -25,6 +25,7 @@ export { } from './functions/util'; export {Dialect, queryZone} from './dialect'; export type { + BooleanForm, DialectFieldList, CompiledOrderBy, LateralJoinExpression, diff --git a/packages/malloy/src/dialect/sqlserver/function_overrides.ts b/packages/malloy/src/dialect/sqlserver/function_overrides.ts index 6a07b069f..684e79c41 100644 --- a/packages/malloy/src/dialect/sqlserver/function_overrides.ts +++ b/packages/malloy/src/dialect/sqlserver/function_overrides.ts @@ -12,17 +12,15 @@ export const SQLSERVER_MALLOY_STANDARD_OVERLOADS: OverrideMap = { ceil: {function: 'CEILING'}, chr: {sql: "CASE WHEN ${value} = 0 THEN '' ELSE NCHAR(${value}) END"}, div: {sql: 'FLOOR(${dividend} / ${divisor})'}, - // starts_with and ends_with are written as comparisons because SQL Server - // has no boolean values: legal as a condition (where:, having:), a server - // error as a value. A NULL input matches nothing, where the standard's - // COALESCE(..., false) returns false. + // A boolean function is a 1/0 value; the compiler makes a condition of it + // where one is wanted. Of NULL, 0: the standard is COALESCE(..., false). ends_with: { - sql: `RIGHT(\${value}, ${len('${suffix}')}) = \${suffix}`, + sql: `CASE WHEN RIGHT(\${value}, ${len('${suffix}')}) = \${suffix} THEN 1 ELSE 0 END`, }, ifnull: {function: 'ISNULL'}, - // A FLOAT holds neither infinity nor NaN, so neither condition is ever true - is_inf: {sql: '(1=0)'}, - is_nan: {sql: '(1=0)'}, + // A FLOAT holds neither infinity nor NaN + is_inf: {sql: '0'}, + is_nan: {sql: '0'}, length: {sql: len('${value}')}, ln: {sql: 'LOG(${value})'}, log: {sql: 'LOG(${value}, ${base})'}, @@ -35,7 +33,7 @@ export const SQLSERVER_MALLOY_STANDARD_OVERLOADS: OverrideMap = { atan2: {function: 'ATN2'}, trim: {characters: {sql: 'TRIM(${trim_characters} FROM ${value})'}}, starts_with: { - sql: `LEFT(\${value}, ${len('${prefix}')}) = \${prefix}`, + sql: `CASE WHEN LEFT(\${value}, ${len('${prefix}')}) = \${prefix} THEN 1 ELSE 0 END`, }, stddev: {function: 'STDEV'}, string_repeat: {function: 'REPLICATE'}, diff --git a/packages/malloy/src/lang/ast/expressions/expr-func.ts b/packages/malloy/src/lang/ast/expressions/expr-func.ts index 6d984ab89..8ddb57366 100644 --- a/packages/malloy/src/lang/ast/expressions/expr-func.ts +++ b/packages/malloy/src/lang/ast/expressions/expr-func.ts @@ -450,7 +450,11 @@ export class ExprFunc extends ExpressionDef { } } - funcCall = composeSQLExpr(expr); + const raw = composeSQLExpr(expr); + if (func.name === 'sql_boolean') { + raw.condition = true; + } + funcCall = raw; } } const maxEvalSpace = mergeEvalSpaces(...argExprs.map(e => e.evalSpace)); diff --git a/packages/malloy/src/lang/ast/query-items/field-declaration.ts b/packages/malloy/src/lang/ast/query-items/field-declaration.ts index f58816926..47e531469 100644 --- a/packages/malloy/src/lang/ast/query-items/field-declaration.ts +++ b/packages/malloy/src/lang/ast/query-items/field-declaration.ts @@ -126,23 +126,7 @@ export abstract class AtomicFieldDeclaration ret.timeframe = exprValue.timeframe; } ret.location = this.location; - if ( - exprValue.type === 'boolean' && - exprFS.dialectObj()?.booleanType === 'none' - ) { - // when generating a boolean field on a database without boolean support - // map it to integers - ret.e = { - node: 'case', - kids: { - caseWhen: [exprValue.value], - caseThen: [{node: 'numberLiteral', literal: '1'}], - caseElse: {node: 'numberLiteral', literal: '0'}, - }, - }; - } else { - ret.e = exprValue.value; - } + ret.e = exprValue.value; ret.drillExpression = this.expr.drillExpression(); ret.refSummary = exprValue.refSummary; ret.ungroupings = exprValue.ungroupings; diff --git a/packages/malloy/src/model/CONTEXT.md b/packages/malloy/src/model/CONTEXT.md index 12e404249..0e6848b88 100644 --- a/packages/malloy/src/model/CONTEXT.md +++ b/packages/malloy/src/model/CONTEXT.md @@ -447,6 +447,20 @@ Compiles IR expression trees into SQL expressions. - Function calls - Field references and path expressions +**Conditions and values.** SQL has two places for a boolean: a *condition* (`WHERE`, `ON`, +`CASE WHEN`, the operands of `AND`/`OR`/`NOT`) and a *value* (everywhere else). On a dialect +with a boolean type the two are interchangeable; on one with `booleanType: 'none'` (SQL Server) +a comparison is legal only as a condition and a `bit` only as a value. `compileExpr` carries the +`wanted` form down the tree (`wantedForm`: the operands of `and`/`or`/`not`, a filter and a +subject-less `CASE WHEN` want a condition, everything else a value), knows each node's natural +form (`booleanForm`: comparisons, `in`, `like`, `is null`, a filter match and `sql_boolean` are +conditions), and crosses the boundary through `Dialect.sqlConditionAsValue` / +`sqlValueAsCondition`, which return their argument on every dialect with booleans. The literals +`true`/`false` are spelled in the wanted form (`sqlBoolean` / `sqlBooleanValue`), and Malloy's +NULL rules for `not`, `!=` and `!~` go through `Dialect.sqlNot` / `sqlNullIsTrue`. A +`where:`/`having:` list and a join `on:` are compiled wanting a condition; a field is compiled +wanting a value. + ### 3. Dialect System The compiler supports multiple SQL dialects through a plugin architecture. diff --git a/packages/malloy/src/model/expression_compiler.ts b/packages/malloy/src/model/expression_compiler.ts index c81ed6f2e..b079b03d1 100644 --- a/packages/malloy/src/model/expression_compiler.ts +++ b/packages/malloy/src/model/expression_compiler.ts @@ -60,7 +60,7 @@ import { } from './utils'; import {isBasicScalar} from './query_node'; import type {QueryStruct, QueryField} from './query_node'; -import type {Dialect, QueryInfo} from '../dialect'; +import type {BooleanForm, Dialect, QueryInfo} from '../dialect'; import {MalloyCompileError} from './malloy_compile_error'; import {lookupGivenValue} from './given_binding'; @@ -112,6 +112,65 @@ function deepCopyExpr(expr: T): T { return {...expr}; } +// The form a node's SQL takes when it is a boolean the compiler can tell +// apart: a condition or a value. Undefined for a node whose SQL is its +// child's ('()', 'filterCondition'), for raw SQL, and for the literals true +// and false, which are spelled in whichever form is wanted. +function booleanForm(expr: Expr): BooleanForm | undefined { + switch (expr.node) { + case '>': + case '<': + case '>=': + case '<=': + case '=': + case '!=': + case 'and': + case 'or': + case 'not': + case 'in': + case 'inGiven': + case 'like': + case '!like': + case 'is-null': + case 'is-not-null': + case 'regexpMatch': + case 'filterMatch': + return 'condition'; + case 'genericSQLExpr': + return expr.condition ? 'condition' : undefined; + case '()': + case 'filterCondition': + case 'true': + case 'false': + return undefined; + default: + return 'value'; + } +} + +// The form a node wants of one of its children +function wantedForm( + parent: Expr, + kid: string, + wanted: BooleanForm +): BooleanForm { + switch (parent.node) { + case 'and': + case 'or': + case 'not': + case 'filterCondition': + return 'condition'; + case '()': + return wanted; + case 'case': + return kid === 'caseWhen' && parent.kids.caseValue === undefined + ? 'condition' + : 'value'; + default: + return 'value'; + } +} + /** * Compiles an expression tree by mutating it in-place to set all .sql fields. * Assumes the expression tree is already a copy that can be mutated. @@ -121,7 +180,8 @@ function compileExpr( context: QueryStruct, expr: T, state: GenerateState = new GenerateState(), - wrap = true + wrap = true, + wanted: BooleanForm = 'value' ): T { /* * Translate the children first, and stash the translation @@ -129,16 +189,24 @@ function compileExpr( * it will have access to the translated children. */ if (exprHasE(expr)) { - compileExpr(resultSet, context, expr.e, state); + compileExpr( + resultSet, + context, + expr.e, + state, + true, + wantedForm(expr, 'e', wanted) + ); } else if (exprHasKids(expr)) { - for (const kidExpr of Object.values(expr.kids)) { + for (const [kidName, kidExpr] of Object.entries(expr.kids)) { if (kidExpr === null) continue; + const kidWanted = wantedForm(expr, kidName, wanted); if (Array.isArray(kidExpr)) { for (const e of kidExpr) { - compileExpr(resultSet, context, e, state); + compileExpr(resultSet, context, e, state, true, kidWanted); } } else { - compileExpr(resultSet, context, kidExpr, state); + compileExpr(resultSet, context, kidExpr, state, true, kidWanted); } } } @@ -148,12 +216,11 @@ function compileExpr( */ const qi = resultSet.getQueryInfo(); const dialectSQL = context.dialect.exprToSQL(qi, expr); - if (dialectSQL) { - expr.sql = wrap && exprHasKids(expr) ? `(${dialectSQL})` : dialectSQL; - return expr; - } const sql = (() => { + if (dialectSQL) { + return dialectSQL; + } switch (expr.node) { case 'field': return generateFieldFragment(resultSet, context, expr, state); @@ -226,7 +293,7 @@ function compileExpr( // Malloy inequality comparisons always return a boolean case '!=': { const notEqual = `${expr.kids.left.sql}!=${expr.kids.right.sql}`; - return `COALESCE(${notEqual},true)`; + return context.dialect.sqlNullIsTrue(notEqual); } case 'and': case 'or': @@ -269,13 +336,14 @@ function compileExpr( expr.kids.right.literal ) : `${expr.kids.left.sql} ${likeIt} ${expr.kids.right.sql}`; - return expr.node === 'like' ? compare : `COALESCE(${compare},true)`; + return expr.node === 'like' + ? compare + : context.dialect.sqlNullIsTrue(compare); } case '()': return `(${expr.e.sql})`; case 'not': - // Malloy not operator always returns a boolean - return `COALESCE(NOT ${expr.e.sql},TRUE)`; + return context.dialect.sqlNot(expr.e.sql ?? ''); case 'unary-': return `-${expr.e.sql}`; case 'is-null': @@ -284,7 +352,9 @@ function compileExpr( return `${expr.e.sql} IS NOT NULL`; case 'true': case 'false': - return context.dialect.sqlBoolean(expr.node === 'true'); + return wanted === 'condition' + ? context.dialect.sqlBoolean(expr.node === 'true') + : context.dialect.sqlBooleanValue(expr.node === 'true'); case 'null': return 'NULL'; case 'case': @@ -318,7 +388,14 @@ function compileExpr( } })(); - expr.sql = wrap && exprHasKids(expr) ? `(${sql})` : sql; + const form = booleanForm(expr); + const formed = + form === 'condition' && wanted === 'value' + ? context.dialect.sqlConditionAsValue(sql) + : form === 'value' && wanted === 'condition' + ? context.dialect.sqlValueAsCondition(sql) + : sql; + expr.sql = wrap && exprHasKids(expr) ? `(${formed})` : formed; return expr; } @@ -330,13 +407,21 @@ export function exprToSQL( resultSet: FieldInstanceResult, context: QueryStruct, exprToTranslate: Expr, - state: GenerateState = new GenerateState() + state: GenerateState = new GenerateState(), + wanted: BooleanForm = 'value' ): string { // Make a deep copy that we can mutate during compilation const exprCopy = deepCopyExpr(exprToTranslate); // Compile the copy, setting .sql on all nodes - const compiled = compileExpr(resultSet, context, exprCopy, state, false); + const compiled = compileExpr( + resultSet, + context, + exprCopy, + state, + false, + wanted + ); return compiled.sql!; } @@ -916,7 +1001,9 @@ export function generateFilterFragment( ): string { const allWhere = new AndChain(state.whereSQL); for (const cond of expr.kids.filterList) { - allWhere.add(exprToSQL(resultSet, context, cond.e, state.withWhere())); + allWhere.add( + exprToSQL(resultSet, context, cond.e, state.withWhere(), 'condition') + ); } return exprToSQL( resultSet, diff --git a/packages/malloy/src/model/field_instance.ts b/packages/malloy/src/model/field_instance.ts index a44ae9256..3a6a912f2 100644 --- a/packages/malloy/src/model/field_instance.ts +++ b/packages/malloy/src/model/field_instance.ts @@ -3,7 +3,7 @@ * SPDX-License-Identifier: MIT */ -import type {FieldReferenceType, QueryInfo} from '../dialect'; +import type {BooleanForm, FieldReferenceType, QueryInfo} from '../dialect'; import type {QueryStruct} from './query_node'; import type { Expr, @@ -64,7 +64,8 @@ export class FieldInstanceField implements FieldInstance { resultSet: FieldInstanceResult, context: QueryStruct, expr: Expr, - state?: GenerateState + state?: GenerateState, + wanted?: BooleanForm ) => string; static registerExpressionCompiler( @@ -72,7 +73,8 @@ export class FieldInstanceField implements FieldInstance { resultSet: FieldInstanceResult, context: QueryStruct, expr: Expr, - state?: GenerateState + state?: GenerateState, + wanted?: BooleanForm ) => string ) { FieldInstanceField.exprCompiler = compiler; @@ -110,7 +112,9 @@ export class FieldInstanceField implements FieldInstance { return FieldInstanceField.exprCompiler( this.parent, this.f.parent, - this.f.fieldDef.e + this.f.fieldDef.e, + undefined, + this.fieldUsage.type === 'where' ? 'condition' : 'value' ); } diff --git a/packages/malloy/src/model/filter_compilers.ts b/packages/malloy/src/model/filter_compilers.ts index beb030148..22025cbfe 100644 --- a/packages/malloy/src/model/filter_compilers.ts +++ b/packages/malloy/src/model/filter_compilers.ts @@ -172,9 +172,13 @@ export const FilterCompilers = { // must not fall through to the null/false truth-table below). return noneToSQL(bc.not, d); } else if (bc.operator === '=true') { - return bc.not ? `NOT ${px}` : x; + return bc.not + ? `NOT ${d.sqlValueAsCondition(px)}` + : d.sqlValueAsCondition(x); } else if (bc.operator === '=false') { - return bc.not ? x : `NOT ${px}`; + return bc.not + ? d.sqlValueAsCondition(x) + : `NOT ${d.sqlValueAsCondition(px)}`; } else if (bc.operator === 'null') { return bc.not ? `${px} IS NOT NULL` : `${px} IS NULL`; } @@ -185,21 +189,25 @@ export const FilterCompilers = { const quoteChar = d.sqlQuoteIdentifier('select')[0]; const isColumn = x.match(`^[()${quoteChar}\\w.]+$`); + const isFalse = `${px} = ${d.sqlBooleanValue(false)}`; if (isColumn) { if (bc.operator === 'true') { return bc.not - ? `${px} IS NULL OR ${px} = false` - : `${px} IS NOT NULL AND ${px}`; + ? `${px} IS NULL OR ${isFalse}` + : `${px} IS NOT NULL AND ${d.sqlValueAsCondition(px)}`; } return bc.not - ? `${px} IS NOT NULL AND ${px}` // not false: exclude null - : `${px} IS NULL OR ${px} = false`; // false: include null + ? `${px} IS NOT NULL AND ${d.sqlValueAsCondition(px)}` // not false: exclude null + : `${px} IS NULL OR ${isFalse}`; // false: include null } + const notNull = d.sqlValueAsCondition( + `COALESCE(${x}, ${d.sqlBooleanValue(false)})` + ); if (bc.operator === 'true') { - return bc.not ? `NOT COALESCE(${x}, false)` : `COALESCE(${x}, false)`; + return bc.not ? `NOT ${notNull}` : notNull; } // else bc.operator === 'false' - return bc.not ? `COALESCE(${x}, false)` : `NOT COALESCE(${x}, false)`; + return bc.not ? notNull : `NOT ${notNull}`; }, stringCompile(sc: StringFilter, x: string, d: Dialect): string { switch (sc.operator) { diff --git a/packages/malloy/src/model/malloy_types.ts b/packages/malloy/src/model/malloy_types.ts index 45ffe45f0..c92b5fa23 100644 --- a/packages/malloy/src/model/malloy_types.ts +++ b/packages/malloy/src/model/malloy_types.ts @@ -429,6 +429,9 @@ export interface GenericSQLExpr extends ExprWithKids { node: 'genericSQLExpr'; kids: {args: Expr[]}; src: string[]; + // The SQL is a condition (sql_boolean), which a dialect without boolean + // values converts where a value is wanted + condition?: true; } export interface NullNode extends ExprLeaf { diff --git a/packages/malloy/src/model/query_query.ts b/packages/malloy/src/model/query_query.ts index a559e8039..1fec0cb49 100644 --- a/packages/malloy/src/model/query_query.ts +++ b/packages/malloy/src/model/query_query.ts @@ -590,7 +590,13 @@ export class QueryQuery extends QueryField { (which === 'having' && expressionIsCalculation(cond.expressionType)) || (which === 'where' && expressionIsScalar(cond.expressionType)) ) { - const sqlClause = exprToSQL(resultStruct, context, cond.e, undefined); + const sqlClause = exprToSQL( + resultStruct, + context, + cond.e, + undefined, + 'condition' + ); resultFilters.add(sqlClause); } } diff --git a/test/src/databases/sqlserver/sqlserver.spec.ts b/test/src/databases/sqlserver/sqlserver.spec.ts index 9a07107d6..de7e0f710 100644 --- a/test/src/databases/sqlserver/sqlserver.spec.ts +++ b/test/src/databases/sqlserver/sqlserver.spec.ts @@ -111,6 +111,84 @@ describe('SQL Server', () => { ]); }); + test('a declared boolean is a condition in where: and having:', async () => { + await expect(` + run: sqlserver.table('malloytest.state_facts') extend { + dimension: big is airport_count > 500 + measure: n is count() + } -> { + where: big + group_by: big + aggregate: n + having: n > 1 + } + `).toEqualResult(tm, [{big: true, n: 12}]); + }); + + test('not, and a comparison true on NULL, follow Malloy rather than SQL', async () => { + await expect(` + run: sqlserver.sql('SELECT CAST(NULL AS INT) AS n, 1 AS one') -> { + select: + not_null is not (n = 1) + ne_null is n != 1 + not_one is not (one = 1) + ne_one is one != 1 + } + `).toEqualResult(tm, [ + {not_null: true, ne_null: true, not_one: false, ne_one: false}, + ]); + }); + + test('a boolean picks, and is picked', async () => { + await expect(` + run: sqlserver.table('malloytest.state_facts') -> { + group_by: + size is pick 'big' when airport_count > 500 else 'small' + flagged is pick true when airport_count > 500 else false + aggregate: n is count() + order_by: size + } + `).toEqualResult(tm, [ + {size: 'big', flagged: true, n: 12}, + {size: 'small', flagged: false, n: 39}, + ]); + }); + + test('a boolean function is a value, and a condition where one is wanted', async () => { + await expect(` + run: sqlserver.table('malloytest.state_facts') -> { + where: starts_with(state, 'C') + select: state, c is starts_with(state, 'C'), a is ends_with(state, 'A') + order_by: state + } + `).toEqualResult(tm, [ + {state: 'CA', c: true, a: true}, + {state: 'CO', c: true, a: false}, + {state: 'CT', c: true, a: false}, + ]); + }); + + test('a boolean literal is a value in select:', async () => { + await expect(` + run: sqlserver.table('malloytest.state_facts') -> { + select: t is true, f is false + limit: 1 + } + `).toEqualResult(tm, [{t: true, f: false}]); + }); + + test('sql_boolean is a condition', async () => { + await expect(` + ##! experimental { sql_functions } + run: sqlserver.table('malloytest.state_facts') extend { + dimension: big is sql_boolean("\${airport_count} > 500") + } -> { + where: big + aggregate: n is count() + } + `).toEqualResult(tm, [{n: 12}]); + }); + test('a cast to boolean is refused', async () => { await expect( runtime @@ -123,7 +201,7 @@ describe('SQL Server', () => { ).rejects.toThrow(/boolean/); }); - test('starts_with and ends_with are conditions', async () => { + test('starts_with, ends_with and is_nan filter an aggregate', async () => { await expect(` run: sqlserver.table('malloytest.state_facts') -> { aggregate: From 4df76c074d485e2b3ee85d943d95088353574808 Mon Sep 17 00:00:00 2001 From: James Estes Date: Wed, 14 Oct 2026 22:06:58 -0600 Subject: [PATCH 03/15] test: a result matcher reads 1/0 as a boolean on any dialect without one resultBoolean returns 1/0 for every booleanType but 'supported'; the matcher converted a number back to a boolean only for 'simulated'. Co-Authored-By: Claude Fable 5.1 Signed-off-by: James Estes --- packages/malloy/src/test/resultMatchers.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/malloy/src/test/resultMatchers.ts b/packages/malloy/src/test/resultMatchers.ts index ade3afba4..8ad67035e 100644 --- a/packages/malloy/src/test/resultMatchers.ts +++ b/packages/malloy/src/test/resultMatchers.ts @@ -443,10 +443,10 @@ function matchCell( return matchFail(path, expected, actual); } let actual: unknown = cell.number_value; - // Handle simulated booleans (MySQL returns 1/0 for booleans) + // A dialect without a boolean type returns 1/0 for a boolean if ( typeof expected === 'boolean' && - dialect.booleanType === 'simulated' && + dialect.booleanType !== 'supported' && typeof actual === 'number' ) { actual = actual !== 0; From d1a646adc23013707667c90546abfe53c9a6f3f7 Mon Sep 17 00:00:00 2001 From: James Estes Date: Mon, 19 Oct 2026 17:21:23 -0600 Subject: [PATCH 04/15] The translator refuses a boolean where a dialect without boolean values needs a value On a dialect with booleanType 'none' a boolean is a condition and never a value. A boolean as a result column, a function argument, a null test, in ??, in a cast, compared with = or !=, matched with a filter expression or as the result of a pick is an error at that expression, naming the value to write instead; select: * leaves a boolean field out with a warning. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01T1vFteHTo61YyRRVVrJAn4 Signed-off-by: James Estes --- .../src/lang/ast/expressions/boolean-value.ts | 32 ++++ .../malloy/src/lang/ast/expressions/case.ts | 8 + .../ast/expressions/expr-alternation-tree.ts | 8 + .../src/lang/ast/expressions/expr-cast.ts | 15 ++ .../src/lang/ast/expressions/expr-coalesce.ts | 8 + .../src/lang/ast/expressions/expr-func.ts | 16 +- .../src/lang/ast/expressions/expr-null.ts | 8 + .../src/lang/ast/expressions/pick-when.ts | 15 ++ .../lang/ast/field-space/index-field-space.ts | 9 +- .../src/lang/ast/field-space/query-spaces.ts | 7 +- .../lang/ast/query-items/field-declaration.ts | 27 +++ .../lang/ast/query-items/field-references.ts | 30 ++++ .../src/lang/ast/types/expression-def.ts | 18 ++ packages/malloy/src/lang/parse-log.ts | 4 + .../malloy/src/lang/test/boolean-none.spec.ts | 155 ++++++++++++++++++ .../malloy/src/lang/test/test-translator.ts | 20 ++- 16 files changed, 370 insertions(+), 10 deletions(-) create mode 100644 packages/malloy/src/lang/ast/expressions/boolean-value.ts create mode 100644 packages/malloy/src/lang/test/boolean-none.spec.ts diff --git a/packages/malloy/src/lang/ast/expressions/boolean-value.ts b/packages/malloy/src/lang/ast/expressions/boolean-value.ts new file mode 100644 index 000000000..2e642a4b2 --- /dev/null +++ b/packages/malloy/src/lang/ast/expressions/boolean-value.ts @@ -0,0 +1,32 @@ +/* + * Copyright Contributors to the Malloy project + * SPDX-License-Identifier: MIT + */ + +import type {FieldSpace} from '../types/field-space'; +import type {MalloyElement} from '../types/malloy-element'; + +/** + * On a dialect with booleanType 'none' a boolean is a condition and never a + * value. Logs an error at `logTo` when `type` is a boolean about to be used + * as a value; `where` says how, `hint` what to write instead. Returns true + * when it logged. + */ +export function refuseBooleanValue( + fs: FieldSpace, + type: {type: string} | undefined, + where: string, + logTo: MalloyElement, + hint: string +): boolean { + const dialect = fs.dialectObj(); + if (type?.type !== 'boolean' || dialect?.booleanType !== 'none') { + return false; + } + logTo.logError('boolean-value-unsupported', { + dialect: dialect.name, + where, + hint, + }); + return true; +} diff --git a/packages/malloy/src/lang/ast/expressions/case.ts b/packages/malloy/src/lang/ast/expressions/case.ts index 864d88adf..5d5492a06 100644 --- a/packages/malloy/src/lang/ast/expressions/case.ts +++ b/packages/malloy/src/lang/ast/expressions/case.ts @@ -10,6 +10,7 @@ import type {FieldSpace} from '../types/field-space'; import {MalloyElement} from '../types/malloy-element'; import * as TDU from '../typedesc-utils'; import type {CaseExpr} from '../../../model'; +import {refuseBooleanValue} from './boolean-value'; interface Choice { then: ExprValue; @@ -94,6 +95,13 @@ export class Case extends ExpressionDef { dependents.push(elseValue); resultExpr.kids.caseElse = elseValue.value; } + refuseBooleanValue( + fs, + returnType, + 'as the result of a case', + this, + "write values, pick 'yes' when … else 'no', or the condition itself with and, or and not" + ); return computedExprValue({ value: resultExpr, dataType: returnType ? TDU.atomicDef(returnType) : {type: 'null'}, diff --git a/packages/malloy/src/lang/ast/expressions/expr-alternation-tree.ts b/packages/malloy/src/lang/ast/expressions/expr-alternation-tree.ts index ad5517e04..651446742 100644 --- a/packages/malloy/src/lang/ast/expressions/expr-alternation-tree.ts +++ b/packages/malloy/src/lang/ast/expressions/expr-alternation-tree.ts @@ -9,6 +9,7 @@ import type {FieldSpace} from '../types/field-space'; import {ATNodeType, ExpressionDef} from '../types/expression-def'; import type {BinaryMalloyOperator} from '../types/binary_operators'; import {isEquality} from '../types/binary_operators'; +import {refuseBooleanValue} from './boolean-value'; /** * Return a flattened version of an alternation tree, if the tree is @@ -70,6 +71,13 @@ export class ExprAlternationTree extends ExpressionDef { if (inList.length > 0 && (applyOp === '=' || applyOp === '!=')) { const isIn = expr.getExpression(fs); const values = inList.map(v => v.getExpression(fs)); + refuseBooleanValue( + fs, + isIn, + `compared with ${applyOp}`, + expr, + 'test the condition itself, or its not' + ); return computedExprValue({ dataType: {type: 'boolean'}, value: { diff --git a/packages/malloy/src/lang/ast/expressions/expr-cast.ts b/packages/malloy/src/lang/ast/expressions/expr-cast.ts index af70e3840..6d68c738e 100644 --- a/packages/malloy/src/lang/ast/expressions/expr-cast.ts +++ b/packages/malloy/src/lang/ast/expressions/expr-cast.ts @@ -9,6 +9,7 @@ import type {ExprValue} from '../types/expr-value'; import {computedExprValue} from '../types/expr-value'; import {ExpressionDef} from '../types/expression-def'; import type {FieldSpace} from '../types/field-space'; +import {refuseBooleanValue} from './boolean-value'; export class ExprCast extends ExpressionDef { elementType = 'cast'; @@ -46,6 +47,20 @@ export class ExprCast extends ExpressionDef { } } } + refuseBooleanValue( + fs, + expr, + 'in a cast', + this, + 'write the value, pick 1 when … else 0' + ); + refuseBooleanValue( + fs, + dataType, + 'as the type of a cast', + this, + 'write a comparison' + ); return computedExprValue({ dataType, value: castTo(this.castType, expr.value, expr.type, this.safe), diff --git a/packages/malloy/src/lang/ast/expressions/expr-coalesce.ts b/packages/malloy/src/lang/ast/expressions/expr-coalesce.ts index b0787c5ca..cbc5ee191 100644 --- a/packages/malloy/src/lang/ast/expressions/expr-coalesce.ts +++ b/packages/malloy/src/lang/ast/expressions/expr-coalesce.ts @@ -9,6 +9,7 @@ import * as TDU from '../typedesc-utils'; import type {ExprValue} from '../types/expr-value'; import {ExpressionDef} from '../types/expression-def'; import type {FieldSpace} from '../types/field-space'; +import {refuseBooleanValue} from './boolean-value'; export class ExprCoalesce extends ExpressionDef { elementType = 'coalesce expression'; @@ -23,6 +24,13 @@ export class ExprCoalesce extends ExpressionDef { protected computeExpression(fs: FieldSpace): ExprValue { const maybeNull = this.expr.getExpression(fs); const whenNull = this.altExpr.getExpression(fs); + refuseBooleanValue( + fs, + maybeNull, + 'in ??', + this, + 'a boolean here is a condition, which is false when unknown' + ); if (maybeNull.type === 'null') { return whenNull; } diff --git a/packages/malloy/src/lang/ast/expressions/expr-func.ts b/packages/malloy/src/lang/ast/expressions/expr-func.ts index 8ddb57366..871aa017c 100644 --- a/packages/malloy/src/lang/ast/expressions/expr-func.ts +++ b/packages/malloy/src/lang/ast/expressions/expr-func.ts @@ -60,6 +60,7 @@ import {composeSQLExpr} from '../../../model/utils'; import * as TDU from '../typedesc-utils'; import {mergeRefSummaries} from '../../composite-source-utils'; import type {AnyMessageCodeAndParameters} from '../../parse-log'; +import {refuseBooleanValue} from './boolean-value'; // Built-in functions that take a string literal of user-supplied SQL // and emit it directly. Gated by experimental.sql_functions in plain @@ -237,6 +238,15 @@ export class ExprFunc extends ExpressionDef { nullabilityErrors, returnType, } = result; + argExprs.forEach((argExpr, i) => { + refuseBooleanValue( + fs, + argExpr, + `as an argument of ${this.name}`, + this.args[i - (implicitExpr ? 1 : 0)] ?? this, + 'write the value, pick 1 when … else 0' + ); + }); // Report errors for expression type mismatch for (const error of expressionTypeErrors) { const adjustedIndex = error.argIndex - (implicitExpr ? 1 : 0); @@ -450,11 +460,7 @@ export class ExprFunc extends ExpressionDef { } } - const raw = composeSQLExpr(expr); - if (func.name === 'sql_boolean') { - raw.condition = true; - } - funcCall = raw; + funcCall = composeSQLExpr(expr); } } const maxEvalSpace = mergeEvalSpaces(...argExprs.map(e => e.evalSpace)); diff --git a/packages/malloy/src/lang/ast/expressions/expr-null.ts b/packages/malloy/src/lang/ast/expressions/expr-null.ts index ba7641021..c2742736f 100644 --- a/packages/malloy/src/lang/ast/expressions/expr-null.ts +++ b/packages/malloy/src/lang/ast/expressions/expr-null.ts @@ -7,12 +7,20 @@ import type {BinaryMalloyOperator, FieldSpace} from '..'; import type {ExprValue} from '../types/expr-value'; import {computedExprValue, literalExprValue} from '../types/expr-value'; import {ATNodeType, ExpressionDef} from '../types/expression-def'; +import {refuseBooleanValue} from './boolean-value'; function doIsNull(fs: FieldSpace, op: string, expr: ExpressionDef): ExprValue { // Build a new value rather than retyping the operand's in place: what // getExpression hands back is shared, and the operand is usually asked for // its value again after this. const nullCmp = expr.getExpression(fs); + refuseBooleanValue( + fs, + nullCmp, + 'in a null test', + expr, + 'a boolean here is a condition, which is false when unknown' + ); return computedExprValue({ dataType: {type: 'boolean'}, value: { diff --git a/packages/malloy/src/lang/ast/expressions/pick-when.ts b/packages/malloy/src/lang/ast/expressions/pick-when.ts index 5e9dba3da..292cc0491 100644 --- a/packages/malloy/src/lang/ast/expressions/pick-when.ts +++ b/packages/malloy/src/lang/ast/expressions/pick-when.ts @@ -11,6 +11,7 @@ import {computedExprValue} from '../types/expr-value'; import {ExpressionDef} from '../types/expression-def'; import type {FieldSpace} from '../types/field-space'; import {MalloyElement} from '../types/malloy-element'; +import {refuseBooleanValue} from './boolean-value'; interface Choice { pick: ExprValue; @@ -110,6 +111,13 @@ export class Pick extends ExpressionDef { 'Pick statments cannot have filter expression values' ); } + refuseBooleanValue( + fs, + definedReturnType, + 'as the result of a pick', + this, + "write values, pick 'yes' when … else 'no', or the condition itself with and, or and not" + ); caseValue.kids.caseElse = defaultVal.value; return computedExprValue({ dataType: definedReturnType, @@ -186,6 +194,13 @@ export class Pick extends ExpressionDef { 'Pick statments cannot have filter expression values' ); } + refuseBooleanValue( + fs, + definedReturnType, + 'as the result of a pick', + this, + "write values, pick 'yes' when … else 'no', or the condition itself with and, or and not" + ); pick.kids.caseElse = defVal.value; return computedExprValue({ dataType: definedReturnType, diff --git a/packages/malloy/src/lang/ast/field-space/index-field-space.ts b/packages/malloy/src/lang/ast/field-space/index-field-space.ts index e9108fc9d..3550acd51 100644 --- a/packages/malloy/src/lang/ast/field-space/index-field-space.ts +++ b/packages/malloy/src/lang/ast/field-space/index-field-space.ts @@ -123,8 +123,13 @@ export class IndexFieldSpace extends QueryOperationSpace { ); } else { const eTypeDesc = entry.typeDesc(); - // Don't index arrays and records - if ( + if (eTypeDesc.type === 'boolean' && dialect?.booleanType === 'none') { + wild.logWarning( + 'wildcard-omits-boolean', + `'${name}' is left out of '${wild.refString}': '${dialect.name}' does not support a boolean as a result column` + ); + } else if ( + // Don't index arrays and records TD.isBasicAtomic(eTypeDesc) && expressionIsScalar(eTypeDesc.expressionType) && (dialect === undefined || !dialect.ignoreInProject(name)) diff --git a/packages/malloy/src/lang/ast/field-space/query-spaces.ts b/packages/malloy/src/lang/ast/field-space/query-spaces.ts index 8c43b1adf..ae20fb223 100644 --- a/packages/malloy/src/lang/ast/field-space/query-spaces.ts +++ b/packages/malloy/src/lang/ast/field-space/query-spaces.ts @@ -207,7 +207,12 @@ export abstract class QueryOperationSpace ); } else { const eType = entry.typeDesc(); - if ( + if (eType.type === 'boolean' && dialect?.booleanType === 'none') { + wild.logWarning( + 'wildcard-omits-boolean', + `'${name}' is left out of '${wild.refString}': '${dialect.name}' does not support a boolean as a result column; write the value, pick 'yes' when ${name} else 'no'` + ); + } else if ( model.TD.isAtomic(eType) && model.expressionIsScalar(eType.expressionType) && (dialect === undefined || !dialect.ignoreInProject(name)) diff --git a/packages/malloy/src/lang/ast/query-items/field-declaration.ts b/packages/malloy/src/lang/ast/query-items/field-declaration.ts index 47e531469..f4582d88e 100644 --- a/packages/malloy/src/lang/ast/query-items/field-declaration.ts +++ b/packages/malloy/src/lang/ast/query-items/field-declaration.ts @@ -36,6 +36,7 @@ import type {Noteable} from '../types/noteable'; import type {DynamicSpace} from '../field-space/dynamic-space'; import {SpaceField} from '../types/space-field'; import {DefSpace} from '../field-space/def-space'; +import {refuseBooleanValue} from '../expressions/boolean-value'; export type FieldDeclarationConstructor = new ( expr: ExpressionDef, @@ -81,6 +82,11 @@ export abstract class AtomicFieldDeclaration return false; } + // The declaration is a result column of a query + outputsValue(): boolean { + return false; + } + queryFieldDef(exprFS: FieldSpace, exprName: string): AtomicFieldDef { let exprValue: ExprValue; @@ -121,6 +127,15 @@ export abstract class AtomicFieldDeclaration } if (isAtomicFieldType(exprValue.type) && exprValue.type !== 'error') { this.typecheckExprValue(exprValue); + if (this.outputsValue()) { + refuseBooleanValue( + exprFS, + exprValue, + 'as a result column', + this, + `write the value, pick 'yes' when ${this.exprSrc ?? '…'} else 'no'` + ); + } const ret = mkFieldDef(TDU.atomicDef(exprValue), exprName); if (TD.isTemporal(ret) && isGranularResult(exprValue)) { ret.timeframe = exprValue.timeframe; @@ -165,6 +180,9 @@ export abstract class AtomicFieldDeclaration export class CalculateFieldDeclaration extends AtomicFieldDeclaration { elementType = 'calculateFieldDeclaration'; + outputsValue(): boolean { + return true; + } typecheckExprValue(expr: ExprValue) { typecheckCalculate(expr, this); } @@ -175,6 +193,9 @@ export class CalculateFieldDeclaration extends AtomicFieldDeclaration { export class AggregateFieldDeclaration extends AtomicFieldDeclaration { elementType = 'aggregateFieldDeclaration'; + outputsValue(): boolean { + return true; + } typecheckExprValue(expr: ExprValue) { typecheckAggregate(expr, this); } @@ -182,6 +203,9 @@ export class AggregateFieldDeclaration extends AtomicFieldDeclaration { export class GroupByFieldDeclaration extends AtomicFieldDeclaration { elementType = 'groupByFieldDeclaration'; + outputsValue(): boolean { + return true; + } typecheckExprValue(expr: ExprValue) { typecheckGroupBy(expr, this); } @@ -189,6 +213,9 @@ export class GroupByFieldDeclaration extends AtomicFieldDeclaration { export class ProjectFieldDeclaration extends AtomicFieldDeclaration { elementType = 'projectFieldDeclaration'; + outputsValue(): boolean { + return true; + } typecheckExprValue(expr: ExprValue) { typecheckProject(expr, this); } diff --git a/packages/malloy/src/lang/ast/query-items/field-references.ts b/packages/malloy/src/lang/ast/query-items/field-references.ts index 1a0ea4a54..79a66666b 100644 --- a/packages/malloy/src/lang/ast/query-items/field-references.ts +++ b/packages/malloy/src/lang/ast/query-items/field-references.ts @@ -20,6 +20,7 @@ import {ListOf, MalloyElement} from '../types/malloy-element'; import type {Noteable} from '../types/noteable'; import type {MakeEntry} from '../types/space-entry'; import {ExprIdReference} from '../expressions/expr-id-reference'; +import {refuseBooleanValue} from '../expressions/boolean-value'; import type {FieldDeclarationConstructor} from './field-declaration'; import { AggregateFieldDeclaration, @@ -175,12 +176,26 @@ export abstract class FieldReference abstract typecheck(type: TypeDesc): void; + // The reference is a result column of a query + outputsValue(): boolean { + return false; + } + getField(fs: FieldSpace): LookupResult { const result = fs.lookup(this.list); if (result.found) { const actualType = result.found.typeDesc(); this.typecheck(actualType); + if (this.outputsValue()) { + refuseBooleanValue( + fs, + actualType, + 'as a result column', + this, + `write the value, pick 'yes' when ${this.refString} else 'no'` + ); + } } return result; @@ -252,6 +267,9 @@ export class DrillFieldReference extends FieldReference { export class CalculateFieldReference extends FieldReference { elementType = 'calculateFieldReference'; + outputsValue(): boolean { + return true; + } protected autoRenameDeclarationCtor() { return CalculateFieldDeclaration; } @@ -262,6 +280,9 @@ export class CalculateFieldReference extends FieldReference { export class IndexFieldReference extends FieldReference { elementType = 'indexFieldReference'; + outputsValue(): boolean { + return true; + } typecheck(type: TypeDesc) { typecheckIndex(type, this); } @@ -278,6 +299,9 @@ export class IndexFieldReference extends FieldReference { export class AggregateFieldReference extends FieldReference { elementType = 'aggregateFieldReference'; + outputsValue(): boolean { + return true; + } protected autoRenameDeclarationCtor() { return AggregateFieldDeclaration; } @@ -288,6 +312,9 @@ export class AggregateFieldReference extends FieldReference { export class GroupByFieldReference extends FieldReference { elementType = 'groupByFieldReference'; + outputsValue(): boolean { + return true; + } protected autoRenameDeclarationCtor() { return GroupByFieldDeclaration; } @@ -298,6 +325,9 @@ export class GroupByFieldReference extends FieldReference { export class ProjectFieldReference extends FieldReference { elementType = 'projectFieldReference'; + outputsValue(): boolean { + return true; + } protected autoRenameDeclarationCtor() { return ProjectFieldDeclaration; } diff --git a/packages/malloy/src/lang/ast/types/expression-def.ts b/packages/malloy/src/lang/ast/types/expression-def.ts index bdb63acd4..5a1ed6edb 100644 --- a/packages/malloy/src/lang/ast/types/expression-def.ts +++ b/packages/malloy/src/lang/ast/types/expression-def.ts @@ -44,6 +44,7 @@ import { TemporalFilterExpression, isFilterable, } from '@malloydata/malloy-filter'; +import {refuseBooleanValue} from '../expressions/boolean-value'; class TypeMismatch extends Error {} @@ -432,6 +433,16 @@ function equality( const err = errorCascade('boolean', lhs, rhs); if (err) return err; + if (lhs.type === 'boolean' && rhs.type === 'boolean') { + refuseBooleanValue( + fs, + lhs, + `compared with ${op}`, + left, + 'test the condition itself, or its not' + ); + } + // Unsupported types can be compare with null const lhRaw = TD.isSQL(lhs) ? lhs.rawType || 'typeless-left' : undefined; const rhRaw = TD.isSQL(rhs) ? rhs.rawType || 'typeless-right' : undefined; @@ -454,6 +465,13 @@ function equality( `Cannot use the '${op}' operator with a filter expression` ); } + refuseBooleanValue( + fs, + lhs, + 'matched with a filter expression', + left, + 'test the condition itself' + ); if (isFilterable(lhs.type)) { let actualFilter = rhs.value; while (actualFilter.node === '()') { diff --git a/packages/malloy/src/lang/parse-log.ts b/packages/malloy/src/lang/parse-log.ts index d58862c6b..1b7aa395b 100644 --- a/packages/malloy/src/lang/parse-log.ts +++ b/packages/malloy/src/lang/parse-log.ts @@ -94,6 +94,8 @@ type MessageParameterTypes = { 'experiment-not-enabled': {experimentId: string}; 'experimental-dialect-not-enabled': {dialect: string}; 'nesting-unsupported': {dialect: string}; + 'boolean-value-unsupported': {dialect: string; where: string; hint: string}; + 'wildcard-omits-boolean': string; 'nested-multi-stage-unsupported': {dialect: string}; 'nested-projection-limit-unsupported': {dialect: string}; 'sql-native-not-allowed-in-expression': { @@ -466,6 +468,8 @@ export const MESSAGE_FORMATTERS: PartialErrorCodeMessageMap = { 'experimental-dialect-not-enabled': e => `Requires compiler flag '##! experimental.dialect.${e.dialect}'`, 'nesting-unsupported': e => `'${e.dialect}' does not support nested queries`, + 'boolean-value-unsupported': e => + `'${e.dialect}' does not support a boolean ${e.where}; ${e.hint}`, 'nested-multi-stage-unsupported': e => `'${e.dialect}' does not support a multi-stage pipeline ('->') in a nested query`, 'nested-projection-limit-unsupported': e => diff --git a/packages/malloy/src/lang/test/boolean-none.spec.ts b/packages/malloy/src/lang/test/boolean-none.spec.ts new file mode 100644 index 000000000..424c78a8b --- /dev/null +++ b/packages/malloy/src/lang/test/boolean-none.spec.ts @@ -0,0 +1,155 @@ +/* + * Copyright Contributors to the Malloy project + * SPDX-License-Identifier: MIT + */ + +import './parse-expects'; +import { + errorMessage, + makeModelFunc, + model, + warningMessage, +} from './test-translator'; + +// On a dialect with booleanType 'none' a boolean is a condition and never a +// value. `m` is a SQL Server source with a declared boolean, `big`. +const m = makeModelFunc({ + prefix: `##! experimental.dialect.sqlserver +source: m is _mssql_.table('aTable') extend { + dimension: big is ai > 5 + measure: n is count() +} +`, +}); + +const refused = (where: string) => + errorMessage(new RegExp(`^'sqlserver' does not support a boolean ${where};`)); + +describe('a boolean on a dialect without boolean values', () => { + describe('is a condition', () => { + test('in where: and having:', () => { + expect( + m`run: m -> { where: big; aggregate: n; having: n > 1 }` + ).toTranslate(); + }); + test('in pick … when', () => { + expect( + m`run: m -> { group_by: size is pick 'big' when big else 'small' }` + ).toTranslate(); + }); + test('in a filtered aggregate', () => { + expect( + m`run: m -> { aggregate: bigs is n { where: big } }` + ).toTranslate(); + }); + test('in a join on:', () => { + expect( + m`source: j is m extend { join_one: o is m on big and o.ai = ai }` + ).toTranslate(); + }); + test('under not, and, or', () => { + expect( + m`run: m -> { where: not big and (big or ai = 1); aggregate: n }` + ).toTranslate(); + }); + test('as a declared dimension', () => { + expect( + m`source: m2 is m extend { dimension: small is not big }` + ).toTranslate(); + }); + }); + + describe('is refused as a value', () => { + test('a declared boolean in group_by:', () => { + expect(m`run: m -> { group_by: big; aggregate: n }`).toLog( + errorMessage( + "'sqlserver' does not support a boolean as a result column; write the value, pick 'yes' when big else 'no'" + ) + ); + }); + test('a comparison in select:', () => { + expect(m`run: m -> { select: is_big is ai > 5 }`).toLog( + errorMessage( + "'sqlserver' does not support a boolean as a result column; write the value, pick 'yes' when ai > 5 else 'no'" + ) + ); + }); + test('a boolean aggregate', () => { + expect(m`run: m -> { aggregate: any_big is n > 1 }`).toLog( + refused('as a result column') + ); + }); + test('in index:', () => { + expect(m`run: m -> { index: big }`).toLog(refused('as a result column')); + }); + test('in a null test', () => { + expect(m`run: m -> { where: big is null; aggregate: n }`).toLog( + refused('in a null test') + ); + }); + test('compared with = or !=', () => { + expect(m`run: m -> { where: big = true; aggregate: n }`).toLog( + refused('compared with =') + ); + expect(m`run: m -> { where: big != (ai > 6); aggregate: n }`).toLog( + refused('compared with !=') + ); + }); + test('in an alternation', () => { + expect(m`run: m -> { where: big ? true | false; aggregate: n }`).toLog( + refused('compared with =') + ); + }); + test('in ??', () => { + expect(m`run: m -> { where: big ?? false; aggregate: n }`).toLog( + refused('in \\?\\?') + ); + }); + test('as the result of a pick', () => { + expect( + m`run: m -> { where: pick true when ai > 1 else false; aggregate: n }` + ).toLog(refused('as the result of a pick')); + }); + test('as the result of a case', () => { + expect( + m`run: m -> { where: case when ai > 1 then true else false end; aggregate: n }` + ).toLog( + warningMessage('Use a `pick` statement instead of `case`'), + refused('as the result of a case') + ); + }); + test('matched with a filter expression', () => { + expect(m`run: m -> { where: big ~ f'true'; aggregate: n }`).toLog( + refused('matched with a filter expression') + ); + }); + test('as a function argument', () => { + expect(m`run: m -> { group_by: s is concat('x', big) }`).toLog( + refused('as an argument of concat') + ); + }); + test('in a cast, either way', () => { + expect(m`run: m -> { group_by: s is big::string }`).toLog( + refused('in a cast') + ); + expect(m`run: m -> { where: ai::boolean; aggregate: n }`).toLog( + refused('as the type of a cast') + ); + }); + }); + + test('select: * leaves a boolean out, with a warning', () => { + expect(m`run: m -> { select: * }`).toLog( + warningMessage( + "'big' is left out of '*': 'sqlserver' does not support a boolean as a result column; write the value, pick 'yes' when big else 'no'" + ) + ); + }); + + test('the same boolean is a value on a dialect with booleans', () => { + expect(model` + source: d is _db_.table('aTable') extend { dimension: big is ai > 5 } + run: d -> { group_by: big, s is big::string, c is concat('x', big); where: big = true } + `).toTranslate(); + }); +}); diff --git a/packages/malloy/src/lang/test/test-translator.ts b/packages/malloy/src/lang/test/test-translator.ts index 2b23c7658..55a3579ab 100644 --- a/packages/malloy/src/lang/test/test-translator.ts +++ b/packages/malloy/src/lang/test/test-translator.ts @@ -223,6 +223,17 @@ export const pgTableDef: SourceDef = { fields: baseFields, }; +// SQL Server has no boolean values (booleanType 'none'), so no column of +// its table is a boolean +export const mssqlTableDef: SourceDef = { + type: 'table', + name: 'aTable', + dialect: 'sqlserver', + tablePath: 'aTable', + connection: '_mssql_', + fields: baseFields.filter(f => f.name !== 'abool'), +}; + /** * A TestTranlator never actually talks to connection, instead uses * some mocked schema definitions. @@ -233,6 +244,7 @@ export const mockSchema: TableSourceDef[] = [ db2TableDef, bqTableDef, pgTableDef, + mssqlTableDef, { type: 'table', name: 'carriers', @@ -396,7 +408,7 @@ export interface TestTranslatorOptions { /** * Every connection name a test mentions resolves to a dialect, as it would - * through a host's connection lookup: the four listed below to theirs, and + * through a host's connection lookup: the five listed below to theirs, and * any other name to `TEST_DIALECT`. */ class TestConnectionDialectZone extends Zone { @@ -412,7 +424,7 @@ export class TestTranslator extends MalloyTranslator { testRoot?: TestRoot; connectionDialectZone = new TestConnectionDialectZone(); /* - * There are four connections: + * There are five connections: * _db_ - duckdb dialect, with the following tables ... * aTable, malloytest.carriers, malloytest.flights, malloytest.airports * _db2_ - duckdb dialect, with one table, aTable. A second connection @@ -421,6 +433,8 @@ export class TestTranslator extends MalloyTranslator { * aTable * _pg_ - postgres dialect, with one table * aTable + * _mssql_ - sqlserver dialect, which has no boolean values, with one + * table, aTable * * The "aTable" table is a mocked table with one column of each type. * The _bq_ version does not have the timestamptz column, and when @@ -450,6 +464,7 @@ export class TestTranslator extends MalloyTranslator { _db2_: {type: 'connection', name: '_db2_'}, _bq_: {type: 'connection', name: '_bq_'}, _pg_: {type: 'connection', name: '_pg_'}, + _mssql_: {type: 'connection', name: '_mssql_'}, a: {...aTableDef, primaryKey: 'astr', name: 'a'}, b: {...aTableDef, primaryKey: 'astr', name: 'b'}, bq_a: {...bqTableDef, primaryKey: 'astr', name: 'bq_a'}, @@ -523,6 +538,7 @@ export class TestTranslator extends MalloyTranslator { this.connectionDialectZone.define('_db2_', TEST_DIALECT); this.connectionDialectZone.define('_bq_', 'standardsql'); this.connectionDialectZone.define('_pg_', 'postgres'); + this.connectionDialectZone.define('_mssql_', 'sqlserver'); for (const flag of options.compilerFlags ?? []) { this.compilerFlagSrc.push(`##! ${flag}\n`); } From a5fdaf39562161ed7ca51fcc51340e114c184f4c Mon Sep 17 00:00:00 2001 From: James Estes Date: Mon, 19 Oct 2026 17:21:23 -0600 Subject: [PATCH 05/15] The compiler writes a boolean only as a condition on a dialect without boolean values Nothing converts a condition into a 1/0 or back: a declared boolean is its comparison, and the translator refuses a boolean where a value is wanted. Malloy's not, != and !~ are true where SQL says NULL; Dialect.sqlNot and sqlNullIsTrue spell that, as a condition on a dialect with booleanType 'none' and as today's COALESCE everywhere else. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01T1vFteHTo61YyRRVVrJAn4 Signed-off-by: James Estes --- packages/malloy-db-sqlserver/README.md | 6 +- packages/malloy/src/dialect/dialect.spec.ts | 24 ---- packages/malloy/src/dialect/dialect.ts | 43 +------ packages/malloy/src/dialect/index.ts | 1 - .../dialect/sqlserver/function_overrides.ts | 16 +-- packages/malloy/src/model/CONTEXT.md | 23 ++-- .../malloy/src/model/expression_compiler.ts | 116 +++--------------- packages/malloy/src/model/field_instance.ts | 12 +- packages/malloy/src/model/filter_compilers.ts | 24 ++-- packages/malloy/src/model/malloy_types.ts | 3 - packages/malloy/src/model/query_query.ts | 8 +- packages/malloy/src/test/resultMatchers.ts | 4 +- 12 files changed, 56 insertions(+), 224 deletions(-) diff --git a/packages/malloy-db-sqlserver/README.md b/packages/malloy-db-sqlserver/README.md index acfa19f15..35daa9d76 100644 --- a/packages/malloy-db-sqlserver/README.md +++ b/packages/malloy-db-sqlserver/README.md @@ -49,7 +49,7 @@ Cancelling a query uses the driver, so no `VIEW SERVER STATE` or `ALTER ANY CONN ## What works -Sources from tables (`table('schema.table')`, `[bracketed]` names, three-part `db.schema.table`) and SQL blocks; `select`, `group_by`, `aggregate`, `where`, `having`, `order_by`, `limit`; expressions as dimensions; joins with symmetric aggregates across a fan-out; multi-stage pipelines and a query used as a source; dates, timestamps, truncation, extraction, intervals, and `timezone:` with IANA names through `AT TIME ZONE`; the standard function library in its T-SQL spellings; booleans in both positions T-SQL has for them, a comparison, a declared boolean, `not`, `!=`, `pick` or a boolean function written as a condition in `where:`, `having:` and `pick … when`, and as a `1`/`0` value in `select:`, `group_by:` and a function argument. +Sources from tables (`table('schema.table')`, `[bracketed]` names, three-part `db.schema.table`) and SQL blocks; `select`, `group_by`, `aggregate`, `where`, `having`, `order_by`, `limit`; expressions as dimensions; joins with symmetric aggregates across a fan-out; multi-stage pipelines and a query used as a source; dates, timestamps, truncation, extraction, intervals, and `timezone:` with IANA names through `AT TIME ZONE`; the standard function library in its T-SQL spellings; a boolean as a condition: a comparison, a declared boolean, `not`, `!=`, `!~` or a boolean function in `where:`, `having:`, a join `on:`, a filtered aggregate and `pick … when`. ## What does not work yet @@ -59,7 +59,7 @@ Each of these is a later change, not a promise. |---|---| | `nest:` | Refused at translation (`supportsNesting` is false) | | Arrays and records, in table data or as literals | Declared unsupported; a JSON column is a string | -| `x::boolean` | Compile error: there is no boolean to cast to | +| A boolean where SQL expects a value: a declared boolean or a comparison in `select:`, `group_by:`, `aggregate:` or `index:`, as a function argument, in `??`, `is null`, `=` or a cast, or as the result of a `pick` | Compile error at the expression, naming the value to write instead, `pick 'yes' when is_late else 'no'`. A boolean is a condition on this dialect and never a value; `select: *` leaves a boolean out with a warning | | `all()`, `exclude()` | Compile error: ungrouped aggregates share the nest machinery | | `x ~ r'...'` | Error when SQL is generated: no regular expressions before SQL Server 2025 | | `regexp_extract`, `replace` with a regular expression | Server error: the functions arrive in SQL Server 2025 | @@ -79,7 +79,7 @@ Every Malloy query is one T-SQL batch: the session settings `SET DATEFIRST 7`, ` - A `datetimeoffset` is a Malloy `timestamptz`, an instant. Its clock is read at UTC, or in the query's `timezone:`, not at the offset the value was written in, so `t.day` is the same for `12:34:56 -05:00` as for `17:34:56 +00:00`. - Every string literal is written `N'...'`, so text outside the database's code page compares as itself. - The default collation compares case-insensitively; Malloy does not change that. -- A boolean is a `1`/`0` value in a result, in a `group_by:` and as a function argument, and a condition in `where:`, `having:` and `pick … when`; the compiler writes whichever T-SQL wants where the expression is, and a NULL boolean stays NULL. +- A boolean is a condition, never a value: `where: is_late` writes the comparison `is_late` was declared as. Malloy's `not` is true of NULL and `x != y` is true when either side is NULL, as on every dialect, written as `CASE … END = 0` tests rather than `COALESCE`, which has no boolean to return here. - `bit` is an integer, read as 0 or 1. `uniqueidentifier`, `text`, `ntext`, `time`, `binary`, `varbinary`, `xml`, `geography` and the other types the map does not name are `sql native`, to be passed through or cast in a `sql()` dimension. - A `bigint` arrives as the driver's text and is read as a number, so a value above 2^53 loses precision. - The connection's digest, which names the tables it may persist and the cache it may share, is built from the server, port, instance, database, authentication kind and the configured principal: the `user`, `domain\user`, or `clientId`. An ambient identity, `azure-default` or `azure-msi` with no `clientId`, is unknown until the first token request, so two such connections to one database share a digest. To tell them apart, set `clientId` (a user-assigned identity) or use `azure-service-principal`. diff --git a/packages/malloy/src/dialect/dialect.spec.ts b/packages/malloy/src/dialect/dialect.spec.ts index abe8ffa9c..2dd14053f 100644 --- a/packages/malloy/src/dialect/dialect.spec.ts +++ b/packages/malloy/src/dialect/dialect.spec.ts @@ -16,30 +16,6 @@ describe('Dialect booleans', () => { expect(noBooleans.sqlBoolean(false)).toBe('(1=0)'); }); - test('a dialect with no boolean type spells a literal value as 1 or 0', () => { - expect(noBooleans.sqlBooleanValue(true)).toBe('1'); - expect(noBooleans.sqlBooleanValue(false)).toBe('0'); - }); - - test('a dialect with booleans spells a literal the same in either form', () => { - expect(booleans.sqlBoolean(true)).toBe('true'); - expect(booleans.sqlBooleanValue(true)).toBe('true'); - expect(booleans.sqlBoolean(false)).toBe('false'); - expect(booleans.sqlBooleanValue(false)).toBe('false'); - }); - - test('a condition becomes a value which is NULL when the condition is', () => { - expect(noBooleans.sqlConditionAsValue('a > b')).toBe( - 'CASE WHEN a > b THEN 1 WHEN NOT (a > b) THEN 0 END' - ); - expect(booleans.sqlConditionAsValue('a > b')).toBe('a > b'); - }); - - test('a value becomes a condition by comparing it with 1', () => { - expect(noBooleans.sqlValueAsCondition('x')).toBe('(x = 1)'); - expect(booleans.sqlValueAsCondition('x')).toBe('x'); - }); - test('not, and a comparison true on NULL, keep COALESCE where there are booleans', () => { expect(booleans.sqlNot('c')).toBe('COALESCE(NOT c,TRUE)'); expect(booleans.sqlNullIsTrue('c')).toBe('COALESCE(c,true)'); diff --git a/packages/malloy/src/dialect/dialect.ts b/packages/malloy/src/dialect/dialect.ts index c049516ba..fdabf070a 100644 --- a/packages/malloy/src/dialect/dialect.ts +++ b/packages/malloy/src/dialect/dialect.ts @@ -180,12 +180,6 @@ export type OrderByClauseType = 'output_name' | 'ordinal' | 'expression'; export type GroupByClauseType = 'ordinal' | 'expression'; export type OrderByRequest = 'query' | 'turtle' | 'analytical'; export type BooleanTypeSupport = 'supported' | 'simulated' | 'none'; -/** - * Where SQL expects a boolean expression to be: a condition (WHERE, ON, - * CASE WHEN, the operands of AND, OR and NOT) or a value (everywhere else). - * A dialect with booleanType 'none' converts between the two. - */ -export type BooleanForm = 'condition' | 'value'; /** * Maps a range of integer values to a Malloy number type. @@ -1238,7 +1232,7 @@ export abstract class Dialect { } /** - * A boolean literal where a condition is expected + * SQL to generate to get a boolean value for a boolean expression */ sqlBoolean(bv: boolean): string { if (this.booleanType === 'none') { @@ -1248,38 +1242,9 @@ export abstract class Dialect { } /** - * A boolean literal where a value is expected - */ - sqlBooleanValue(bv: boolean): string { - if (this.booleanType === 'none') { - return bv ? '1' : '0'; - } - return bv ? 'true' : 'false'; - } - - /** - * On a dialect with booleanType 'none' a condition (a comparison, AND, - * IS NULL) is legal only where SQL expects a condition, and a value (a - * bit) only where SQL expects a value. The compiler crosses that boundary - * through these two; a NULL condition is a NULL value. On every other - * dialect each returns its argument. - */ - sqlConditionAsValue(condition: string): string { - if (this.booleanType === 'none') { - return `CASE WHEN ${condition} THEN 1 WHEN NOT (${condition}) THEN 0 END`; - } - return condition; - } - - sqlValueAsCondition(value: string): string { - if (this.booleanType === 'none') { - return `(${value} = 1)`; - } - return value; - } - - /** - * Malloy's `not`: of NULL, true + * Malloy's `not`: of NULL, true. A dialect with booleanType 'none' has no + * boolean value for COALESCE to return, so the test is written as a + * condition. */ sqlNot(condition: string): string { if (this.booleanType === 'none') { diff --git a/packages/malloy/src/dialect/index.ts b/packages/malloy/src/dialect/index.ts index 121c3ac0b..52e391e1b 100644 --- a/packages/malloy/src/dialect/index.ts +++ b/packages/malloy/src/dialect/index.ts @@ -25,7 +25,6 @@ export { } from './functions/util'; export {Dialect, queryZone} from './dialect'; export type { - BooleanForm, DialectFieldList, CompiledOrderBy, LateralJoinExpression, diff --git a/packages/malloy/src/dialect/sqlserver/function_overrides.ts b/packages/malloy/src/dialect/sqlserver/function_overrides.ts index 684e79c41..6a07b069f 100644 --- a/packages/malloy/src/dialect/sqlserver/function_overrides.ts +++ b/packages/malloy/src/dialect/sqlserver/function_overrides.ts @@ -12,15 +12,17 @@ export const SQLSERVER_MALLOY_STANDARD_OVERLOADS: OverrideMap = { ceil: {function: 'CEILING'}, chr: {sql: "CASE WHEN ${value} = 0 THEN '' ELSE NCHAR(${value}) END"}, div: {sql: 'FLOOR(${dividend} / ${divisor})'}, - // A boolean function is a 1/0 value; the compiler makes a condition of it - // where one is wanted. Of NULL, 0: the standard is COALESCE(..., false). + // starts_with and ends_with are written as comparisons because SQL Server + // has no boolean values: legal as a condition (where:, having:), a server + // error as a value. A NULL input matches nothing, where the standard's + // COALESCE(..., false) returns false. ends_with: { - sql: `CASE WHEN RIGHT(\${value}, ${len('${suffix}')}) = \${suffix} THEN 1 ELSE 0 END`, + sql: `RIGHT(\${value}, ${len('${suffix}')}) = \${suffix}`, }, ifnull: {function: 'ISNULL'}, - // A FLOAT holds neither infinity nor NaN - is_inf: {sql: '0'}, - is_nan: {sql: '0'}, + // A FLOAT holds neither infinity nor NaN, so neither condition is ever true + is_inf: {sql: '(1=0)'}, + is_nan: {sql: '(1=0)'}, length: {sql: len('${value}')}, ln: {sql: 'LOG(${value})'}, log: {sql: 'LOG(${value}, ${base})'}, @@ -33,7 +35,7 @@ export const SQLSERVER_MALLOY_STANDARD_OVERLOADS: OverrideMap = { atan2: {function: 'ATN2'}, trim: {characters: {sql: 'TRIM(${trim_characters} FROM ${value})'}}, starts_with: { - sql: `CASE WHEN LEFT(\${value}, ${len('${prefix}')}) = \${prefix} THEN 1 ELSE 0 END`, + sql: `LEFT(\${value}, ${len('${prefix}')}) = \${prefix}`, }, stddev: {function: 'STDEV'}, string_repeat: {function: 'REPLICATE'}, diff --git a/packages/malloy/src/model/CONTEXT.md b/packages/malloy/src/model/CONTEXT.md index 0e6848b88..180eb8eae 100644 --- a/packages/malloy/src/model/CONTEXT.md +++ b/packages/malloy/src/model/CONTEXT.md @@ -447,19 +447,16 @@ Compiles IR expression trees into SQL expressions. - Function calls - Field references and path expressions -**Conditions and values.** SQL has two places for a boolean: a *condition* (`WHERE`, `ON`, -`CASE WHEN`, the operands of `AND`/`OR`/`NOT`) and a *value* (everywhere else). On a dialect -with a boolean type the two are interchangeable; on one with `booleanType: 'none'` (SQL Server) -a comparison is legal only as a condition and a `bit` only as a value. `compileExpr` carries the -`wanted` form down the tree (`wantedForm`: the operands of `and`/`or`/`not`, a filter and a -subject-less `CASE WHEN` want a condition, everything else a value), knows each node's natural -form (`booleanForm`: comparisons, `in`, `like`, `is null`, a filter match and `sql_boolean` are -conditions), and crosses the boundary through `Dialect.sqlConditionAsValue` / -`sqlValueAsCondition`, which return their argument on every dialect with booleans. The literals -`true`/`false` are spelled in the wanted form (`sqlBoolean` / `sqlBooleanValue`), and Malloy's -NULL rules for `not`, `!=` and `!~` go through `Dialect.sqlNot` / `sqlNullIsTrue`. A -`where:`/`having:` list and a join `on:` are compiled wanting a condition; a field is compiled -wanting a value. +**A boolean on a dialect without boolean values.** SQL has two places for a boolean: a +*condition* (`WHERE`, `ON`, `CASE WHEN`, the operands of `AND`/`OR`/`NOT`) and a *value* +(everywhere else). On a dialect with `booleanType: 'none'` (SQL Server) a boolean is a condition +and never a value: a declared boolean compiles to its comparison, `where: is_late` writes the +comparison, and nothing converts a condition into a `1`/`0`. Malloy's NULL rules for `not`, +`!=` and `!~` (true where SQL says NULL) go through `Dialect.sqlNot` / `sqlNullIsTrue`, which +keep the `COALESCE` spelling on every dialect with booleans and write the test as a condition +on one without. A boolean where SQL wants a value (a result column, a function argument, `??`, a null +test, `=` between booleans, a cast, the result of a `pick`) is refused by the translator +(`lang/ast/expressions/boolean-value.ts`); the compiler does not look for it. ### 3. Dialect System diff --git a/packages/malloy/src/model/expression_compiler.ts b/packages/malloy/src/model/expression_compiler.ts index b079b03d1..7e635f306 100644 --- a/packages/malloy/src/model/expression_compiler.ts +++ b/packages/malloy/src/model/expression_compiler.ts @@ -60,7 +60,7 @@ import { } from './utils'; import {isBasicScalar} from './query_node'; import type {QueryStruct, QueryField} from './query_node'; -import type {BooleanForm, Dialect, QueryInfo} from '../dialect'; +import type {Dialect, QueryInfo} from '../dialect'; import {MalloyCompileError} from './malloy_compile_error'; import {lookupGivenValue} from './given_binding'; @@ -112,65 +112,6 @@ function deepCopyExpr(expr: T): T { return {...expr}; } -// The form a node's SQL takes when it is a boolean the compiler can tell -// apart: a condition or a value. Undefined for a node whose SQL is its -// child's ('()', 'filterCondition'), for raw SQL, and for the literals true -// and false, which are spelled in whichever form is wanted. -function booleanForm(expr: Expr): BooleanForm | undefined { - switch (expr.node) { - case '>': - case '<': - case '>=': - case '<=': - case '=': - case '!=': - case 'and': - case 'or': - case 'not': - case 'in': - case 'inGiven': - case 'like': - case '!like': - case 'is-null': - case 'is-not-null': - case 'regexpMatch': - case 'filterMatch': - return 'condition'; - case 'genericSQLExpr': - return expr.condition ? 'condition' : undefined; - case '()': - case 'filterCondition': - case 'true': - case 'false': - return undefined; - default: - return 'value'; - } -} - -// The form a node wants of one of its children -function wantedForm( - parent: Expr, - kid: string, - wanted: BooleanForm -): BooleanForm { - switch (parent.node) { - case 'and': - case 'or': - case 'not': - case 'filterCondition': - return 'condition'; - case '()': - return wanted; - case 'case': - return kid === 'caseWhen' && parent.kids.caseValue === undefined - ? 'condition' - : 'value'; - default: - return 'value'; - } -} - /** * Compiles an expression tree by mutating it in-place to set all .sql fields. * Assumes the expression tree is already a copy that can be mutated. @@ -180,8 +121,7 @@ function compileExpr( context: QueryStruct, expr: T, state: GenerateState = new GenerateState(), - wrap = true, - wanted: BooleanForm = 'value' + wrap = true ): T { /* * Translate the children first, and stash the translation @@ -189,24 +129,16 @@ function compileExpr( * it will have access to the translated children. */ if (exprHasE(expr)) { - compileExpr( - resultSet, - context, - expr.e, - state, - true, - wantedForm(expr, 'e', wanted) - ); + compileExpr(resultSet, context, expr.e, state); } else if (exprHasKids(expr)) { - for (const [kidName, kidExpr] of Object.entries(expr.kids)) { + for (const kidExpr of Object.values(expr.kids)) { if (kidExpr === null) continue; - const kidWanted = wantedForm(expr, kidName, wanted); if (Array.isArray(kidExpr)) { for (const e of kidExpr) { - compileExpr(resultSet, context, e, state, true, kidWanted); + compileExpr(resultSet, context, e, state); } } else { - compileExpr(resultSet, context, kidExpr, state, true, kidWanted); + compileExpr(resultSet, context, kidExpr, state); } } } @@ -216,11 +148,12 @@ function compileExpr( */ const qi = resultSet.getQueryInfo(); const dialectSQL = context.dialect.exprToSQL(qi, expr); + if (dialectSQL) { + expr.sql = wrap && exprHasKids(expr) ? `(${dialectSQL})` : dialectSQL; + return expr; + } const sql = (() => { - if (dialectSQL) { - return dialectSQL; - } switch (expr.node) { case 'field': return generateFieldFragment(resultSet, context, expr, state); @@ -352,9 +285,7 @@ function compileExpr( return `${expr.e.sql} IS NOT NULL`; case 'true': case 'false': - return wanted === 'condition' - ? context.dialect.sqlBoolean(expr.node === 'true') - : context.dialect.sqlBooleanValue(expr.node === 'true'); + return context.dialect.sqlBoolean(expr.node === 'true'); case 'null': return 'NULL'; case 'case': @@ -388,14 +319,7 @@ function compileExpr( } })(); - const form = booleanForm(expr); - const formed = - form === 'condition' && wanted === 'value' - ? context.dialect.sqlConditionAsValue(sql) - : form === 'value' && wanted === 'condition' - ? context.dialect.sqlValueAsCondition(sql) - : sql; - expr.sql = wrap && exprHasKids(expr) ? `(${formed})` : formed; + expr.sql = wrap && exprHasKids(expr) ? `(${sql})` : sql; return expr; } @@ -407,21 +331,13 @@ export function exprToSQL( resultSet: FieldInstanceResult, context: QueryStruct, exprToTranslate: Expr, - state: GenerateState = new GenerateState(), - wanted: BooleanForm = 'value' + state: GenerateState = new GenerateState() ): string { // Make a deep copy that we can mutate during compilation const exprCopy = deepCopyExpr(exprToTranslate); // Compile the copy, setting .sql on all nodes - const compiled = compileExpr( - resultSet, - context, - exprCopy, - state, - false, - wanted - ); + const compiled = compileExpr(resultSet, context, exprCopy, state, false); return compiled.sql!; } @@ -1001,9 +917,7 @@ export function generateFilterFragment( ): string { const allWhere = new AndChain(state.whereSQL); for (const cond of expr.kids.filterList) { - allWhere.add( - exprToSQL(resultSet, context, cond.e, state.withWhere(), 'condition') - ); + allWhere.add(exprToSQL(resultSet, context, cond.e, state.withWhere())); } return exprToSQL( resultSet, diff --git a/packages/malloy/src/model/field_instance.ts b/packages/malloy/src/model/field_instance.ts index 3a6a912f2..a44ae9256 100644 --- a/packages/malloy/src/model/field_instance.ts +++ b/packages/malloy/src/model/field_instance.ts @@ -3,7 +3,7 @@ * SPDX-License-Identifier: MIT */ -import type {BooleanForm, FieldReferenceType, QueryInfo} from '../dialect'; +import type {FieldReferenceType, QueryInfo} from '../dialect'; import type {QueryStruct} from './query_node'; import type { Expr, @@ -64,8 +64,7 @@ export class FieldInstanceField implements FieldInstance { resultSet: FieldInstanceResult, context: QueryStruct, expr: Expr, - state?: GenerateState, - wanted?: BooleanForm + state?: GenerateState ) => string; static registerExpressionCompiler( @@ -73,8 +72,7 @@ export class FieldInstanceField implements FieldInstance { resultSet: FieldInstanceResult, context: QueryStruct, expr: Expr, - state?: GenerateState, - wanted?: BooleanForm + state?: GenerateState ) => string ) { FieldInstanceField.exprCompiler = compiler; @@ -112,9 +110,7 @@ export class FieldInstanceField implements FieldInstance { return FieldInstanceField.exprCompiler( this.parent, this.f.parent, - this.f.fieldDef.e, - undefined, - this.fieldUsage.type === 'where' ? 'condition' : 'value' + this.f.fieldDef.e ); } diff --git a/packages/malloy/src/model/filter_compilers.ts b/packages/malloy/src/model/filter_compilers.ts index 22025cbfe..beb030148 100644 --- a/packages/malloy/src/model/filter_compilers.ts +++ b/packages/malloy/src/model/filter_compilers.ts @@ -172,13 +172,9 @@ export const FilterCompilers = { // must not fall through to the null/false truth-table below). return noneToSQL(bc.not, d); } else if (bc.operator === '=true') { - return bc.not - ? `NOT ${d.sqlValueAsCondition(px)}` - : d.sqlValueAsCondition(x); + return bc.not ? `NOT ${px}` : x; } else if (bc.operator === '=false') { - return bc.not - ? d.sqlValueAsCondition(x) - : `NOT ${d.sqlValueAsCondition(px)}`; + return bc.not ? x : `NOT ${px}`; } else if (bc.operator === 'null') { return bc.not ? `${px} IS NOT NULL` : `${px} IS NULL`; } @@ -189,25 +185,21 @@ export const FilterCompilers = { const quoteChar = d.sqlQuoteIdentifier('select')[0]; const isColumn = x.match(`^[()${quoteChar}\\w.]+$`); - const isFalse = `${px} = ${d.sqlBooleanValue(false)}`; if (isColumn) { if (bc.operator === 'true') { return bc.not - ? `${px} IS NULL OR ${isFalse}` - : `${px} IS NOT NULL AND ${d.sqlValueAsCondition(px)}`; + ? `${px} IS NULL OR ${px} = false` + : `${px} IS NOT NULL AND ${px}`; } return bc.not - ? `${px} IS NOT NULL AND ${d.sqlValueAsCondition(px)}` // not false: exclude null - : `${px} IS NULL OR ${isFalse}`; // false: include null + ? `${px} IS NOT NULL AND ${px}` // not false: exclude null + : `${px} IS NULL OR ${px} = false`; // false: include null } - const notNull = d.sqlValueAsCondition( - `COALESCE(${x}, ${d.sqlBooleanValue(false)})` - ); if (bc.operator === 'true') { - return bc.not ? `NOT ${notNull}` : notNull; + return bc.not ? `NOT COALESCE(${x}, false)` : `COALESCE(${x}, false)`; } // else bc.operator === 'false' - return bc.not ? notNull : `NOT ${notNull}`; + return bc.not ? `COALESCE(${x}, false)` : `NOT COALESCE(${x}, false)`; }, stringCompile(sc: StringFilter, x: string, d: Dialect): string { switch (sc.operator) { diff --git a/packages/malloy/src/model/malloy_types.ts b/packages/malloy/src/model/malloy_types.ts index c92b5fa23..45ffe45f0 100644 --- a/packages/malloy/src/model/malloy_types.ts +++ b/packages/malloy/src/model/malloy_types.ts @@ -429,9 +429,6 @@ export interface GenericSQLExpr extends ExprWithKids { node: 'genericSQLExpr'; kids: {args: Expr[]}; src: string[]; - // The SQL is a condition (sql_boolean), which a dialect without boolean - // values converts where a value is wanted - condition?: true; } export interface NullNode extends ExprLeaf { diff --git a/packages/malloy/src/model/query_query.ts b/packages/malloy/src/model/query_query.ts index 1fec0cb49..a559e8039 100644 --- a/packages/malloy/src/model/query_query.ts +++ b/packages/malloy/src/model/query_query.ts @@ -590,13 +590,7 @@ export class QueryQuery extends QueryField { (which === 'having' && expressionIsCalculation(cond.expressionType)) || (which === 'where' && expressionIsScalar(cond.expressionType)) ) { - const sqlClause = exprToSQL( - resultStruct, - context, - cond.e, - undefined, - 'condition' - ); + const sqlClause = exprToSQL(resultStruct, context, cond.e, undefined); resultFilters.add(sqlClause); } } diff --git a/packages/malloy/src/test/resultMatchers.ts b/packages/malloy/src/test/resultMatchers.ts index 8ad67035e..ade3afba4 100644 --- a/packages/malloy/src/test/resultMatchers.ts +++ b/packages/malloy/src/test/resultMatchers.ts @@ -443,10 +443,10 @@ function matchCell( return matchFail(path, expected, actual); } let actual: unknown = cell.number_value; - // A dialect without a boolean type returns 1/0 for a boolean + // Handle simulated booleans (MySQL returns 1/0 for booleans) if ( typeof expected === 'boolean' && - dialect.booleanType !== 'supported' && + dialect.booleanType === 'simulated' && typeof actual === 'number' ) { actual = actual !== 0; From 850c5fdbb7fa97bacf8ea4006771bc9c191c3580 Mon Sep 17 00:00:00 2001 From: James Estes Date: Mon, 19 Oct 2026 17:21:23 -0600 Subject: [PATCH 06/15] Tests that asserted through a boolean column assert through a pick A comparison selected as a result column is a boolean value, which a dialect without boolean values refuses. The tests whose subject was the comparison now assert it through a pick on every dialect; the tests whose subject is a boolean column are gated on booleanType. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01T1vFteHTo61YyRRVVrJAn4 Signed-off-by: James Estes --- test/src/databases/all/expr.spec.ts | 17 +-- test/src/databases/all/functions.spec.ts | 102 ++++++++---------- test/src/databases/all/new-matchers.spec.ts | 28 +++-- test/src/databases/all/orderby.spec.ts | 31 +++--- test/src/databases/all/test-select.spec.ts | 7 +- test/src/databases/all/time.spec.ts | 68 ++++++------ .../src/databases/sqlserver/sqlserver.spec.ts | 79 ++++++-------- 7 files changed, 159 insertions(+), 173 deletions(-) diff --git a/test/src/databases/all/expr.spec.ts b/test/src/databases/all/expr.spec.ts index b6c905c40..f0318aa55 100644 --- a/test/src/databases/all/expr.spec.ts +++ b/test/src/databases/all/expr.spec.ts @@ -527,12 +527,13 @@ describe.each(runtimes.runtimeList)('%s', (databaseName, runtime) => { source: a is ${databaseName}.table('malloytest.aircraft_models') extend { where: aircraft_model_code ? '0270202' } run: a -> { - group_by: boolean_1 is sql_boolean("\${seats} > 20") - group_by: boolean_2 is sql_boolean("\${engines} = 2") + group_by: + boolean_1 is pick 'yes' when sql_boolean("\${seats} > 20") else 'no' + boolean_2 is pick 'yes' when sql_boolean("\${engines} = 2") else 'no' } `).toMatchResult(testModel, { - boolean_1: true, - boolean_2: false, + boolean_1: 'yes', + boolean_2: 'no', }); }); @@ -1067,12 +1068,12 @@ describe.each(runtimes.runtimeList)('%s', (databaseName, runtime) => { dimension: fot is (false) or (true) } -> { select: - no_paren is false and fot - paren is false and (fot) + no_paren is pick 'yes' when false and fot else 'no' + paren is pick 'yes' when false and (fot) else 'no' }` ).toMatchResult(testModel, { - paren: false, - no_paren: false, + paren: 'no', + no_paren: 'no', }); }); }); diff --git a/test/src/databases/all/functions.spec.ts b/test/src/databases/all/functions.spec.ts index 98fe0554d..d7d99e51f 100644 --- a/test/src/databases/all/functions.spec.ts +++ b/test/src/databases/all/functions.spec.ts @@ -51,8 +51,6 @@ expressionModels.forEach((x, databaseName) => { const expressionModel = x.expressionModel; const runtime = x.runtime; const testModel = wrapTestModel(runtime, modelText(databaseName)); - const dbTrue = runtime.dialect.resultBoolean(true); - const dbFalse = runtime.dialect.resultBoolean(false); const funcTestGeneral = async ( expr: string, type: 'group_by' | 'aggregate', @@ -124,14 +122,19 @@ expressionModels.forEach((x, databaseName) => { await funcTestMultiple( ["concat('foo', 'bar')", 'foobar'], ["concat(1, 'bar')", '1bar'], - [ - "concat('cons', true)", - databaseName === 'postgres' - ? 'const' - : databaseName === 'mysql' - ? 'cons1' - : 'construe', - ], + // A dialect without boolean values has no boolean to pass + ...(runtime.dialect.booleanType === 'none' + ? [] + : [ + [ + "concat('cons', true)", + databaseName === 'postgres' + ? 'const' + : databaseName === 'mysql' + ? 'cons1' + : 'construe', + ] as [string, string], + ]), ["concat('foo', @2003)", 'foo2003-01-01'], [ "concat('foo', @2003-01-01 12:00:00)", @@ -624,16 +627,13 @@ expressionModels.forEach((x, databaseName) => { # test.debug run: state_facts -> { group_by: state - calculate: lag_val is lag(@2011-11-11 11:11:11, 1, now).year = now.year + calculate: lag_val is + pick 'now' when lag(@2011-11-11 11:11:11, 1, now).year = now.year else 'then' }` ) .run(); - expect(result.data.path(0, 'lag_val').value).toBe( - runtime.dialect.resultBoolean(true) - ); - expect(result.data.path(1, 'lag_val').value).toBe( - runtime.dialect.resultBoolean(false) - ); + expect(result.data.path(0, 'lag_val').value).toBe('now'); + expect(result.data.path(1, 'lag_val').value).toBe('then'); }); }); @@ -855,18 +855,18 @@ expressionModels.forEach((x, databaseName) => { : "'+inf'::number"; it.when(databaseName !== 'mysql')(`works - ${databaseName}`, async () => { await funcTestMultiple( - [`is_inf(${inf})`, dbTrue], - ['is_inf(100)', dbFalse], - ['is_inf(null)', dbFalse] + [`pick 'yes' when is_inf(${inf}) else 'no'`, 'yes'], + ["pick 'yes' when is_inf(100) else 'no'", 'no'], + ["pick 'yes' when is_inf(null) else 'no'", 'no'] ); }); }); describe('is_nan', () => { it.when(databaseName !== 'mysql')(`works - ${databaseName}`, async () => { await funcTestMultiple( - ["is_nan('NaN'::number)", dbTrue], - ['is_nan(100)', dbFalse], - ['is_nan(null)', dbFalse] + ["pick 'yes' when is_nan('NaN'::number) else 'no'", 'yes'], + ["pick 'yes' when is_nan(100) else 'no'", 'no'], + ["pick 'yes' when is_nan(null) else 'no'", 'no'] ); }); }); @@ -875,12 +875,12 @@ expressionModels.forEach((x, databaseName) => { await funcTestMultiple( ['greatest(1, 10, -100)', 10], [ - 'greatest(@2003, @2004, @1994) = @2004', - runtime.dialect.resultBoolean(true), + "pick 'yes' when greatest(@2003, @2004, @1994) = @2004 else 'no'", + 'yes', ], [ - 'greatest(@2023-05-26 11:58:00, @2023-05-26 11:59:00) = @2023-05-26 11:59:00', - runtime.dialect.resultBoolean(true), + "pick 'yes' when greatest(@2023-05-26 11:58:00, @2023-05-26 11:59:00) = @2023-05-26 11:59:00 else 'no'", + 'yes', ], ["greatest('a', 'b')", 'b'], ['greatest(1, null, 0)', null], @@ -892,13 +892,10 @@ expressionModels.forEach((x, databaseName) => { it(`works - ${databaseName}`, async () => { await funcTestMultiple( ['least(1, 10, -100)', -100], + ["pick 'yes' when least(@2003, @2004, @1994) = @1994 else 'no'", 'yes'], [ - 'least(@2003, @2004, @1994) = @1994', - runtime.dialect.resultBoolean(true), - ], - [ - 'least(@2023-05-26 11:58:00, @2023-05-26 11:59:00) = @2023-05-26 11:58:00', - runtime.dialect.resultBoolean(true), + "pick 'yes' when least(@2023-05-26 11:58:00, @2023-05-26 11:59:00) = @2023-05-26 11:58:00 else 'no'", + 'yes', ], ["least('a', 'b')", 'a'], ['least(1, null, 0)', null], @@ -929,34 +926,22 @@ expressionModels.forEach((x, databaseName) => { it(`works - ${databaseName}`, async () => { await funcTestMultiple( [ - "starts_with('hello world', 'hello')", - runtime.dialect.resultBoolean(true), - ], - [ - "starts_with('hello world', 'world')", - runtime.dialect.resultBoolean(false), + "pick 'yes' when starts_with('hello world', 'hello') else 'no'", + 'yes', ], - ["starts_with(null, 'world')", runtime.dialect.resultBoolean(false)], - [ - "starts_with('hello world', null)", - runtime.dialect.resultBoolean(false), - ] + ["pick 'yes' when starts_with('hello world', 'world') else 'no'", 'no'], + ["pick 'yes' when starts_with(null, 'world') else 'no'", 'no'], + ["pick 'yes' when starts_with('hello world', null) else 'no'", 'no'] ); }); }); describe('ends_with', () => { it(`works - ${databaseName}`, async () => { await funcTestMultiple( - [ - "ends_with('hello world', 'world')", - runtime.dialect.resultBoolean(true), - ], - [ - "ends_with('hello world', 'hello')", - runtime.dialect.resultBoolean(false), - ], - ["ends_with(null, 'world')", runtime.dialect.resultBoolean(false)], - ["ends_with('hello world', null)", runtime.dialect.resultBoolean(false)] + ["pick 'yes' when ends_with('hello world', 'world') else 'no'", 'yes'], + ["pick 'yes' when ends_with('hello world', 'hello') else 'no'", 'no'], + ["pick 'yes' when ends_with(null, 'world') else 'no'", 'no'], + ["pick 'yes' when ends_with('hello world', null) else 'no'", 'no'] ); }); }); @@ -1001,14 +986,17 @@ expressionModels.forEach((x, databaseName) => { // There are around a billion values that rand() can be, so if this // test fails, most likely something is broken. Otherwise, you're the lucky // one in a billion! - await funcTest('rand() = rand()', runtime.dialect.resultBoolean(false)); + await funcTest( + "pick 'same' when rand() = rand() else 'differ'", + 'differ' + ); }); }); describe('pi', () => { it(`is pi - ${databaseName}`, async () => { await funcTest( - 'abs(pi() - 3.141592653589793) < 0.0000000000001', - runtime.dialect.resultBoolean(true) + "pick 'yes' when abs(pi() - 3.141592653589793) < 0.0000000000001 else 'no'", + 'yes' ); }); }); diff --git a/test/src/databases/all/new-matchers.spec.ts b/test/src/databases/all/new-matchers.spec.ts index 56d3af3d6..fa3abe318 100644 --- a/test/src/databases/all/new-matchers.spec.ts +++ b/test/src/databases/all/new-matchers.spec.ts @@ -11,8 +11,11 @@ import '@malloydata/malloy/test/matchers'; const runtimes = new RuntimeList(databasesFromEnvironmentOr(allDatabases)); describe.each(runtimes.runtimeList)('New matchers for %s', (db, runtime) => { + // A dialect without boolean values has no boolean column + const booleans = runtime.dialect.booleanType !== 'none'; + describe('mkTestModel and TV', () => { - test('basic types with type inference', async () => { + test.when(booleans)('basic types with type inference', async () => { const tm = mkTestModel(runtime, { data: [ {t_int: 1, t_string: 'a', t_bool: true}, @@ -155,16 +158,19 @@ describe.each(runtimes.runtimeList)('New matchers for %s', (db, runtime) => { }); }); - test('plain booleans work with dialect boolean handling', async () => { - const tm = mkTestModel(runtime, { - data: [{a: true, b: false}], - }); - // Booleans are handled by dialect.resultBoolean() in the matcher - await expect('run: data -> { select: * }').toMatchResult(tm, { - a: true, - b: false, - }); - }); + test.when(booleans)( + 'plain booleans work with dialect boolean handling', + async () => { + const tm = mkTestModel(runtime, { + data: [{a: true, b: false}], + }); + // Booleans are handled by dialect.resultBoolean() in the matcher + await expect('run: data -> { select: * }').toMatchResult(tm, { + a: true, + b: false, + }); + } + ); }); describe('test.debug tag', () => { diff --git a/test/src/databases/all/orderby.spec.ts b/test/src/databases/all/orderby.spec.ts index 88bc7c847..83eb294fa 100644 --- a/test/src/databases/all/orderby.spec.ts +++ b/test/src/databases/all/orderby.spec.ts @@ -24,20 +24,26 @@ describe.each(runtimes.runtimeList)('%s', (databaseName, runtime) => { }` ); - it(`boolean type - ${databaseName}`, async () => { - await expect(` + // A dialect without boolean values cannot group by one + it.when(runtime.dialect.booleanType !== 'none')( + `boolean type - ${databaseName}`, + async () => { + await expect(` run: models-> { group_by: big is seats >=20 aggregate: model_count is count() } `).toMatchResult(orderByModel, { - big: false, - model_count: 58451, - }); - }); + big: false, + model_count: 58451, + }); + } + ); - it(`boolean in pipeline - ${databaseName}`, async () => { - await expect(` + it.when(runtime.dialect.booleanType !== 'none')( + `boolean in pipeline - ${databaseName}`, + async () => { + await expect(` run: models->{ group_by: manufacturer, @@ -48,10 +54,11 @@ describe.each(runtimes.runtimeList)('%s', (databaseName, runtime) => { aggregate: model_count is model_count.sum() } `).toMatchResult(orderByModel, { - big: false, - model_count: 58500, - }); - }); + big: false, + model_count: 58500, + }); + } + ); it(`filtered measures in model are aggregates #352 - ${databaseName}`, async () => { await expect(` diff --git a/test/src/databases/all/test-select.spec.ts b/test/src/databases/all/test-select.spec.ts index 142a5d1ad..50adc5150 100644 --- a/test/src/databases/all/test-select.spec.ts +++ b/test/src/databases/all/test-select.spec.ts @@ -15,8 +15,11 @@ describe.each(runtimes.runtimeList)('TestSelect for %s', (db, runtime) => { const ts = new TestSelect(runtime.dialect); const testModel = wrapTestModel(runtime, ''); + // A dialect without boolean values has no boolean column + const booleans = runtime.dialect.booleanType !== 'none'; + // Basic Type Tests - test(`${db} inferred basic types`, async () => { + test.when(booleans)(`${db} inferred basic types`, async () => { const sql = ts.generate( {t_int: 1, t_string: 'a', t_bool: true, t_float: 1.5}, {t_int: 2, t_string: 'b', t_bool: false, t_float: 2.5} @@ -27,7 +30,7 @@ describe.each(runtimes.runtimeList)('TestSelect for %s', (db, runtime) => { ]); }); - test(`${db} explicit type hints`, async () => { + test.when(booleans)(`${db} explicit type hints`, async () => { const sql = ts.generate({ t_int: ts.mk_int(1), t_float: ts.mk_float(1.5), diff --git a/test/src/databases/all/time.spec.ts b/test/src/databases/all/time.spec.ts index 300e171a0..2f7254d02 100644 --- a/test/src/databases/all/time.spec.ts +++ b/test/src/databases/all/time.spec.ts @@ -337,13 +337,13 @@ describe.each(runtimes.runtimeList)('%s date and time', (dbName, runtime) => { run: tzdata -> { select: ts is tstz::timestamp - same is tstz::timestamp = tstz - back is same_instant::timestamptz = tstz + same is pick 'yes' when tstz::timestamp = tstz else 'no' + back is pick 'yes' when same_instant::timestamptz = tstz else 'no' } `).toMatchResult(tzModel(), { ts: '2020-01-31T02:00:00.000Z', - same: true, - back: true, + same: 'yes', + back: 'yes', }); }); @@ -351,16 +351,16 @@ describe.each(runtimes.runtimeList)('%s date and time', (dbName, runtime) => { await expect(` run: tzdata -> { select: - eq_instant is tstz = same_instant - eq_digits is tstz = same_digits - gt_digits is tstz > same_digits - lt_digits is same_digits < tstz + eq_instant is pick 'yes' when tstz = same_instant else 'no' + eq_digits is pick 'yes' when tstz = same_digits else 'no' + gt_digits is pick 'yes' when tstz > same_digits else 'no' + lt_digits is pick 'yes' when same_digits < tstz else 'no' } `).toMatchResult(tzModel(), { - eq_instant: true, - eq_digits: false, - gt_digits: true, - lt_digits: true, + eq_instant: 'yes', + eq_digits: 'no', + gt_digits: 'yes', + lt_digits: 'yes', }); }); }); @@ -600,16 +600,16 @@ describe.each(runtimes.runtimeList)('%s date and time', (dbName, runtime) => { }); await expect(` run: fourTimes -> { select: - isBefore is tbefore ? @2026-01-01 - isFirst is tfirst ? @2026-01-01 - isLast is tlast ? @2026-01-01 - isAfter is tafter ? @2026-01-01 + isBefore is pick 'in' when tbefore ? @2026-01-01 else 'out' + isFirst is pick 'in' when tfirst ? @2026-01-01 else 'out' + isLast is pick 'in' when tlast ? @2026-01-01 else 'out' + isAfter is pick 'in' when tafter ? @2026-01-01 else 'out' } `).toMatchResult(tm, { - isBefore: false, - isFirst: true, - isLast: true, - isAfter: false, + isBefore: 'out', + isFirst: 'in', + isLast: 'in', + isAfter: 'out', }); }); }); @@ -940,8 +940,8 @@ describe.each(runtimes.runtimeList)('%s: query tz', (dbName, runtime) => { ], }); await expect( - 'run: tzdata -> { select: cmp is updated ? @2020-01 }' - ).toMatchResult(tzModel, {cmp: true}); + "run: tzdata -> { select: cmp is pick 'in' when updated ? @2020-01 else 'out' }" + ).toMatchResult(tzModel, {cmp: 'in'}); }); // 11:00 on the 31st in Tokyo is 20:00 on the 30th in Mexico City. With a @@ -1003,13 +1003,13 @@ describe.each(runtimes.runtimeList)('%s: query tz', (dbName, runtime) => { timezone: '${zone}' select: ts is tstz::timestamp - same is tstz::timestamp = tstz - back is same_instant::timestamptz = tstz + same is pick 'yes' when tstz::timestamp = tstz else 'no' + back is pick 'yes' when same_instant::timestamptz = tstz else 'no' } `).toMatchResult(tzModel(), { ts: '2020-01-31T02:00:00.000Z', - same: true, - back: true, + same: 'yes', + back: 'yes', }); }); }); @@ -1092,16 +1092,16 @@ describe.each(runtimes.runtimeList)('%s: query tz', (dbName, runtime) => { timezone: 'Europe/Dublin' select: june1_correct, - month_ok is by_month = june1_correct, by_month - day_ok is by_day = june1_correct, by_day - hour_ok is by_hour = june1_correct, by_hour - quarter_ok is by_quarter = august1_correct, by_quarter + month_ok is pick 'yes' when by_month = june1_correct else 'no', by_month + day_ok is pick 'yes' when by_day = june1_correct else 'no', by_day + hour_ok is pick 'yes' when by_hour = june1_correct else 'no', by_hour + quarter_ok is pick 'yes' when by_quarter = august1_correct else 'no', by_quarter }` ).toMatchResult(testModel, { - month_ok: true, - day_ok: true, - hour_ok: true, - quarter_ok: true, + month_ok: 'yes', + day_ok: 'yes', + hour_ok: 'yes', + quarter_ok: 'yes', }); }); }); diff --git a/test/src/databases/sqlserver/sqlserver.spec.ts b/test/src/databases/sqlserver/sqlserver.spec.ts index de7e0f710..9a4a4f07b 100644 --- a/test/src/databases/sqlserver/sqlserver.spec.ts +++ b/test/src/databases/sqlserver/sqlserver.spec.ts @@ -94,21 +94,18 @@ describe('SQL Server', () => { `).toEqualResult(tm, [{n: 51}]); }); - test('a declared boolean is a 0/1 value', async () => { - await expect(` - run: sqlserver.table('malloytest.state_facts') -> { - group_by: - big is airport_count > 500 - c_state is starts_with(state, 'C') - aggregate: n is count() - order_by: big desc, c_state desc - } - `).toEqualResult(tm, [ - {big: true, c_state: true, n: 1}, - {big: true, c_state: false, n: 11}, - {big: false, c_state: true, n: 2}, - {big: false, c_state: false, n: 37}, - ]); + test('a declared boolean in a result column is refused, naming the value to write', async () => { + await expect( + runtime + .loadQuery( + `run: sqlserver.table('malloytest.state_facts') extend { + dimension: big is airport_count > 500 + } -> { group_by: big; aggregate: n is count() }` + ) + .run() + ).rejects.toThrow( + "'sqlserver' does not support a boolean as a result column; write the value, pick 'yes' when big else 'no'" + ); }); test('a declared boolean is a condition in where: and having:', async () => { @@ -118,63 +115,47 @@ describe('SQL Server', () => { measure: n is count() } -> { where: big - group_by: big aggregate: n having: n > 1 } - `).toEqualResult(tm, [{big: true, n: 12}]); + `).toEqualResult(tm, [{n: 12}]); }); test('not, and a comparison true on NULL, follow Malloy rather than SQL', async () => { await expect(` run: sqlserver.sql('SELECT CAST(NULL AS INT) AS n, 1 AS one') -> { select: - not_null is not (n = 1) - ne_null is n != 1 - not_one is not (one = 1) - ne_one is one != 1 + not_null is pick 'yes' when not (n = 1) else 'no' + ne_null is pick 'yes' when n != 1 else 'no' + not_one is pick 'yes' when not (one = 1) else 'no' + ne_one is pick 'yes' when one != 1 else 'no' } `).toEqualResult(tm, [ - {not_null: true, ne_null: true, not_one: false, ne_one: false}, + {not_null: 'yes', ne_null: 'yes', not_one: 'no', ne_one: 'no'}, ]); }); - test('a boolean picks, and is picked', async () => { + test('a comparison picks', async () => { await expect(` run: sqlserver.table('malloytest.state_facts') -> { - group_by: - size is pick 'big' when airport_count > 500 else 'small' - flagged is pick true when airport_count > 500 else false + group_by: size is pick 'big' when airport_count > 500 else 'small' aggregate: n is count() order_by: size } `).toEqualResult(tm, [ - {size: 'big', flagged: true, n: 12}, - {size: 'small', flagged: false, n: 39}, + {size: 'big', n: 12}, + {size: 'small', n: 39}, ]); }); - test('a boolean function is a value, and a condition where one is wanted', async () => { + test('a boolean function is a condition', async () => { await expect(` run: sqlserver.table('malloytest.state_facts') -> { - where: starts_with(state, 'C') - select: state, c is starts_with(state, 'C'), a is ends_with(state, 'A') + where: starts_with(state, 'C') and not ends_with(state, 'A') + select: state order_by: state } - `).toEqualResult(tm, [ - {state: 'CA', c: true, a: true}, - {state: 'CO', c: true, a: false}, - {state: 'CT', c: true, a: false}, - ]); - }); - - test('a boolean literal is a value in select:', async () => { - await expect(` - run: sqlserver.table('malloytest.state_facts') -> { - select: t is true, f is false - limit: 1 - } - `).toEqualResult(tm, [{t: true, f: false}]); + `).toEqualResult(tm, [{state: 'CO'}, {state: 'CT'}]); }); test('sql_boolean is a condition', async () => { @@ -289,13 +270,13 @@ describe('SQL Server', () => { test('a constant dimension groups with the others', async () => { await expect(` run: sqlserver.table('malloytest.state_facts') -> { - group_by: tag is 'facts', big is airport_count > 500 + group_by: tag is 'facts', size is pick 'big' when airport_count > 500 else 'small' aggregate: n is count() - order_by: big desc + order_by: size } `).toEqualResult(tm, [ - {tag: 'facts', big: true, n: 12}, - {tag: 'facts', big: false, n: 39}, + {tag: 'facts', size: 'big', n: 12}, + {tag: 'facts', size: 'small', n: 39}, ]); }); From 6ea2ce910229c8ae39bed7f041b4c825ca353031 Mon Sep 17 00:00:00 2001 From: James Estes Date: Tue, 20 Oct 2026 10:58:38 -0600 Subject: [PATCH 07/15] A boolean as a value is refused once, at every comparison, and as a view or case subject The refusal returns an error-typed value which its caller returns, so an enclosing expression stays quiet and one mistake logs one error, at the expression which made it. Two booleans compared with any operator, a boolean field named as the whole view, and a boolean case subject are refused too. The wildcard warning names a joined field by its path and never a measure; the null-test hint says what to write. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01T1vFteHTo61YyRRVVrJAn4 Signed-off-by: James Estes --- .../src/lang/ast/expressions/boolean-value.ts | 26 +++-- .../malloy/src/lang/ast/expressions/case.ts | 15 ++- .../ast/expressions/expr-alternation-tree.ts | 5 +- .../src/lang/ast/expressions/expr-cast.ts | 32 +++--- .../src/lang/ast/expressions/expr-coalesce.ts | 7 +- .../src/lang/ast/expressions/expr-func.ts | 9 +- .../src/lang/ast/expressions/expr-null.ts | 7 +- .../src/lang/ast/expressions/pick-when.ts | 12 +- .../lang/ast/field-space/index-field-space.ts | 15 ++- .../src/lang/ast/field-space/query-spaces.ts | 15 ++- .../lang/ast/query-items/field-declaration.ts | 13 ++- .../lang/ast/query-items/field-references.ts | 4 +- .../src/lang/ast/query-properties/filters.ts | 10 +- .../src/lang/ast/types/expression-def.ts | 45 +++++--- .../lang/ast/view-elements/reference-view.ts | 12 ++ packages/malloy/src/lang/parse-log.ts | 4 +- .../malloy/src/lang/test/boolean-none.spec.ts | 107 +++++++++++++----- .../malloy/src/lang/test/test-translator.ts | 19 ++++ 18 files changed, 247 insertions(+), 110 deletions(-) diff --git a/packages/malloy/src/lang/ast/expressions/boolean-value.ts b/packages/malloy/src/lang/ast/expressions/boolean-value.ts index 2e642a4b2..d6d6e6f7f 100644 --- a/packages/malloy/src/lang/ast/expressions/boolean-value.ts +++ b/packages/malloy/src/lang/ast/expressions/boolean-value.ts @@ -3,30 +3,40 @@ * SPDX-License-Identifier: MIT */ +import type {ExprValue} from '../types/expr-value'; import type {FieldSpace} from '../types/field-space'; import type {MalloyElement} from '../types/malloy-element'; /** * On a dialect with booleanType 'none' a boolean is a condition and never a - * value. Logs an error at `logTo` when `type` is a boolean about to be used - * as a value; `where` says how, `hint` what to write instead. Returns true - * when it logged. + * value. When `type` is a boolean about to be used as a value, logs an error + * at `logTo` and returns an error-typed value for the caller to return in + * place of what it was building, so the enclosing expressions see an error + * and stay quiet. Otherwise returns undefined. `where` says how the boolean + * was used, `hint` what to write instead. */ -export function refuseBooleanValue( +export function booleanValueError( fs: FieldSpace, type: {type: string} | undefined, where: string, logTo: MalloyElement, hint: string -): boolean { +): ExprValue | undefined { const dialect = fs.dialectObj(); if (type?.type !== 'boolean' || dialect?.booleanType !== 'none') { - return false; + return undefined; } - logTo.logError('boolean-value-unsupported', { + return logTo.loggedErrorExpr('boolean-value-unsupported', { dialect: dialect.name, where, hint, }); - return true; } + +/** The hint where a boolean's unknown state is examined: `is null`, `??` */ +export const CONDITION_HINT = + 'test the condition itself; unknown already tests as false'; + +/** The hint for a `pick` or `case` whose result is a boolean */ +export const CHOICE_HINT = + "write values, pick 'yes' when … else 'no', or the condition itself with and, or and not"; diff --git a/packages/malloy/src/lang/ast/expressions/case.ts b/packages/malloy/src/lang/ast/expressions/case.ts index 5d5492a06..5c1c3b22b 100644 --- a/packages/malloy/src/lang/ast/expressions/case.ts +++ b/packages/malloy/src/lang/ast/expressions/case.ts @@ -10,7 +10,7 @@ import type {FieldSpace} from '../types/field-space'; import {MalloyElement} from '../types/malloy-element'; import * as TDU from '../typedesc-utils'; import type {CaseExpr} from '../../../model'; -import {refuseBooleanValue} from './boolean-value'; +import {booleanValueError, CHOICE_HINT} from './boolean-value'; interface Choice { then: ExprValue; @@ -46,6 +46,14 @@ export class Case extends ExpressionDef { let value: ExprValue | undefined = undefined; if (this.value) { const v = this.value.getExpression(fs); + const noSubject = booleanValueError( + fs, + v, + 'as the subject of a case', + this.value, + 'write the conditions, case when … then …' + ); + if (noSubject) return noSubject; dependents.push(v); resultExpr.kids.caseValue = v.value; value = v; @@ -95,13 +103,14 @@ export class Case extends ExpressionDef { dependents.push(elseValue); resultExpr.kids.caseElse = elseValue.value; } - refuseBooleanValue( + const noCase = booleanValueError( fs, returnType, 'as the result of a case', this, - "write values, pick 'yes' when … else 'no', or the condition itself with and, or and not" + CHOICE_HINT ); + if (noCase) return noCase; return computedExprValue({ value: resultExpr, dataType: returnType ? TDU.atomicDef(returnType) : {type: 'null'}, diff --git a/packages/malloy/src/lang/ast/expressions/expr-alternation-tree.ts b/packages/malloy/src/lang/ast/expressions/expr-alternation-tree.ts index 651446742..569b772f8 100644 --- a/packages/malloy/src/lang/ast/expressions/expr-alternation-tree.ts +++ b/packages/malloy/src/lang/ast/expressions/expr-alternation-tree.ts @@ -9,7 +9,7 @@ import type {FieldSpace} from '../types/field-space'; import {ATNodeType, ExpressionDef} from '../types/expression-def'; import type {BinaryMalloyOperator} from '../types/binary_operators'; import {isEquality} from '../types/binary_operators'; -import {refuseBooleanValue} from './boolean-value'; +import {booleanValueError} from './boolean-value'; /** * Return a flattened version of an alternation tree, if the tree is @@ -71,13 +71,14 @@ export class ExprAlternationTree extends ExpressionDef { if (inList.length > 0 && (applyOp === '=' || applyOp === '!=')) { const isIn = expr.getExpression(fs); const values = inList.map(v => v.getExpression(fs)); - refuseBooleanValue( + const noIn = booleanValueError( fs, isIn, `compared with ${applyOp}`, expr, 'test the condition itself, or its not' ); + if (noIn) return noIn; return computedExprValue({ dataType: {type: 'boolean'}, value: { diff --git a/packages/malloy/src/lang/ast/expressions/expr-cast.ts b/packages/malloy/src/lang/ast/expressions/expr-cast.ts index 6d68c738e..c4bcd6f2a 100644 --- a/packages/malloy/src/lang/ast/expressions/expr-cast.ts +++ b/packages/malloy/src/lang/ast/expressions/expr-cast.ts @@ -9,7 +9,7 @@ import type {ExprValue} from '../types/expr-value'; import {computedExprValue} from '../types/expr-value'; import {ExpressionDef} from '../types/expression-def'; import type {FieldSpace} from '../types/field-space'; -import {refuseBooleanValue} from './boolean-value'; +import {booleanValueError} from './boolean-value'; export class ExprCast extends ExpressionDef { elementType = 'cast'; @@ -47,20 +47,22 @@ export class ExprCast extends ExpressionDef { } } } - refuseBooleanValue( - fs, - expr, - 'in a cast', - this, - 'write the value, pick 1 when … else 0' - ); - refuseBooleanValue( - fs, - dataType, - 'as the type of a cast', - this, - 'write a comparison' - ); + const noCast = + booleanValueError( + fs, + expr, + 'in a cast', + this, + 'write the value, pick 1 when … else 0' + ) || + booleanValueError( + fs, + dataType, + 'as the type of a cast', + this, + 'write a comparison' + ); + if (noCast) return noCast; return computedExprValue({ dataType, value: castTo(this.castType, expr.value, expr.type, this.safe), diff --git a/packages/malloy/src/lang/ast/expressions/expr-coalesce.ts b/packages/malloy/src/lang/ast/expressions/expr-coalesce.ts index cbc5ee191..6fb76c1fa 100644 --- a/packages/malloy/src/lang/ast/expressions/expr-coalesce.ts +++ b/packages/malloy/src/lang/ast/expressions/expr-coalesce.ts @@ -9,7 +9,7 @@ import * as TDU from '../typedesc-utils'; import type {ExprValue} from '../types/expr-value'; import {ExpressionDef} from '../types/expression-def'; import type {FieldSpace} from '../types/field-space'; -import {refuseBooleanValue} from './boolean-value'; +import {booleanValueError, CONDITION_HINT} from './boolean-value'; export class ExprCoalesce extends ExpressionDef { elementType = 'coalesce expression'; @@ -24,13 +24,14 @@ export class ExprCoalesce extends ExpressionDef { protected computeExpression(fs: FieldSpace): ExprValue { const maybeNull = this.expr.getExpression(fs); const whenNull = this.altExpr.getExpression(fs); - refuseBooleanValue( + const noCoalesce = booleanValueError( fs, maybeNull, 'in ??', this, - 'a boolean here is a condition, which is false when unknown' + CONDITION_HINT ); + if (noCoalesce) return noCoalesce; if (maybeNull.type === 'null') { return whenNull; } diff --git a/packages/malloy/src/lang/ast/expressions/expr-func.ts b/packages/malloy/src/lang/ast/expressions/expr-func.ts index 871aa017c..26e500373 100644 --- a/packages/malloy/src/lang/ast/expressions/expr-func.ts +++ b/packages/malloy/src/lang/ast/expressions/expr-func.ts @@ -60,7 +60,7 @@ import {composeSQLExpr} from '../../../model/utils'; import * as TDU from '../typedesc-utils'; import {mergeRefSummaries} from '../../composite-source-utils'; import type {AnyMessageCodeAndParameters} from '../../parse-log'; -import {refuseBooleanValue} from './boolean-value'; +import {booleanValueError} from './boolean-value'; // Built-in functions that take a string literal of user-supplied SQL // and emit it directly. Gated by experimental.sql_functions in plain @@ -238,15 +238,16 @@ export class ExprFunc extends ExpressionDef { nullabilityErrors, returnType, } = result; - argExprs.forEach((argExpr, i) => { - refuseBooleanValue( + for (const [i, argExpr] of argExprs.entries()) { + const noArg = booleanValueError( fs, argExpr, `as an argument of ${this.name}`, this.args[i - (implicitExpr ? 1 : 0)] ?? this, 'write the value, pick 1 when … else 0' ); - }); + if (noArg) return noArg; + } // Report errors for expression type mismatch for (const error of expressionTypeErrors) { const adjustedIndex = error.argIndex - (implicitExpr ? 1 : 0); diff --git a/packages/malloy/src/lang/ast/expressions/expr-null.ts b/packages/malloy/src/lang/ast/expressions/expr-null.ts index c2742736f..76b6f2df6 100644 --- a/packages/malloy/src/lang/ast/expressions/expr-null.ts +++ b/packages/malloy/src/lang/ast/expressions/expr-null.ts @@ -7,20 +7,21 @@ import type {BinaryMalloyOperator, FieldSpace} from '..'; import type {ExprValue} from '../types/expr-value'; import {computedExprValue, literalExprValue} from '../types/expr-value'; import {ATNodeType, ExpressionDef} from '../types/expression-def'; -import {refuseBooleanValue} from './boolean-value'; +import {booleanValueError, CONDITION_HINT} from './boolean-value'; function doIsNull(fs: FieldSpace, op: string, expr: ExpressionDef): ExprValue { // Build a new value rather than retyping the operand's in place: what // getExpression hands back is shared, and the operand is usually asked for // its value again after this. const nullCmp = expr.getExpression(fs); - refuseBooleanValue( + const noTest = booleanValueError( fs, nullCmp, 'in a null test', expr, - 'a boolean here is a condition, which is false when unknown' + CONDITION_HINT ); + if (noTest) return noTest; return computedExprValue({ dataType: {type: 'boolean'}, value: { diff --git a/packages/malloy/src/lang/ast/expressions/pick-when.ts b/packages/malloy/src/lang/ast/expressions/pick-when.ts index 292cc0491..d027d1966 100644 --- a/packages/malloy/src/lang/ast/expressions/pick-when.ts +++ b/packages/malloy/src/lang/ast/expressions/pick-when.ts @@ -11,7 +11,7 @@ import {computedExprValue} from '../types/expr-value'; import {ExpressionDef} from '../types/expression-def'; import type {FieldSpace} from '../types/field-space'; import {MalloyElement} from '../types/malloy-element'; -import {refuseBooleanValue} from './boolean-value'; +import {booleanValueError, CHOICE_HINT} from './boolean-value'; interface Choice { pick: ExprValue; @@ -111,13 +111,14 @@ export class Pick extends ExpressionDef { 'Pick statments cannot have filter expression values' ); } - refuseBooleanValue( + const noPick = booleanValueError( fs, definedReturnType, 'as the result of a pick', this, - "write values, pick 'yes' when … else 'no', or the condition itself with and, or and not" + CHOICE_HINT ); + if (noPick) return noPick; caseValue.kids.caseElse = defaultVal.value; return computedExprValue({ dataType: definedReturnType, @@ -194,13 +195,14 @@ export class Pick extends ExpressionDef { 'Pick statments cannot have filter expression values' ); } - refuseBooleanValue( + const noPick = booleanValueError( fs, definedReturnType, 'as the result of a pick', this, - "write values, pick 'yes' when … else 'no', or the condition itself with and, or and not" + CHOICE_HINT ); + if (noPick) return noPick; pick.kids.caseElse = defVal.value; return computedExprValue({ dataType: definedReturnType, diff --git a/packages/malloy/src/lang/ast/field-space/index-field-space.ts b/packages/malloy/src/lang/ast/field-space/index-field-space.ts index 3550acd51..3af2cee09 100644 --- a/packages/malloy/src/lang/ast/field-space/index-field-space.ts +++ b/packages/malloy/src/lang/ast/field-space/index-field-space.ts @@ -123,17 +123,20 @@ export class IndexFieldSpace extends QueryOperationSpace { ); } else { const eTypeDesc = entry.typeDesc(); - if (eTypeDesc.type === 'boolean' && dialect?.booleanType === 'none') { - wild.logWarning( - 'wildcard-omits-boolean', - `'${name}' is left out of '${wild.refString}': '${dialect.name}' does not support a boolean as a result column` - ); - } else if ( + if ( // Don't index arrays and records TD.isBasicAtomic(eTypeDesc) && expressionIsScalar(eTypeDesc.expressionType) && (dialect === undefined || !dialect.ignoreInProject(name)) ) { + if (eTypeDesc.type === 'boolean' && dialect?.booleanType === 'none') { + wild.logWarning('wildcard-omits-boolean', { + dialect: dialect.name, + wildcard: wild.refString, + name: [...joinPath, name].join('.'), + }); + continue; + } expandEntries.push({name: indexName, entry}); this.expandedWild.set(indexName, { path: joinPath.concat(name), diff --git a/packages/malloy/src/lang/ast/field-space/query-spaces.ts b/packages/malloy/src/lang/ast/field-space/query-spaces.ts index ae20fb223..2b4228c3c 100644 --- a/packages/malloy/src/lang/ast/field-space/query-spaces.ts +++ b/packages/malloy/src/lang/ast/field-space/query-spaces.ts @@ -207,16 +207,19 @@ export abstract class QueryOperationSpace ); } else { const eType = entry.typeDesc(); - if (eType.type === 'boolean' && dialect?.booleanType === 'none') { - wild.logWarning( - 'wildcard-omits-boolean', - `'${name}' is left out of '${wild.refString}': '${dialect.name}' does not support a boolean as a result column; write the value, pick 'yes' when ${name} else 'no'` - ); - } else if ( + if ( model.TD.isAtomic(eType) && model.expressionIsScalar(eType.expressionType) && (dialect === undefined || !dialect.ignoreInProject(name)) ) { + if (eType.type === 'boolean' && dialect?.booleanType === 'none') { + wild.logWarning('wildcard-omits-boolean', { + dialect: dialect.name, + wildcard: wild.refString, + name: joinPath.concat(name).join('.'), + }); + continue; + } expandEntries.push({name, entry}); this.expandedWild.set(name, { path: joinPath.concat(name), diff --git a/packages/malloy/src/lang/ast/query-items/field-declaration.ts b/packages/malloy/src/lang/ast/query-items/field-declaration.ts index f4582d88e..de7c19f18 100644 --- a/packages/malloy/src/lang/ast/query-items/field-declaration.ts +++ b/packages/malloy/src/lang/ast/query-items/field-declaration.ts @@ -36,7 +36,7 @@ import type {Noteable} from '../types/noteable'; import type {DynamicSpace} from '../field-space/dynamic-space'; import {SpaceField} from '../types/space-field'; import {DefSpace} from '../field-space/def-space'; -import {refuseBooleanValue} from '../expressions/boolean-value'; +import {booleanValueError} from '../expressions/boolean-value'; export type FieldDeclarationConstructor = new ( expr: ExpressionDef, @@ -127,14 +127,17 @@ export abstract class AtomicFieldDeclaration } if (isAtomicFieldType(exprValue.type) && exprValue.type !== 'error') { this.typecheckExprValue(exprValue); - if (this.outputsValue()) { - refuseBooleanValue( + if ( + this.outputsValue() && + booleanValueError( exprFS, exprValue, 'as a result column', - this, + this.expr, `write the value, pick 'yes' when ${this.exprSrc ?? '…'} else 'no'` - ); + ) + ) { + return {name: exprName, type: 'error'}; } const ret = mkFieldDef(TDU.atomicDef(exprValue), exprName); if (TD.isTemporal(ret) && isGranularResult(exprValue)) { diff --git a/packages/malloy/src/lang/ast/query-items/field-references.ts b/packages/malloy/src/lang/ast/query-items/field-references.ts index 79a66666b..86120255b 100644 --- a/packages/malloy/src/lang/ast/query-items/field-references.ts +++ b/packages/malloy/src/lang/ast/query-items/field-references.ts @@ -20,7 +20,7 @@ import {ListOf, MalloyElement} from '../types/malloy-element'; import type {Noteable} from '../types/noteable'; import type {MakeEntry} from '../types/space-entry'; import {ExprIdReference} from '../expressions/expr-id-reference'; -import {refuseBooleanValue} from '../expressions/boolean-value'; +import {booleanValueError} from '../expressions/boolean-value'; import type {FieldDeclarationConstructor} from './field-declaration'; import { AggregateFieldDeclaration, @@ -188,7 +188,7 @@ export abstract class FieldReference const actualType = result.found.typeDesc(); this.typecheck(actualType); if (this.outputsValue()) { - refuseBooleanValue( + booleanValueError( fs, actualType, 'as a result column', diff --git a/packages/malloy/src/lang/ast/query-properties/filters.ts b/packages/malloy/src/lang/ast/query-properties/filters.ts index a05e5edc5..56f88c995 100644 --- a/packages/malloy/src/lang/ast/query-properties/filters.ts +++ b/packages/malloy/src/lang/ast/query-properties/filters.ts @@ -64,10 +64,12 @@ export class FilterElement extends MalloyElement { filterCondition(fs: FieldSpace): FilterCondition { const exprVal = this.expr.getExpression(fs); if (exprVal.type !== 'boolean') { - this.expr.logError( - 'non-boolean-filter', - 'Filter expression must have boolean value' - ); + if (exprVal.type !== 'error') { + this.expr.logError( + 'non-boolean-filter', + 'Filter expression must have boolean value' + ); + } return { node: 'filterCondition', code: this.exprSrc, diff --git a/packages/malloy/src/lang/ast/types/expression-def.ts b/packages/malloy/src/lang/ast/types/expression-def.ts index 5a1ed6edb..bed24e50e 100644 --- a/packages/malloy/src/lang/ast/types/expression-def.ts +++ b/packages/malloy/src/lang/ast/types/expression-def.ts @@ -44,7 +44,7 @@ import { TemporalFilterExpression, isFilterable, } from '@malloydata/malloy-filter'; -import {refuseBooleanValue} from '../expressions/boolean-value'; +import {booleanValueError} from '../expressions/boolean-value'; class TypeMismatch extends Error {} @@ -420,6 +420,30 @@ function regexEqual(left: ExprValue, right: ExprValue): Expr | undefined { return undefined; } +/** + * Two booleans can only be compared on a dialect with boolean values. Every + * comparison operator, applied plainly, as a partial, or through an + * alternation, arrives here through equality() or compare(). + */ +function booleanCompare( + fs: FieldSpace, + left: ExpressionDef, + lhs: ExprValue, + op: CompareMalloyOperator, + rhs: ExprValue +): ExprValue | undefined { + if (lhs.type !== 'boolean' || rhs.type !== 'boolean') { + return undefined; + } + return booleanValueError( + fs, + lhs, + `compared with ${op}`, + left, + 'test the condition itself, or its not' + ); +} + function equality( fs: FieldSpace, left: ExpressionDef, @@ -430,19 +454,10 @@ function equality( const rhs = right.getExpression(fs); const node = getExprNode(op); - const err = errorCascade('boolean', lhs, rhs); + const err = + errorCascade('boolean', lhs, rhs) || booleanCompare(fs, left, lhs, op, rhs); if (err) return err; - if (lhs.type === 'boolean' && rhs.type === 'boolean') { - refuseBooleanValue( - fs, - lhs, - `compared with ${op}`, - left, - 'test the condition itself, or its not' - ); - } - // Unsupported types can be compare with null const lhRaw = TD.isSQL(lhs) ? lhs.rawType || 'typeless-left' : undefined; const rhRaw = TD.isSQL(rhs) ? rhs.rawType || 'typeless-right' : undefined; @@ -465,13 +480,14 @@ function equality( `Cannot use the '${op}' operator with a filter expression` ); } - refuseBooleanValue( + const noMatch = booleanValueError( fs, lhs, 'matched with a filter expression', left, 'test the condition itself' ); + if (noMatch) return noMatch; if (isFilterable(lhs.type)) { let actualFilter = rhs.value; while (actualFilter.node === '()') { @@ -537,7 +553,8 @@ function compare( const lhs = left.getExpression(fs); const rhs = right.getExpression(fs); - const err = errorCascade('boolean', lhs, rhs); + const err = + errorCascade('boolean', lhs, rhs) || booleanCompare(fs, left, lhs, op, rhs); if (err) return err; const noCompare = unsupportError(left, lhs, right, rhs); diff --git a/packages/malloy/src/lang/ast/view-elements/reference-view.ts b/packages/malloy/src/lang/ast/view-elements/reference-view.ts index b6869be24..1bdd68686 100644 --- a/packages/malloy/src/lang/ast/view-elements/reference-view.ts +++ b/packages/malloy/src/lang/ast/view-elements/reference-view.ts @@ -15,6 +15,7 @@ import { sourceBase, } from '../../../model/malloy_types'; import {ErrorFactory} from '../error-factory'; +import {booleanValueError} from '../expressions/boolean-value'; import type {QueryOperationSpace} from '../field-space/query-spaces'; import type {ViewOrScalarFieldReference} from '../query-items/field-references'; import {attachDrillPaths} from '../query-properties/drill'; @@ -74,6 +75,17 @@ export class ReferenceView extends View { throw new Error('Expected field to have definition'); } if (isAtomic(fieldDef)) { + if ( + booleanValueError( + fs, + fieldDef, + 'as a result column', + this.reference, + `write the value, pick 'yes' when ${this.reference.refString} else 'no'` + ) + ) { + return oops(); + } const name = this.reference.nameString; const outputStruct: SourceDef = { ...sourceBase(fs.structDef()), diff --git a/packages/malloy/src/lang/parse-log.ts b/packages/malloy/src/lang/parse-log.ts index 1b7aa395b..1def9a04a 100644 --- a/packages/malloy/src/lang/parse-log.ts +++ b/packages/malloy/src/lang/parse-log.ts @@ -95,7 +95,7 @@ type MessageParameterTypes = { 'experimental-dialect-not-enabled': {dialect: string}; 'nesting-unsupported': {dialect: string}; 'boolean-value-unsupported': {dialect: string; where: string; hint: string}; - 'wildcard-omits-boolean': string; + 'wildcard-omits-boolean': {dialect: string; wildcard: string; name: string}; 'nested-multi-stage-unsupported': {dialect: string}; 'nested-projection-limit-unsupported': {dialect: string}; 'sql-native-not-allowed-in-expression': { @@ -470,6 +470,8 @@ export const MESSAGE_FORMATTERS: PartialErrorCodeMessageMap = { 'nesting-unsupported': e => `'${e.dialect}' does not support nested queries`, 'boolean-value-unsupported': e => `'${e.dialect}' does not support a boolean ${e.where}; ${e.hint}`, + 'wildcard-omits-boolean': e => + `'${e.name}' is left out of '${e.wildcard}': '${e.dialect}' does not support a boolean as a result column; write the value, pick 'yes' when ${e.name} else 'no'`, 'nested-multi-stage-unsupported': e => `'${e.dialect}' does not support a multi-stage pipeline ('->') in a nested query`, 'nested-projection-limit-unsupported': e => diff --git a/packages/malloy/src/lang/test/boolean-none.spec.ts b/packages/malloy/src/lang/test/boolean-none.spec.ts index 424c78a8b..5c807130e 100644 --- a/packages/malloy/src/lang/test/boolean-none.spec.ts +++ b/packages/malloy/src/lang/test/boolean-none.spec.ts @@ -12,12 +12,17 @@ import { } from './test-translator'; // On a dialect with booleanType 'none' a boolean is a condition and never a -// value. `m` is a SQL Server source with a declared boolean, `big`. +// value. `m` is a SQL Server source with a declared boolean, `big`, a boolean +// measure, `anyn`, and a view, `v`. In a refusal test, the `${…}` marks the +// expression the error is expected at. const m = makeModelFunc({ prefix: `##! experimental.dialect.sqlserver source: m is _mssql_.table('aTable') extend { dimension: big is ai > 5 measure: n is count() + measure: anyn is n > 1 + view: v is { group_by: ai } + join_one: o is _mssql_.table('aTable') extend { dimension: big is ai > 5 } on o.ai = ai } `, }); @@ -44,7 +49,7 @@ describe('a boolean on a dialect without boolean values', () => { }); test('in a join on:', () => { expect( - m`source: j is m extend { join_one: o is m on big and o.ai = ai }` + m`source: j is m extend { join_one: p is m on big and p.ai = ai }` ).toTranslate(); }); test('under not, and, or', () => { @@ -52,62 +57,84 @@ describe('a boolean on a dialect without boolean values', () => { m`run: m -> { where: not big and (big or ai = 1); aggregate: n }` ).toTranslate(); }); - test('as a declared dimension', () => { + test('as a declared dimension or measure', () => { expect( - m`source: m2 is m extend { dimension: small is not big }` + m`source: m2 is m extend { dimension: small is not big; measure: none is n = 0 }` ).toTranslate(); }); }); describe('is refused as a value', () => { test('a declared boolean in group_by:', () => { - expect(m`run: m -> { group_by: big; aggregate: n }`).toLog( + expect(m`run: m -> { group_by: ${'big'}; aggregate: n }`).toLog( errorMessage( "'sqlserver' does not support a boolean as a result column; write the value, pick 'yes' when big else 'no'" ) ); }); test('a comparison in select:', () => { - expect(m`run: m -> { select: is_big is ai > 5 }`).toLog( + expect(m`run: m -> { select: is_big is ${'ai > 5'} }`).toLog( errorMessage( "'sqlserver' does not support a boolean as a result column; write the value, pick 'yes' when ai > 5 else 'no'" ) ); }); test('a boolean aggregate', () => { - expect(m`run: m -> { aggregate: any_big is n > 1 }`).toLog( + expect(m`run: m -> { aggregate: any_big is ${'n > 1'} }`).toLog( refused('as a result column') ); }); test('in index:', () => { - expect(m`run: m -> { index: big }`).toLog(refused('as a result column')); + expect(m`run: m -> { index: ${'big'} }`).toLog( + refused('as a result column') + ); + }); + test('a boolean field named as the whole view', () => { + expect(m`run: m -> ${'big'}`).toLog(refused('as a result column')); + expect(m`run: m -> ${'anyn'}`).toLog(refused('as a result column')); + expect(m`run: m -> v + ${'big'}`).toLog(refused('as a result column')); }); test('in a null test', () => { - expect(m`run: m -> { where: big is null; aggregate: n }`).toLog( - refused('in a null test') + expect(m`run: m -> { where: ${'big'} is null; aggregate: n }`).toLog( + errorMessage( + "'sqlserver' does not support a boolean in a null test; test the condition itself; unknown already tests as false" + ) ); }); test('compared with = or !=', () => { - expect(m`run: m -> { where: big = true; aggregate: n }`).toLog( + expect(m`run: m -> { where: ${'big'} = true; aggregate: n }`).toLog( refused('compared with =') ); - expect(m`run: m -> { where: big != (ai > 6); aggregate: n }`).toLog( + expect(m`run: m -> { where: ${'big'} != (ai > 6); aggregate: n }`).toLog( refused('compared with !=') ); }); - test('in an alternation', () => { - expect(m`run: m -> { where: big ? true | false; aggregate: n }`).toLog( - refused('compared with =') + test('compared with an ordering operator', () => { + expect(m`run: m -> { where: ${'big'} > false; aggregate: n }`).toLog( + refused('compared with >') ); + expect(m`run: m -> { where: ${'big'} ? > false; aggregate: n }`).toLog( + refused('compared with >') + ); + expect( + m`run: m -> { where: ${'big'} ? > false | < true; aggregate: n }` + ).toLog(refused('compared with >'), refused('compared with <')); + }); + test('in an alternation', () => { + expect( + m`run: m -> { where: ${'big'} ? true | false; aggregate: n }` + ).toLog(refused('compared with =')); }); test('in ??', () => { - expect(m`run: m -> { where: big ?? false; aggregate: n }`).toLog( - refused('in \\?\\?') + expect(m`run: m -> { where: ${'big ?? false'}; aggregate: n }`).toLog( + errorMessage( + "'sqlserver' does not support a boolean in ??; test the condition itself; unknown already tests as false" + ) ); }); test('as the result of a pick', () => { expect( - m`run: m -> { where: pick true when ai > 1 else false; aggregate: n }` + m`run: m -> { where: ${'pick true when ai > 1 else false'}; aggregate: n }` ).toLog(refused('as the result of a pick')); }); test('as the result of a case', () => { @@ -119,37 +146,59 @@ describe('a boolean on a dialect without boolean values', () => { ); }); test('matched with a filter expression', () => { - expect(m`run: m -> { where: big ~ f'true'; aggregate: n }`).toLog( + expect(m`run: m -> { where: ${'big'} ~ f'true'; aggregate: n }`).toLog( refused('matched with a filter expression') ); }); test('as a function argument', () => { - expect(m`run: m -> { group_by: s is concat('x', big) }`).toLog( + expect(m`run: m -> { group_by: s is concat('x', ${'big'}) }`).toLog( refused('as an argument of concat') ); }); test('in a cast, either way', () => { - expect(m`run: m -> { group_by: s is big::string }`).toLog( + expect(m`run: m -> { group_by: s is ${'big::string'} }`).toLog( refused('in a cast') ); - expect(m`run: m -> { where: ai::boolean; aggregate: n }`).toLog( + expect(m`run: m -> { where: ${'ai::boolean'}; aggregate: n }`).toLog( refused('as the type of a cast') ); }); + test('once, at the expression which used it', () => { + expect( + m`run: m -> { group_by: x is ${'pick big when ai > 1 else big'} }` + ).toLog(refused('as the result of a pick')); + }); }); - test('select: * leaves a boolean out, with a warning', () => { - expect(m`run: m -> { select: * }`).toLog( - warningMessage( - "'big' is left out of '*': 'sqlserver' does not support a boolean as a result column; write the value, pick 'yes' when big else 'no'" - ) - ); + describe('select: * leaves a boolean out, with a warning', () => { + test('naming the field, and not a measure', () => { + expect(m`run: m -> { select: * }`).toLog( + warningMessage( + "'big' is left out of '*': 'sqlserver' does not support a boolean as a result column; write the value, pick 'yes' when big else 'no'" + ) + ); + }); + test('naming a joined field by its path', () => { + expect(m`run: m -> { select: o.* }`).toLog( + warningMessage( + "'o.big' is left out of 'o.*': 'sqlserver' does not support a boolean as a result column; write the value, pick 'yes' when o.big else 'no'" + ) + ); + }); + test('and so does index: *', () => { + expect(m`run: m -> { index: * }`).toLog( + warningMessage( + "'big' is left out of '*': 'sqlserver' does not support a boolean as a result column; write the value, pick 'yes' when big else 'no'" + ) + ); + }); }); test('the same boolean is a value on a dialect with booleans', () => { expect(model` source: d is _db_.table('aTable') extend { dimension: big is ai > 5 } - run: d -> { group_by: big, s is big::string, c is concat('x', big); where: big = true } + run: d -> { group_by: big, s is big::string, c is concat('x', big); where: big = true and big > false } + run: d -> big `).toTranslate(); }); }); diff --git a/packages/malloy/src/lang/test/test-translator.ts b/packages/malloy/src/lang/test/test-translator.ts index 55a3579ab..858a9856d 100644 --- a/packages/malloy/src/lang/test/test-translator.ts +++ b/packages/malloy/src/lang/test/test-translator.ts @@ -796,6 +796,10 @@ export function makeModelFunc( const {prefix, wrap, ...ttOptions} = options; return { ...ms, + // A marked location is where the mark sits in the whole source, after + // the prefix. A wrap could put the code anywhere, so marks stay as + // they are. + locations: wrap ? ms.locations : ms.locations.map(l => after(prefix, l)), translator: new TestTranslator( (prefix ?? '') + (wrap ? wrap(ms.code) : ms.code), ttOptions @@ -804,6 +808,21 @@ export function makeModelFunc( }; } +function after(prefix: string | undefined, l: DocumentLocation) { + if (!prefix) return l; + const lines = prefix.split('\n'); + const dLine = lines.length - 1; + const dChar = lines[dLine].length; + const shift = (p: {line: number; character: number}) => ({ + line: p.line + dLine, + character: p.line === 0 ? p.character + dChar : p.character, + }); + return { + ...l, + range: {start: shift(l.range.start), end: shift(l.range.end)}, + }; +} + export function makeExprFunc( model?: ModelDef, sourceName: string = 'ab', From e65c79071c6e758948ab40d4467ffc108bceb1ba Mon Sep 17 00:00:00 2001 From: James Estes Date: Tue, 20 Oct 2026 10:58:38 -0600 Subject: [PATCH 08/15] Null-input tests for the boolean functions run where a boolean is a value A pick cannot tell false from NULL, and starts_with, ends_with, is_inf and is_nan are COALESCEd to false on purpose. The concat of a boolean is its own gated test. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01T1vFteHTo61YyRRVVrJAn4 Signed-off-by: James Estes --- test/src/databases/all/functions.spec.ts | 57 ++++++++++++++++++------ 1 file changed, 44 insertions(+), 13 deletions(-) diff --git a/test/src/databases/all/functions.spec.ts b/test/src/databases/all/functions.spec.ts index d7d99e51f..98a571132 100644 --- a/test/src/databases/all/functions.spec.ts +++ b/test/src/databases/all/functions.spec.ts @@ -122,19 +122,6 @@ expressionModels.forEach((x, databaseName) => { await funcTestMultiple( ["concat('foo', 'bar')", 'foobar'], ["concat(1, 'bar')", '1bar'], - // A dialect without boolean values has no boolean to pass - ...(runtime.dialect.booleanType === 'none' - ? [] - : [ - [ - "concat('cons', true)", - databaseName === 'postgres' - ? 'const' - : databaseName === 'mysql' - ? 'cons1' - : 'construe', - ] as [string, string], - ]), ["concat('foo', @2003)", 'foo2003-01-01'], [ "concat('foo', @2003-01-01 12:00:00)", @@ -145,6 +132,19 @@ expressionModels.forEach((x, databaseName) => { ['concat()', ''] ); }); + it.when(runtime.dialect.booleanType !== 'none')( + `works with a boolean - ${databaseName}`, + async () => { + await funcTest( + "concat('cons', true)", + databaseName === 'postgres' + ? 'const' + : databaseName === 'mysql' + ? 'cons1' + : 'construe' + ); + } + ); }); describe('round', () => { @@ -860,6 +860,13 @@ expressionModels.forEach((x, databaseName) => { ["pick 'yes' when is_inf(null) else 'no'", 'no'] ); }); + // A null input is false, not null + it.when(databaseName !== 'mysql' && runtime.dialect.booleanType !== 'none')( + `is false for null - ${databaseName}`, + async () => { + await funcTest('is_inf(null)', false); + } + ); }); describe('is_nan', () => { it.when(databaseName !== 'mysql')(`works - ${databaseName}`, async () => { @@ -869,6 +876,12 @@ expressionModels.forEach((x, databaseName) => { ["pick 'yes' when is_nan(null) else 'no'", 'no'] ); }); + it.when(databaseName !== 'mysql' && runtime.dialect.booleanType !== 'none')( + `is false for null - ${databaseName}`, + async () => { + await funcTest('is_nan(null)', false); + } + ); }); describe('greatest', () => { it(`works - ${databaseName}`, async () => { @@ -934,6 +947,15 @@ expressionModels.forEach((x, databaseName) => { ["pick 'yes' when starts_with('hello world', null) else 'no'", 'no'] ); }); + it.when(runtime.dialect.booleanType !== 'none')( + `is false for null - ${databaseName}`, + async () => { + await funcTestMultiple( + ["starts_with(null, 'world')", false], + ["starts_with('hello world', null)", false] + ); + } + ); }); describe('ends_with', () => { it(`works - ${databaseName}`, async () => { @@ -944,6 +966,15 @@ expressionModels.forEach((x, databaseName) => { ["pick 'yes' when ends_with('hello world', null) else 'no'", 'no'] ); }); + it.when(runtime.dialect.booleanType !== 'none')( + `is false for null - ${databaseName}`, + async () => { + await funcTestMultiple( + ["ends_with(null, 'world')", false], + ["ends_with('hello world', null)", false] + ); + } + ); }); describe('trim', () => { it(`trim works - ${databaseName}`, async () => { From f5bc82be902c86cc51dfd0c6f757937a974b841a Mon Sep 17 00:00:00 2001 From: James Estes Date: Tue, 20 Oct 2026 10:58:39 -0600 Subject: [PATCH 09/15] The design doc lists any boolean comparison; the context docs point at it Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01T1vFteHTo61YyRRVVrJAn4 Signed-off-by: James Estes --- packages/malloy/src/doc/boolean-none.md | 3 ++- packages/malloy/src/lang/test/CONTEXT.md | 4 +++- packages/malloy/src/model/CONTEXT.md | 12 +++++------- 3 files changed, 10 insertions(+), 9 deletions(-) diff --git a/packages/malloy/src/doc/boolean-none.md b/packages/malloy/src/doc/boolean-none.md index 20c4f4e05..080d7e284 100644 --- a/packages/malloy/src/doc/boolean-none.md +++ b/packages/malloy/src/doc/boolean-none.md @@ -43,7 +43,8 @@ is compiled for a `'none'` dialect. - A boolean field in a query's output: `select:`, `group_by:`, `aggregate:`, and any other place a field becomes a result column. - `is null` and `is not null` with a boolean operand. -- `=` and `!=` with two boolean operands. +- Any comparison (`=`, `!=`, `<`, `<=`, `>`, `>=`) between two booleans, + written plainly, as a partial (`is_small ? > false`) or in an alternation. - `??` with a boolean operand. - A `pick` or `case` whose result is a boolean. - A boolean filter expression (`x ~ f'true'`). diff --git a/packages/malloy/src/lang/test/CONTEXT.md b/packages/malloy/src/lang/test/CONTEXT.md index 75d9b08cb..8a3cb77b8 100644 --- a/packages/malloy/src/lang/test/CONTEXT.md +++ b/packages/malloy/src/lang/test/CONTEXT.md @@ -14,18 +14,20 @@ The tests use a mock schema system that provides pre-defined tables and sources, `TestTranslator` extends `MalloyTranslator` with a mock schema and pre-defined model. It provides: -- **Four connections:** +- **Five connections:** - `_db_` - DuckDB dialect, supports `timestamptz` - `_db2_` - DuckDB dialect, a second connection on the same dialect as `_db_`, so a test can write a cross-connection reference which is not also a cross-dialect one - `_bq_` - BigQuery/StandardSQL dialect - `_pg_` - Postgres dialect, the only dialect with `hasFinalStage` + - `_mssql_` - SQL Server dialect, the only dialect with `booleanType: 'none'` - **Mock tables** (accessible via `connection.table('path')`): - `_db_.table('aTable')` - base table with all field types - `_db2_.table('aTable')` - same table, on the second connection - `_bq_.table('aTable')` - BigQuery version - `_pg_.table('aTable')` - Postgres version + - `_mssql_.table('aTable')` - SQL Server version, without the boolean column - `_db_.table('malloytest.carriers')` - `_db_.table('malloytest.flights')` - `_db_.table('malloytest.airports')` diff --git a/packages/malloy/src/model/CONTEXT.md b/packages/malloy/src/model/CONTEXT.md index 180eb8eae..c0c83ed22 100644 --- a/packages/malloy/src/model/CONTEXT.md +++ b/packages/malloy/src/model/CONTEXT.md @@ -447,16 +447,14 @@ Compiles IR expression trees into SQL expressions. - Function calls - Field references and path expressions -**A boolean on a dialect without boolean values.** SQL has two places for a boolean: a -*condition* (`WHERE`, `ON`, `CASE WHEN`, the operands of `AND`/`OR`/`NOT`) and a *value* -(everywhere else). On a dialect with `booleanType: 'none'` (SQL Server) a boolean is a condition -and never a value: a declared boolean compiles to its comparison, `where: is_late` writes the +**A boolean on a dialect without boolean values.** On a dialect with `booleanType: 'none'` +(SQL Server) a boolean is a condition and never a value: a declared boolean compiles to its comparison, and nothing converts a condition into a `1`/`0`. Malloy's NULL rules for `not`, `!=` and `!~` (true where SQL says NULL) go through `Dialect.sqlNot` / `sqlNullIsTrue`, which keep the `COALESCE` spelling on every dialect with booleans and write the test as a condition -on one without. A boolean where SQL wants a value (a result column, a function argument, `??`, a null -test, `=` between booleans, a cast, the result of a `pick`) is refused by the translator -(`lang/ast/expressions/boolean-value.ts`); the compiler does not look for it. +on one without. The translator refuses a boolean anywhere SQL would want a value, so the +compiler does not look for one; the design and the list of refused uses are in +[`doc/boolean-none.md`](../doc/boolean-none.md). ### 3. Dialect System From 4b57fc9da02417e9e66ffb4d9665af581d5f92e5 Mon Sep 17 00:00:00 2001 From: James Estes Date: Tue, 20 Oct 2026 11:09:30 -0600 Subject: [PATCH 10/15] The boolean function tests read a result boolean the dialect's way MySQL hands a boolean back as 0 or 1, so the null-input tests compare through resultBoolean as the tests they replace did, and the concat of a boolean keeps the trino and presto gate the inline case had. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01T1vFteHTo61YyRRVVrJAn4 Signed-off-by: James Estes --- test/src/databases/all/functions.spec.ts | 45 ++++++++++++++---------- 1 file changed, 26 insertions(+), 19 deletions(-) diff --git a/test/src/databases/all/functions.spec.ts b/test/src/databases/all/functions.spec.ts index 98a571132..2e038d4b8 100644 --- a/test/src/databases/all/functions.spec.ts +++ b/test/src/databases/all/functions.spec.ts @@ -132,19 +132,20 @@ expressionModels.forEach((x, databaseName) => { ['concat()', ''] ); }); - it.when(runtime.dialect.booleanType !== 'none')( - `works with a boolean - ${databaseName}`, - async () => { - await funcTest( - "concat('cons', true)", - databaseName === 'postgres' - ? 'const' - : databaseName === 'mysql' - ? 'cons1' - : 'construe' - ); - } - ); + it.when( + runtime.dialect.booleanType !== 'none' && + !brokenIn('trino', databaseName) && + !brokenIn('presto', databaseName) + )(`works with a boolean - ${databaseName}`, async () => { + await funcTest( + "concat('cons', true)", + databaseName === 'postgres' + ? 'const' + : databaseName === 'mysql' + ? 'cons1' + : 'construe' + ); + }); }); describe('round', () => { @@ -864,7 +865,7 @@ expressionModels.forEach((x, databaseName) => { it.when(databaseName !== 'mysql' && runtime.dialect.booleanType !== 'none')( `is false for null - ${databaseName}`, async () => { - await funcTest('is_inf(null)', false); + await funcTest('is_inf(null)', runtime.dialect.resultBoolean(false)); } ); }); @@ -879,7 +880,7 @@ expressionModels.forEach((x, databaseName) => { it.when(databaseName !== 'mysql' && runtime.dialect.booleanType !== 'none')( `is false for null - ${databaseName}`, async () => { - await funcTest('is_nan(null)', false); + await funcTest('is_nan(null)', runtime.dialect.resultBoolean(false)); } ); }); @@ -951,8 +952,11 @@ expressionModels.forEach((x, databaseName) => { `is false for null - ${databaseName}`, async () => { await funcTestMultiple( - ["starts_with(null, 'world')", false], - ["starts_with('hello world', null)", false] + ["starts_with(null, 'world')", runtime.dialect.resultBoolean(false)], + [ + "starts_with('hello world', null)", + runtime.dialect.resultBoolean(false), + ] ); } ); @@ -970,8 +974,11 @@ expressionModels.forEach((x, databaseName) => { `is false for null - ${databaseName}`, async () => { await funcTestMultiple( - ["ends_with(null, 'world')", false], - ["ends_with('hello world', null)", false] + ["ends_with(null, 'world')", runtime.dialect.resultBoolean(false)], + [ + "ends_with('hello world', null)", + runtime.dialect.resultBoolean(false), + ] ); } ); From c2c7b3f1ade208d864d35bf2f44dbf44c824fd88 Mon Sep 17 00:00:00 2001 From: James Estes Date: Tue, 20 Oct 2026 12:20:23 -0600 Subject: [PATCH 11/15] A boolean compared with anything is refused, and so is one passed to a raw function Any comparison with a boolean operand, including a boolean among the choices of an in-list, is refused at the boolean side. The raw function path checks its arguments as the overloaded path does. Unary minus logs its type error, so a filter on it no longer goes quiet. The hints live in one module, and the refusal tests check the error code and where it landed. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01T1vFteHTo61YyRRVVrJAn4 Signed-off-by: James Estes --- .../src/lang/ast/expressions/boolean-value.ts | 11 +++ .../ast/expressions/expr-alternation-tree.ts | 25 +++--- .../src/lang/ast/expressions/expr-cast.ts | 10 +-- .../src/lang/ast/expressions/expr-func.ts | 14 +++- .../src/lang/ast/expressions/expr-minus.ts | 6 +- .../lang/ast/field-space/index-field-space.ts | 5 +- .../src/lang/ast/field-space/query-spaces.ts | 5 +- .../lang/ast/query-items/field-declaration.ts | 7 +- .../lang/ast/query-items/field-references.ts | 7 +- .../src/lang/ast/types/expression-def.ts | 27 +++---- .../lang/ast/view-elements/reference-view.ts | 7 +- packages/malloy/src/lang/parse-log.ts | 9 ++- .../malloy/src/lang/test/boolean-none.spec.ts | 77 +++++++++++-------- .../malloy/src/lang/test/expressions.spec.ts | 5 ++ 14 files changed, 136 insertions(+), 79 deletions(-) diff --git a/packages/malloy/src/lang/ast/expressions/boolean-value.ts b/packages/malloy/src/lang/ast/expressions/boolean-value.ts index d6d6e6f7f..eba802438 100644 --- a/packages/malloy/src/lang/ast/expressions/boolean-value.ts +++ b/packages/malloy/src/lang/ast/expressions/boolean-value.ts @@ -37,6 +37,17 @@ export function booleanValueError( export const CONDITION_HINT = 'test the condition itself; unknown already tests as false'; +/** The hint for a boolean compared with anything */ +export const COMPARE_HINT = 'test the condition itself, or its not'; + +/** The hint where a number would do: a function argument, a cast */ +export const NUMBER_HINT = 'write the value, pick 1 when … else 0'; + +/** The hint for a boolean as a result column, from the source text it came from */ +export function resultColumnHint(src: string): string { + return `write the value, pick 'yes' when ${src} else 'no'`; +} + /** The hint for a `pick` or `case` whose result is a boolean */ export const CHOICE_HINT = "write values, pick 'yes' when … else 'no', or the condition itself with and, or and not"; diff --git a/packages/malloy/src/lang/ast/expressions/expr-alternation-tree.ts b/packages/malloy/src/lang/ast/expressions/expr-alternation-tree.ts index 569b772f8..a38eb7f1c 100644 --- a/packages/malloy/src/lang/ast/expressions/expr-alternation-tree.ts +++ b/packages/malloy/src/lang/ast/expressions/expr-alternation-tree.ts @@ -9,7 +9,7 @@ import type {FieldSpace} from '../types/field-space'; import {ATNodeType, ExpressionDef} from '../types/expression-def'; import type {BinaryMalloyOperator} from '../types/binary_operators'; import {isEquality} from '../types/binary_operators'; -import {booleanValueError} from './boolean-value'; +import {booleanValueError, COMPARE_HINT} from './boolean-value'; /** * Return a flattened version of an alternation tree, if the tree is @@ -71,14 +71,21 @@ export class ExprAlternationTree extends ExpressionDef { if (inList.length > 0 && (applyOp === '=' || applyOp === '!=')) { const isIn = expr.getExpression(fs); const values = inList.map(v => v.getExpression(fs)); - const noIn = booleanValueError( - fs, - isIn, - `compared with ${applyOp}`, - expr, - 'test the condition itself, or its not' - ); - if (noIn) return noIn; + // A boolean on either side of the comparison, the tested value or + // any of the choices, is a boolean compared with something. + for (const [el, val] of [ + [expr, isIn] as const, + ...inList.map((el, i) => [el, values[i]] as const), + ]) { + const noIn = booleanValueError( + fs, + val, + `compared with ${applyOp}`, + el, + COMPARE_HINT + ); + if (noIn) return noIn; + } return computedExprValue({ dataType: {type: 'boolean'}, value: { diff --git a/packages/malloy/src/lang/ast/expressions/expr-cast.ts b/packages/malloy/src/lang/ast/expressions/expr-cast.ts index c4bcd6f2a..9b037df19 100644 --- a/packages/malloy/src/lang/ast/expressions/expr-cast.ts +++ b/packages/malloy/src/lang/ast/expressions/expr-cast.ts @@ -9,7 +9,7 @@ import type {ExprValue} from '../types/expr-value'; import {computedExprValue} from '../types/expr-value'; import {ExpressionDef} from '../types/expression-def'; import type {FieldSpace} from '../types/field-space'; -import {booleanValueError} from './boolean-value'; +import {booleanValueError, NUMBER_HINT} from './boolean-value'; export class ExprCast extends ExpressionDef { elementType = 'cast'; @@ -48,13 +48,7 @@ export class ExprCast extends ExpressionDef { } } const noCast = - booleanValueError( - fs, - expr, - 'in a cast', - this, - 'write the value, pick 1 when … else 0' - ) || + booleanValueError(fs, expr, 'in a cast', this, NUMBER_HINT) || booleanValueError( fs, dataType, diff --git a/packages/malloy/src/lang/ast/expressions/expr-func.ts b/packages/malloy/src/lang/ast/expressions/expr-func.ts index 26e500373..811d62972 100644 --- a/packages/malloy/src/lang/ast/expressions/expr-func.ts +++ b/packages/malloy/src/lang/ast/expressions/expr-func.ts @@ -60,7 +60,7 @@ import {composeSQLExpr} from '../../../model/utils'; import * as TDU from '../typedesc-utils'; import {mergeRefSummaries} from '../../composite-source-utils'; import type {AnyMessageCodeAndParameters} from '../../parse-log'; -import {booleanValueError} from './boolean-value'; +import {booleanValueError, NUMBER_HINT} from './boolean-value'; // Built-in functions that take a string literal of user-supplied SQL // and emit it directly. Gated by experimental.sql_functions in plain @@ -155,6 +155,16 @@ export class ExprFunc extends ExpressionDef { ): ExprValue { const argExprsWithoutImplicit = this.args.map(arg => arg.getExpression(fs)); if (this.isRaw) { + for (const [i, argExpr] of argExprsWithoutImplicit.entries()) { + const noArg = booleanValueError( + fs, + argExpr, + `as an argument of ${this.name}`, + this.args[i], + NUMBER_HINT + ); + if (noArg) return noArg; + } const funcCall: SQLExprElement[] = [`${this.name}(`]; argExprsWithoutImplicit.forEach((expr, i) => { if (i !== 0) { @@ -244,7 +254,7 @@ export class ExprFunc extends ExpressionDef { argExpr, `as an argument of ${this.name}`, this.args[i - (implicitExpr ? 1 : 0)] ?? this, - 'write the value, pick 1 when … else 0' + NUMBER_HINT ); if (noArg) return noArg; } diff --git a/packages/malloy/src/lang/ast/expressions/expr-minus.ts b/packages/malloy/src/lang/ast/expressions/expr-minus.ts index dcc6c9f8c..4df3e0f1b 100644 --- a/packages/malloy/src/lang/ast/expressions/expr-minus.ts +++ b/packages/malloy/src/lang/ast/expressions/expr-minus.ts @@ -3,7 +3,6 @@ * SPDX-License-Identifier: MIT */ -import {errorFor} from '../ast-utils'; import * as TDU from '../typedesc-utils'; import type {ExprValue} from '../types/expr-value'; import {ExpressionDef} from '../types/expression-def'; @@ -26,6 +25,9 @@ export class ExprMinus extends ExpressionDef { value: {node: 'unary-', e: expr.value}, }; } - return errorFor('negate requires number'); + return this.expr.loggedErrorExpr( + 'expression-type-error', + `'${this.elementType}' Can't use type ${TDU.inspect(expr)}` + ); } } diff --git a/packages/malloy/src/lang/ast/field-space/index-field-space.ts b/packages/malloy/src/lang/ast/field-space/index-field-space.ts index 3af2cee09..762832936 100644 --- a/packages/malloy/src/lang/ast/field-space/index-field-space.ts +++ b/packages/malloy/src/lang/ast/field-space/index-field-space.ts @@ -13,6 +13,7 @@ import type { } from '../../../model/malloy_types'; import {expressionIsScalar, TD} from '../../../model/malloy_types'; import {ErrorFactory} from '../error-factory'; +import {resultColumnHint} from '../expressions/boolean-value'; import { FieldReference, IndexFieldReference, @@ -130,10 +131,12 @@ export class IndexFieldSpace extends QueryOperationSpace { (dialect === undefined || !dialect.ignoreInProject(name)) ) { if (eTypeDesc.type === 'boolean' && dialect?.booleanType === 'none') { + const path = [...joinPath, name].join('.'); wild.logWarning('wildcard-omits-boolean', { dialect: dialect.name, wildcard: wild.refString, - name: [...joinPath, name].join('.'), + name: path, + hint: resultColumnHint(path), }); continue; } diff --git a/packages/malloy/src/lang/ast/field-space/query-spaces.ts b/packages/malloy/src/lang/ast/field-space/query-spaces.ts index 2b4228c3c..82aafaf26 100644 --- a/packages/malloy/src/lang/ast/field-space/query-spaces.ts +++ b/packages/malloy/src/lang/ast/field-space/query-spaces.ts @@ -26,6 +26,7 @@ import type { } from '../../parse-log'; import {mergeRefSummaries} from '../../composite-source-utils'; import {ErrorFactory} from '../error-factory'; +import {resultColumnHint} from '../expressions/boolean-value'; import {ReferenceField} from './reference-field'; import {RefineFromSpaceField} from './refine-from-space-field'; @@ -213,10 +214,12 @@ export abstract class QueryOperationSpace (dialect === undefined || !dialect.ignoreInProject(name)) ) { if (eType.type === 'boolean' && dialect?.booleanType === 'none') { + const path = joinPath.concat(name).join('.'); wild.logWarning('wildcard-omits-boolean', { dialect: dialect.name, wildcard: wild.refString, - name: joinPath.concat(name).join('.'), + name: path, + hint: resultColumnHint(path), }); continue; } diff --git a/packages/malloy/src/lang/ast/query-items/field-declaration.ts b/packages/malloy/src/lang/ast/query-items/field-declaration.ts index de7c19f18..6d3315f7e 100644 --- a/packages/malloy/src/lang/ast/query-items/field-declaration.ts +++ b/packages/malloy/src/lang/ast/query-items/field-declaration.ts @@ -36,7 +36,10 @@ import type {Noteable} from '../types/noteable'; import type {DynamicSpace} from '../field-space/dynamic-space'; import {SpaceField} from '../types/space-field'; import {DefSpace} from '../field-space/def-space'; -import {booleanValueError} from '../expressions/boolean-value'; +import { + booleanValueError, + resultColumnHint, +} from '../expressions/boolean-value'; export type FieldDeclarationConstructor = new ( expr: ExpressionDef, @@ -134,7 +137,7 @@ export abstract class AtomicFieldDeclaration exprValue, 'as a result column', this.expr, - `write the value, pick 'yes' when ${this.exprSrc ?? '…'} else 'no'` + resultColumnHint(this.exprSrc ?? '…') ) ) { return {name: exprName, type: 'error'}; diff --git a/packages/malloy/src/lang/ast/query-items/field-references.ts b/packages/malloy/src/lang/ast/query-items/field-references.ts index 86120255b..be64a5402 100644 --- a/packages/malloy/src/lang/ast/query-items/field-references.ts +++ b/packages/malloy/src/lang/ast/query-items/field-references.ts @@ -20,7 +20,10 @@ import {ListOf, MalloyElement} from '../types/malloy-element'; import type {Noteable} from '../types/noteable'; import type {MakeEntry} from '../types/space-entry'; import {ExprIdReference} from '../expressions/expr-id-reference'; -import {booleanValueError} from '../expressions/boolean-value'; +import { + booleanValueError, + resultColumnHint, +} from '../expressions/boolean-value'; import type {FieldDeclarationConstructor} from './field-declaration'; import { AggregateFieldDeclaration, @@ -193,7 +196,7 @@ export abstract class FieldReference actualType, 'as a result column', this, - `write the value, pick 'yes' when ${this.refString} else 'no'` + resultColumnHint(this.refString) ); } } diff --git a/packages/malloy/src/lang/ast/types/expression-def.ts b/packages/malloy/src/lang/ast/types/expression-def.ts index bed24e50e..ca51a54ed 100644 --- a/packages/malloy/src/lang/ast/types/expression-def.ts +++ b/packages/malloy/src/lang/ast/types/expression-def.ts @@ -44,7 +44,7 @@ import { TemporalFilterExpression, isFilterable, } from '@malloydata/malloy-filter'; -import {booleanValueError} from '../expressions/boolean-value'; +import {booleanValueError, COMPARE_HINT} from '../expressions/boolean-value'; class TypeMismatch extends Error {} @@ -421,27 +421,20 @@ function regexEqual(left: ExprValue, right: ExprValue): Expr | undefined { } /** - * Two booleans can only be compared on a dialect with boolean values. Every - * comparison operator, applied plainly, as a partial, or through an - * alternation, arrives here through equality() or compare(). + * A boolean can only be compared, with anything, on a dialect with boolean + * values. Every comparison operator, applied plainly, as a partial, or + * through an alternation, arrives here through equality() or compare(). */ function booleanCompare( fs: FieldSpace, left: ExpressionDef, lhs: ExprValue, op: CompareMalloyOperator, + right: ExpressionDef, rhs: ExprValue ): ExprValue | undefined { - if (lhs.type !== 'boolean' || rhs.type !== 'boolean') { - return undefined; - } - return booleanValueError( - fs, - lhs, - `compared with ${op}`, - left, - 'test the condition itself, or its not' - ); + const [side, val] = lhs.type === 'boolean' ? [left, lhs] : [right, rhs]; + return booleanValueError(fs, val, `compared with ${op}`, side, COMPARE_HINT); } function equality( @@ -455,7 +448,8 @@ function equality( const node = getExprNode(op); const err = - errorCascade('boolean', lhs, rhs) || booleanCompare(fs, left, lhs, op, rhs); + errorCascade('boolean', lhs, rhs) || + booleanCompare(fs, left, lhs, op, right, rhs); if (err) return err; // Unsupported types can be compare with null @@ -554,7 +548,8 @@ function compare( const rhs = right.getExpression(fs); const err = - errorCascade('boolean', lhs, rhs) || booleanCompare(fs, left, lhs, op, rhs); + errorCascade('boolean', lhs, rhs) || + booleanCompare(fs, left, lhs, op, right, rhs); if (err) return err; const noCompare = unsupportError(left, lhs, right, rhs); diff --git a/packages/malloy/src/lang/ast/view-elements/reference-view.ts b/packages/malloy/src/lang/ast/view-elements/reference-view.ts index 1bdd68686..8372b42fa 100644 --- a/packages/malloy/src/lang/ast/view-elements/reference-view.ts +++ b/packages/malloy/src/lang/ast/view-elements/reference-view.ts @@ -15,7 +15,10 @@ import { sourceBase, } from '../../../model/malloy_types'; import {ErrorFactory} from '../error-factory'; -import {booleanValueError} from '../expressions/boolean-value'; +import { + booleanValueError, + resultColumnHint, +} from '../expressions/boolean-value'; import type {QueryOperationSpace} from '../field-space/query-spaces'; import type {ViewOrScalarFieldReference} from '../query-items/field-references'; import {attachDrillPaths} from '../query-properties/drill'; @@ -81,7 +84,7 @@ export class ReferenceView extends View { fieldDef, 'as a result column', this.reference, - `write the value, pick 'yes' when ${this.reference.refString} else 'no'` + resultColumnHint(this.reference.refString) ) ) { return oops(); diff --git a/packages/malloy/src/lang/parse-log.ts b/packages/malloy/src/lang/parse-log.ts index 1def9a04a..e24fbc22e 100644 --- a/packages/malloy/src/lang/parse-log.ts +++ b/packages/malloy/src/lang/parse-log.ts @@ -95,7 +95,12 @@ type MessageParameterTypes = { 'experimental-dialect-not-enabled': {dialect: string}; 'nesting-unsupported': {dialect: string}; 'boolean-value-unsupported': {dialect: string; where: string; hint: string}; - 'wildcard-omits-boolean': {dialect: string; wildcard: string; name: string}; + 'wildcard-omits-boolean': { + dialect: string; + wildcard: string; + name: string; + hint: string; + }; 'nested-multi-stage-unsupported': {dialect: string}; 'nested-projection-limit-unsupported': {dialect: string}; 'sql-native-not-allowed-in-expression': { @@ -471,7 +476,7 @@ export const MESSAGE_FORMATTERS: PartialErrorCodeMessageMap = { 'boolean-value-unsupported': e => `'${e.dialect}' does not support a boolean ${e.where}; ${e.hint}`, 'wildcard-omits-boolean': e => - `'${e.name}' is left out of '${e.wildcard}': '${e.dialect}' does not support a boolean as a result column; write the value, pick 'yes' when ${e.name} else 'no'`, + `'${e.name}' is left out of '${e.wildcard}': '${e.dialect}' does not support a boolean as a result column; ${e.hint}`, 'nested-multi-stage-unsupported': e => `'${e.dialect}' does not support a multi-stage pipeline ('->') in a nested query`, 'nested-projection-limit-unsupported': e => diff --git a/packages/malloy/src/lang/test/boolean-none.spec.ts b/packages/malloy/src/lang/test/boolean-none.spec.ts index 5c807130e..a59875f2a 100644 --- a/packages/malloy/src/lang/test/boolean-none.spec.ts +++ b/packages/malloy/src/lang/test/boolean-none.spec.ts @@ -5,6 +5,7 @@ import './parse-expects'; import { + error, errorMessage, makeModelFunc, model, @@ -27,8 +28,9 @@ source: m is _mssql_.table('aTable') extend { `, }); -const refused = (where: string) => - errorMessage(new RegExp(`^'sqlserver' does not support a boolean ${where};`)); +// A refusal is checked by its code, and by the `${…}` mark saying where it +// landed; the hint's wording is checked only where the hint is the point. +const refused = error('boolean-value-unsupported'); describe('a boolean on a dialect without boolean values', () => { describe('is a condition', () => { @@ -80,93 +82,104 @@ describe('a boolean on a dialect without boolean values', () => { ); }); test('a boolean aggregate', () => { - expect(m`run: m -> { aggregate: any_big is ${'n > 1'} }`).toLog( - refused('as a result column') - ); + expect(m`run: m -> { aggregate: any_big is ${'n > 1'} }`).toLog(refused); }); test('in index:', () => { - expect(m`run: m -> { index: ${'big'} }`).toLog( - refused('as a result column') - ); + expect(m`run: m -> { index: ${'big'} }`).toLog(refused); }); test('a boolean field named as the whole view', () => { - expect(m`run: m -> ${'big'}`).toLog(refused('as a result column')); - expect(m`run: m -> ${'anyn'}`).toLog(refused('as a result column')); - expect(m`run: m -> v + ${'big'}`).toLog(refused('as a result column')); + expect(m`run: m -> ${'big'}`).toLog(refused); + expect(m`run: m -> ${'anyn'}`).toLog(refused); + expect(m`run: m -> v + ${'big'}`).toLog(refused); }); test('in a null test', () => { expect(m`run: m -> { where: ${'big'} is null; aggregate: n }`).toLog( - errorMessage( - "'sqlserver' does not support a boolean in a null test; test the condition itself; unknown already tests as false" - ) + refused ); }); test('compared with = or !=', () => { expect(m`run: m -> { where: ${'big'} = true; aggregate: n }`).toLog( - refused('compared with =') + refused ); expect(m`run: m -> { where: ${'big'} != (ai > 6); aggregate: n }`).toLog( - refused('compared with !=') + refused + ); + }); + test('compared with something which is not a boolean', () => { + expect(m`run: m -> { where: ${'big'} = 1; aggregate: n }`).toLog(refused); + expect(m`run: m -> { where: ai = ${'big'}; aggregate: n }`).toLog( + refused + ); + expect(m`run: m -> { where: null = ${'big'}; aggregate: n }`).toLog( + refused + ); + expect(m`run: m -> { where: ${'big'} ? > 1; aggregate: n }`).toLog( + refused + ); + expect(m`run: m -> { where: astr != ${'big'}; aggregate: n }`).toLog( + refused ); }); test('compared with an ordering operator', () => { expect(m`run: m -> { where: ${'big'} > false; aggregate: n }`).toLog( - refused('compared with >') + refused ); expect(m`run: m -> { where: ${'big'} ? > false; aggregate: n }`).toLog( - refused('compared with >') + refused ); expect( m`run: m -> { where: ${'big'} ? > false | < true; aggregate: n }` - ).toLog(refused('compared with >'), refused('compared with <')); + ).toLog(refused, refused); }); test('in an alternation', () => { expect( m`run: m -> { where: ${'big'} ? true | false; aggregate: n }` - ).toLog(refused('compared with =')); + ).toLog(refused); + expect(m`run: m -> { where: ai ? 1 | ${'big'}; aggregate: n }`).toLog( + refused + ); }); test('in ??', () => { expect(m`run: m -> { where: ${'big ?? false'}; aggregate: n }`).toLog( - errorMessage( - "'sqlserver' does not support a boolean in ??; test the condition itself; unknown already tests as false" - ) + refused ); }); test('as the result of a pick', () => { expect( m`run: m -> { where: ${'pick true when ai > 1 else false'}; aggregate: n }` - ).toLog(refused('as the result of a pick')); + ).toLog(refused); }); test('as the result of a case', () => { expect( m`run: m -> { where: case when ai > 1 then true else false end; aggregate: n }` ).toLog( warningMessage('Use a `pick` statement instead of `case`'), - refused('as the result of a case') + refused ); }); test('matched with a filter expression', () => { expect(m`run: m -> { where: ${'big'} ~ f'true'; aggregate: n }`).toLog( - refused('matched with a filter expression') + refused ); }); test('as a function argument', () => { expect(m`run: m -> { group_by: s is concat('x', ${'big'}) }`).toLog( - refused('as an argument of concat') + refused + ); + expect(m`run: m -> { group_by: s is foo!number(${'big'}) }`).toLog( + refused ); }); test('in a cast, either way', () => { - expect(m`run: m -> { group_by: s is ${'big::string'} }`).toLog( - refused('in a cast') - ); + expect(m`run: m -> { group_by: s is ${'big::string'} }`).toLog(refused); expect(m`run: m -> { where: ${'ai::boolean'}; aggregate: n }`).toLog( - refused('as the type of a cast') + refused ); }); test('once, at the expression which used it', () => { expect( m`run: m -> { group_by: x is ${'pick big when ai > 1 else big'} }` - ).toLog(refused('as the result of a pick')); + ).toLog(refused); }); }); diff --git a/packages/malloy/src/lang/test/expressions.spec.ts b/packages/malloy/src/lang/test/expressions.spec.ts index b74dabb51..6b985b785 100644 --- a/packages/malloy/src/lang/test/expressions.spec.ts +++ b/packages/malloy/src/lang/test/expressions.spec.ts @@ -126,6 +126,11 @@ describe('expressions', () => { test('unary negation', () => { expect('- ai').compilesTo('{unary- ai}'); }); + test('unary negation of a non-number is an error', () => { + expect(expr`- ${'astr'}`).toLog( + errorMessage("'unary minus' Can't use type string") + ); + }); test('equal', () => { expect('42 = 7').compilesTo('{42 = 7}'); }); From f653a3632b4d6703ec1493c8a40bf320c8a74b0b Mon Sep 17 00:00:00 2001 From: James Estes Date: Tue, 20 Oct 2026 12:20:23 -0600 Subject: [PATCH 12/15] The design doc's comparison bullet covers any boolean operand Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01T1vFteHTo61YyRRVVrJAn4 Signed-off-by: James Estes --- packages/malloy/src/doc/boolean-none.md | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/packages/malloy/src/doc/boolean-none.md b/packages/malloy/src/doc/boolean-none.md index 080d7e284..0ab9589ed 100644 --- a/packages/malloy/src/doc/boolean-none.md +++ b/packages/malloy/src/doc/boolean-none.md @@ -43,17 +43,16 @@ is compiled for a `'none'` dialect. - A boolean field in a query's output: `select:`, `group_by:`, `aggregate:`, and any other place a field becomes a result column. - `is null` and `is not null` with a boolean operand. -- Any comparison (`=`, `!=`, `<`, `<=`, `>`, `>=`) between two booleans, - written plainly, as a partial (`is_small ? > false`) or in an alternation. +- Any comparison (`=`, `!=`, `<`, `<=`, `>`, `>=`) with a boolean operand + (`is_small = 1`, `size > is_small`, `is_small ? > false`), written plainly, + as a partial or in an alternation. - `??` with a boolean operand. - A `pick` or `case` whose result is a boolean. - A boolean filter expression (`x ~ f'true'`). - A boolean as a function argument. - A cast to or from boolean (`x::boolean`, `is_small::string`). -- An alternation with a boolean on the left (`is_small ? true | false`). -The error suggests making the value explicit with a `pick`, as in the example -above. +Each error says what to write instead. ## `select: *` From 80ff7df349f5416bc91c536e5294d2a1a40ea1b2 Mon Sep 17 00:00:00 2001 From: James Estes Date: Tue, 20 Oct 2026 18:11:39 -0600 Subject: [PATCH 13/15] A unary minus of an error is that error ExprMinus rebuilt an error-typed operand as a number, so whatever used the result complained again; it now returns the operand unchanged, and the error-type-inference table says so. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01HXm9dCuXFseFX4FS2DK1My Signed-off-by: James Estes --- packages/malloy/src/lang/ast/expressions/expr-minus.ts | 4 ++++ packages/malloy/src/lang/test/boolean-none.spec.ts | 5 +++++ packages/malloy/src/lang/test/expressions.spec.ts | 5 +++++ packages/malloy/src/lang/test/parse.spec.ts | 2 +- 4 files changed, 15 insertions(+), 1 deletion(-) diff --git a/packages/malloy/src/lang/ast/expressions/expr-minus.ts b/packages/malloy/src/lang/ast/expressions/expr-minus.ts index 4df3e0f1b..4a66f866c 100644 --- a/packages/malloy/src/lang/ast/expressions/expr-minus.ts +++ b/packages/malloy/src/lang/ast/expressions/expr-minus.ts @@ -18,6 +18,10 @@ export class ExprMinus extends ExpressionDef { protected computeExpression(fs: FieldSpace): ExprValue { const expr = this.expr.getExpression(fs); + // An error is already logged; it stays an error so nothing else logs it + if (expr.type === 'error') { + return expr; + } if (TDU.typeIn(expr, this.legalChildTypes)) { return { ...expr, diff --git a/packages/malloy/src/lang/test/boolean-none.spec.ts b/packages/malloy/src/lang/test/boolean-none.spec.ts index a59875f2a..01aa9a62a 100644 --- a/packages/malloy/src/lang/test/boolean-none.spec.ts +++ b/packages/malloy/src/lang/test/boolean-none.spec.ts @@ -181,6 +181,11 @@ describe('a boolean on a dialect without boolean values', () => { m`run: m -> { group_by: x is ${'pick big when ai > 1 else big'} }` ).toLog(refused); }); + test('once, through a unary minus', () => { + expect(m`run: m -> { where: -(${'big ?? false'}); aggregate: n }`).toLog( + refused + ); + }); }); describe('select: * leaves a boolean out, with a warning', () => { diff --git a/packages/malloy/src/lang/test/expressions.spec.ts b/packages/malloy/src/lang/test/expressions.spec.ts index 6b985b785..5f3da3227 100644 --- a/packages/malloy/src/lang/test/expressions.spec.ts +++ b/packages/malloy/src/lang/test/expressions.spec.ts @@ -131,6 +131,11 @@ describe('expressions', () => { errorMessage("'unary minus' Can't use type string") ); }); + test('unary negation of an error is that error, logged once', () => { + expect( + `run: a -> { where: -(-${'astr'}); aggregate: c is count() }` + ).toLog(errorMessage("'unary minus' Can't use type string")); + }); test('equal', () => { expect('42 = 7').compilesTo('{42 = 7}'); }); diff --git a/packages/malloy/src/lang/test/parse.spec.ts b/packages/malloy/src/lang/test/parse.spec.ts index f417c8572..56edc4917 100644 --- a/packages/malloy/src/lang/test/parse.spec.ts +++ b/packages/malloy/src/lang/test/parse.spec.ts @@ -331,7 +331,7 @@ describe('error cascading', () => { 'err.month': 'date', 'day_of_week(err)': 'number', 'err::string': 'string', - '-err': 'number', + '-err': 'error', 'err * 1': 'number', '1 * err': 'number', 'err / 1': 'number', From 435188e6231e5b0a1a8fb571fc632d7a227f8cde Mon Sep 17 00:00:00 2001 From: James Estes Date: Tue, 20 Oct 2026 18:45:09 -0600 Subject: [PATCH 14/15] A boolean is coerced between condition and value in the compiler On a dialect without boolean values, the expression compiler tracks whether each edge wants a value or a condition and asks the dialect to coerce where they differ: a condition becomes CASE WHEN c THEN 1 WHEN NOT (c) THEN 0 END, a value becomes v = 1. In the boolean_coercion experiment the translator no longer refuses a boolean in a value position. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01HXm9dCuXFseFX4FS2DK1My Signed-off-by: James Estes --- packages/malloy/src/dialect/dialect.ts | 21 ++ .../src/lang/ast/expressions/boolean-value.ts | 5 + .../src/lang/ast/expressions/expr-func.ts | 7 +- .../lang/ast/field-space/index-field-space.ts | 6 +- .../src/lang/ast/field-space/query-spaces.ts | 6 +- .../src/lang/test/boolean-coercion.spec.ts | 220 ++++++++++++++++++ .../malloy/src/model/expression_compiler.ts | 168 +++++++++++-- packages/malloy/src/model/field_instance.ts | 10 +- packages/malloy/src/model/query_query.ts | 8 +- .../sqlserver/boolean-coercion.spec.ts | 100 ++++++++ 10 files changed, 522 insertions(+), 29 deletions(-) create mode 100644 packages/malloy/src/lang/test/boolean-coercion.spec.ts create mode 100644 test/src/databases/sqlserver/boolean-coercion.spec.ts diff --git a/packages/malloy/src/dialect/dialect.ts b/packages/malloy/src/dialect/dialect.ts index fdabf070a..8276f13e0 100644 --- a/packages/malloy/src/dialect/dialect.ts +++ b/packages/malloy/src/dialect/dialect.ts @@ -1263,6 +1263,27 @@ export abstract class Dialect { return `COALESCE(${condition},true)`; } + /** + * A condition, where a value is wanted. Without a boolean value the + * condition becomes 1, 0, or NULL when it is unknown. + */ + sqlConditionAsValue(condition: string): string { + if (this.booleanType === 'none') { + return `CASE WHEN ${condition} THEN 1 WHEN NOT (${condition}) THEN 0 END`; + } + return condition; + } + + /** + * A value, where a condition is wanted: the value a condition became. + */ + sqlValueAsCondition(value: string): string { + if (this.booleanType === 'none') { + return `(${value} = 1)`; + } + return value; + } + /** * What a boolean value looks like in a query result */ diff --git a/packages/malloy/src/lang/ast/expressions/boolean-value.ts b/packages/malloy/src/lang/ast/expressions/boolean-value.ts index eba802438..e51cbb112 100644 --- a/packages/malloy/src/lang/ast/expressions/boolean-value.ts +++ b/packages/malloy/src/lang/ast/expressions/boolean-value.ts @@ -26,6 +26,11 @@ export function booleanValueError( if (type?.type !== 'boolean' || dialect?.booleanType !== 'none') { return undefined; } + // The compiler coerces a boolean to a value in the boolean_coercion + // experiment, so no use of one is refused + if (logTo.inExperiment('boolean_coercion', true)) { + return undefined; + } return logTo.loggedErrorExpr('boolean-value-unsupported', { dialect: dialect.name, where, diff --git a/packages/malloy/src/lang/ast/expressions/expr-func.ts b/packages/malloy/src/lang/ast/expressions/expr-func.ts index 811d62972..b18705e2f 100644 --- a/packages/malloy/src/lang/ast/expressions/expr-func.ts +++ b/packages/malloy/src/lang/ast/expressions/expr-func.ts @@ -471,7 +471,12 @@ export class ExprFunc extends ExpressionDef { } } - funcCall = composeSQLExpr(expr); + const sqlExpr = composeSQLExpr(expr); + // The compiler needs to know this raw SQL is a condition + if (func.name === 'sql_boolean') { + sqlExpr.typeDef = {type: 'boolean'}; + } + funcCall = sqlExpr; } } const maxEvalSpace = mergeEvalSpaces(...argExprs.map(e => e.evalSpace)); diff --git a/packages/malloy/src/lang/ast/field-space/index-field-space.ts b/packages/malloy/src/lang/ast/field-space/index-field-space.ts index 762832936..48da8258c 100644 --- a/packages/malloy/src/lang/ast/field-space/index-field-space.ts +++ b/packages/malloy/src/lang/ast/field-space/index-field-space.ts @@ -130,7 +130,11 @@ export class IndexFieldSpace extends QueryOperationSpace { expressionIsScalar(eTypeDesc.expressionType) && (dialect === undefined || !dialect.ignoreInProject(name)) ) { - if (eTypeDesc.type === 'boolean' && dialect?.booleanType === 'none') { + if ( + eTypeDesc.type === 'boolean' && + dialect?.booleanType === 'none' && + !wild.inExperiment('boolean_coercion', true) + ) { const path = [...joinPath, name].join('.'); wild.logWarning('wildcard-omits-boolean', { dialect: dialect.name, diff --git a/packages/malloy/src/lang/ast/field-space/query-spaces.ts b/packages/malloy/src/lang/ast/field-space/query-spaces.ts index 82aafaf26..9631d1c76 100644 --- a/packages/malloy/src/lang/ast/field-space/query-spaces.ts +++ b/packages/malloy/src/lang/ast/field-space/query-spaces.ts @@ -213,7 +213,11 @@ export abstract class QueryOperationSpace model.expressionIsScalar(eType.expressionType) && (dialect === undefined || !dialect.ignoreInProject(name)) ) { - if (eType.type === 'boolean' && dialect?.booleanType === 'none') { + if ( + eType.type === 'boolean' && + dialect?.booleanType === 'none' && + !wild.inExperiment('boolean_coercion', true) + ) { const path = joinPath.concat(name).join('.'); wild.logWarning('wildcard-omits-boolean', { dialect: dialect.name, diff --git a/packages/malloy/src/lang/test/boolean-coercion.spec.ts b/packages/malloy/src/lang/test/boolean-coercion.spec.ts new file mode 100644 index 000000000..68ff9501b --- /dev/null +++ b/packages/malloy/src/lang/test/boolean-coercion.spec.ts @@ -0,0 +1,220 @@ +/* + * Copyright Contributors to the Malloy project + * SPDX-License-Identifier: MIT + */ + +import './parse-expects'; +import {TestTranslator} from './test-translator'; +import {QueryModel} from '../../model'; + +// In the boolean_coercion experiment, a boolean on a dialect with +// booleanType 'none' is still a condition, and the compiler turns it into +// a value (1, 0, or NULL) wherever SQL wants a value, and a value back into +// a condition wherever SQL wants a condition. Nothing is refused. +const prefix = `##! experimental { dialect.sqlserver parameters sql_functions boolean_coercion } +source: m is _mssql_.table('aTable') extend { + dimension: big is ai > 5 + dimension: sb is sql_boolean("""ai > 7""") + measure: n is count() + measure: anyn is n > 1 + join_one: o is _mssql_.table('aTable') extend { dimension: big is ai > 5 } on o.ai = ai +} +source: pk is _mssql_.table('aTable') extend { + dimension: big is ai > 5 + primary_key: big +} +source: pkj is _mssql_.table('aTable') extend { + dimension: big is ai > 5 + join_one: pk on pk.ai = ai +} +source: pm(p::boolean is true) is _mssql_.table('aTable') extend { + dimension: pv is p + measure: n is count() +} +`; + +function sqlOf(query: string): string { + const tt = new TestTranslator(prefix + query); + expect(tt).toTranslate(); + const modelDef = tt.translate().modelDef!; + const ql = modelDef.queryList; + const compiled = new QueryModel(modelDef).compileQuery(ql[ql.length - 1]); + return compiled.sql.replace(/\s+/g, ' '); +} + +const bigCond = '(base."ai">5)'; +// At the root of a field's expression the comparison is not wrapped +const bigValue = + 'CASE WHEN base."ai">5 THEN 1 WHEN NOT (base."ai">5) THEN 0 END'; +// Through a reference to the field, it is +const bigRef = `(${bigValue})`; +const trueValue = 'CASE WHEN (1=1) THEN 1 WHEN NOT ((1=1)) THEN 0 END'; + +describe('boolean coercion on a dialect without boolean values', () => { + describe('a condition where a value is wanted becomes 1, 0 or NULL', () => { + test('as a result column', () => { + expect(sqlOf('run: m -> { group_by: big }')).toContain( + `${bigValue} as "big"` + ); + }); + test('as a selected column', () => { + expect(sqlOf('run: m -> { select: big }')).toContain( + `${bigValue} as "big"` + ); + }); + test('in select: *, with no warning', () => { + expect(sqlOf('run: m -> { select: * }')).toContain( + `${bigValue} as "big"` + ); + }); + test('a literal', () => { + expect(sqlOf('run: m -> { group_by: t is true }')).toContain( + `${trueValue} as "t"` + ); + }); + test('a boolean function', () => { + const sql = sqlOf("run: m -> { group_by: s is starts_with(astr, 'a') }"); + expect(sql).toMatch( + /CASE WHEN LEFT\(.*\) = N'a' THEN 1 WHEN NOT \(LEFT\(.*\) = N'a'\) THEN 0 END as "s"/ + ); + }); + test('a sql_boolean', () => { + expect(sqlOf('run: m -> { group_by: sb }')).toContain( + 'CASE WHEN ai > 7 THEN 1 WHEN NOT (ai > 7) THEN 0 END as "sb"' + ); + }); + test('a boolean parameter', () => { + expect(sqlOf('run: pm -> { group_by: pv }')).toContain( + `${trueValue} as "pv"` + ); + }); + test('compared', () => { + expect(sqlOf('run: m -> { where: big = true; aggregate: n }')).toContain( + `WHERE ${bigRef}=${trueValue}` + ); + }); + test('null tested', () => { + expect(sqlOf('run: m -> { where: big is null; aggregate: n }')).toContain( + `WHERE ${bigRef} IS NULL` + ); + }); + test('as a function argument', () => { + expect(sqlOf('run: m -> { group_by: s is concat(big) }')).toContain( + `CONCAT(${bigRef})` + ); + }); + test('as a pick result', () => { + expect( + sqlOf('run: m -> { group_by: x is pick big when ai > 1 else false }') + ).toContain( + `CASE WHEN (base."ai">1) THEN ${bigRef} ELSE CASE WHEN (1=0) THEN 1 WHEN NOT ((1=0)) THEN 0 END END as "x"` + ); + }); + test('cast', () => { + expect(sqlOf('run: m -> { group_by: x is big::number }')).toContain( + `CAST(${bigRef} AS` + ); + }); + test('in a window function', () => { + expect( + sqlOf('run: m -> { group_by: big; calculate: x is lag(big) }') + ).toContain(`LAG(${bigRef})`); + }); + test('as a primary key, hashed and counted', () => { + expect(sqlOf('run: pkj -> { aggregate: x is pk.count() }')).toContain( + 'COUNT(DISTINCT (CASE WHEN pk_0."ai">5 THEN 1 WHEN NOT (pk_0."ai">5) THEN 0 END))' + ); + expect(sqlOf('run: pkj -> { aggregate: x is pk.ai.sum() }')).toContain( + 'HASHBYTES(\'MD5\', CAST((CASE WHEN pk_0."ai">5 THEN 1 WHEN NOT (pk_0."ai">5) THEN 0 END) AS NVARCHAR(MAX)))' + ); + }); + }); + + describe('a value where a condition is wanted is tested against 1', () => { + test('an ungrouped boolean measure in having:', () => { + expect( + sqlOf('run: m -> { group_by: ai; aggregate: n; having: all(anyn) }') + ).toContain( + 'HAVING (MAX((CASE WHEN (COUNT(1))>1 THEN 1 WHEN NOT ((COUNT(1))>1) THEN 0 END)) OVER () = 1)' + ); + }); + test('a boolean column from an earlier stage', () => { + expect( + sqlOf('run: m -> { group_by: ai, big } -> { where: big; select: ai }') + ).toContain('WHERE (base."big" = 1)'); + }); + test('?? of booleans', () => { + expect( + sqlOf('run: m -> { where: big ?? false; aggregate: n }') + ).toContain( + `WHERE (COALESCE(${bigRef},CASE WHEN (1=0) THEN 1 WHEN NOT ((1=0)) THEN 0 END) = 1)` + ); + }); + }); + + describe('a condition where a condition is wanted is unchanged', () => { + test('where:', () => { + const sql = sqlOf('run: m -> { where: big; aggregate: n }'); + expect(sql).toContain(`WHERE ${bigCond}`); + expect(sql).not.toContain('CASE'); + }); + test('having:', () => { + const sql = sqlOf('run: m -> { aggregate: n; having: anyn }'); + expect(sql).toContain('HAVING ((COUNT(1))>1)'); + expect(sql).not.toContain('CASE'); + }); + test('a join on:', () => { + const sql = sqlOf( + 'source: j is m extend { join_one: q is m on q.ai = ai and big }\nrun: j -> { aggregate: x is q.n }' + ); + expect(sql).toContain('ON (q_0."ai"=base."ai") and (base."ai">5)'); + expect(sql).not.toContain('CASE WHEN (base."ai">5) THEN 1'); + }); + test('a source where:', () => { + const sql = sqlOf( + 'source: s2 is m extend { where: big }\nrun: s2 -> { aggregate: n }' + ); + expect(sql).toContain(`WHERE ${bigCond}`); + expect(sql).not.toContain('CASE'); + }); + test('a filtered aggregate', () => { + const sql = sqlOf('run: m -> { aggregate: x is n { where: big } }'); + expect(sql).toContain(`COUNT(CASE WHEN ${bigCond} THEN 1 END)`); + }); + test('pick … when', () => { + const sql = sqlOf( + "run: m -> { group_by: x is pick 'yes' when big else 'no' }" + ); + expect(sql).toContain(`CASE WHEN ${bigCond} THEN N'yes' ELSE N'no' END`); + }); + test('not, and, or', () => { + const sql = sqlOf( + 'run: m -> { where: not big and (big or ai = 1); aggregate: n }' + ); + expect(sql).toContain( + `WHERE (CASE WHEN ${bigCond} THEN 1 ELSE 0 END = 0) and ((${bigCond} or (base."ai"=1)))` + ); + }); + test('a boolean function as a filter', () => { + const sql = sqlOf( + "run: m -> { where: starts_with(astr, 'a'); aggregate: n }" + ); + expect(sql).toMatch(/WHERE LEFT\(/); + expect(sql).not.toContain('CASE'); + }); + test('a boolean parameter as a filter', () => { + const sql = sqlOf('run: pm -> { where: pv; aggregate: n }'); + expect(sql).toContain('WHERE ((1=1))'); + }); + }); + + test('without the experiment a boolean value is still refused', () => { + const tt = new TestTranslator( + `##! experimental.dialect.sqlserver +source: m is _mssql_.table('aTable') extend { dimension: big is ai > 5 } +run: m -> { group_by: big }` + ); + const problems = tt.translate().problems ?? []; + expect(problems.map(p => p.code)).toContain('boolean-value-unsupported'); + }); +}); diff --git a/packages/malloy/src/model/expression_compiler.ts b/packages/malloy/src/model/expression_compiler.ts index 7e635f306..a8bbda99d 100644 --- a/packages/malloy/src/model/expression_compiler.ts +++ b/packages/malloy/src/model/expression_compiler.ts @@ -116,12 +116,105 @@ function deepCopyExpr(expr: T): T { * Compiles an expression tree by mutating it in-place to set all .sql fields. * Assumes the expression tree is already a copy that can be mutated. */ +/** + * The kind of SQL an expression is: a value, or a condition. A dialect with + * boolean values has one kind; on a dialect without them a condition can + * only stand where SQL wants a condition, and a value only where it wants + * a value, so the compiler tracks which kind each edge wants and which kind + * each node produced, and asks the dialect to coerce where they differ. + */ +export type SQLKind = 'value' | 'condition'; + +/** + * The kind a node wants from the child named `kid`, given the kind the + * node itself was asked for. + */ +function wantFromKid(expr: Expr, kid: string, want: SQLKind): SQLKind { + switch (expr.node) { + case 'and': + case 'or': + case 'not': + case 'filterCondition': + return 'condition'; + case '()': + return want; + case 'case': + if (kid === 'caseWhen') { + return expr.kids.caseValue ? 'value' : 'condition'; + } + return kid === 'caseValue' ? 'value' : want; + case 'filteredExpr': + return kid === 'filterList' ? 'condition' : 'value'; + default: + return 'value'; + } +} + +/** + * The kind a node produced. A node which passes its want through to its + * expression (a field, a parameter, a case) produced what it was asked for. + */ +function kindProduced(expr: Expr, want: SQLKind): SQLKind { + switch (expr.node) { + case '>': + case '<': + case '>=': + case '<=': + case '=': + case '!=': + case 'and': + case 'or': + case 'not': + case 'like': + case '!like': + case 'in': + case 'inGiven': + case 'regexpMatch': + case 'filterMatch': + case 'is-null': + case 'is-not-null': + case 'true': + case 'false': + case 'filterCondition': + return 'condition'; + case 'function_call': + return expr.overload.returnType.type === 'boolean' + ? 'condition' + : 'value'; + case 'genericSQLExpr': + return expr.typeDef?.type === 'boolean' ? 'condition' : 'value'; + case 'field': + case 'parameter': + case 'given': + case '()': + case 'case': + return want; + default: + return 'value'; + } +} + +function asKind( + dialect: Dialect, + sql: string, + produced: SQLKind, + want: SQLKind +): string { + if (produced === want) { + return sql; + } + return produced === 'condition' + ? dialect.sqlConditionAsValue(sql) + : dialect.sqlValueAsCondition(sql); +} + function compileExpr( resultSet: FieldInstanceResult, context: QueryStruct, expr: T, state: GenerateState = new GenerateState(), - wrap = true + wrap = true, + want: SQLKind = 'value' ): T { /* * Translate the children first, and stash the translation @@ -129,38 +222,51 @@ function compileExpr( * it will have access to the translated children. */ if (exprHasE(expr)) { - compileExpr(resultSet, context, expr.e, state); + compileExpr( + resultSet, + context, + expr.e, + state, + true, + wantFromKid(expr, 'e', want) + ); } else if (exprHasKids(expr)) { - for (const kidExpr of Object.values(expr.kids)) { + for (const [kidName, kidExpr] of Object.entries(expr.kids)) { if (kidExpr === null) continue; + const kidWant = wantFromKid(expr, kidName, want); if (Array.isArray(kidExpr)) { for (const e of kidExpr) { - compileExpr(resultSet, context, e, state); + compileExpr(resultSet, context, e, state, true, kidWant); } } else { - compileExpr(resultSet, context, kidExpr, state); + compileExpr(resultSet, context, kidExpr, state, true, kidWant); } } } + const finish = (sql: string): T => { + const wrapped = wrap && exprHasKids(expr) ? `(${sql})` : sql; + expr.sql = asKind(context.dialect, wrapped, kindProduced(expr, want), want); + return expr; + }; + /* * Give the dialect a chance to translate this node */ const qi = resultSet.getQueryInfo(); const dialectSQL = context.dialect.exprToSQL(qi, expr); if (dialectSQL) { - expr.sql = wrap && exprHasKids(expr) ? `(${dialectSQL})` : dialectSQL; - return expr; + return finish(dialectSQL); } const sql = (() => { switch (expr.node) { case 'field': - return generateFieldFragment(resultSet, context, expr, state); + return generateFieldFragment(resultSet, context, expr, state, want); case 'parameter': - return generateParameterFragment(resultSet, context, expr, state); + return generateParameterFragment(resultSet, context, expr, state, want); case 'given': - return generateGivenFragment(resultSet, context, expr, state); + return generateGivenFragment(resultSet, context, expr, state, want); case 'filteredExpr': return generateFilterFragment(resultSet, context, expr, state); case 'all': @@ -319,8 +425,7 @@ function compileExpr( } })(); - expr.sql = wrap && exprHasKids(expr) ? `(${sql})` : sql; - return expr; + return finish(sql); } /** @@ -331,13 +436,21 @@ export function exprToSQL( resultSet: FieldInstanceResult, context: QueryStruct, exprToTranslate: Expr, - state: GenerateState = new GenerateState() + state: GenerateState = new GenerateState(), + want: SQLKind = 'value' ): string { // Make a deep copy that we can mutate during compilation const exprCopy = deepCopyExpr(exprToTranslate); // Compile the copy, setting .sql on all nodes - const compiled = compileExpr(resultSet, context, exprCopy, state, false); + const compiled = compileExpr( + resultSet, + context, + exprCopy, + state, + false, + want + ); return compiled.sql!; } @@ -799,7 +912,8 @@ export function generateFieldFragment( resultSet: FieldInstanceResult, context: QueryStruct, expr: FieldnameNode, - state: GenerateState + state: GenerateState, + want: SQLKind = 'value' ): string { // find the structDef and return the path to the field... const fieldRef = context.getFieldByName(expr.path, expr.at); @@ -808,7 +922,8 @@ export function generateFieldFragment( resultSet, fieldRef.parent, fieldRef.fieldDef.e, - state + state, + want ); return `(${ret})`; } else { @@ -824,7 +939,7 @@ export function generateFieldFragment( } // The normal case - just generate the SQL reference - return sqlFullChildReference( + const ref = sqlFullChildReference( fieldRef.parent, fieldRef.fieldDef.name, fieldRef.parent.structDef.type === 'record' @@ -834,6 +949,11 @@ export function generateFieldFragment( } : undefined ); + // A boolean column holds the value a condition became + if (fieldRef.fieldDef.type === 'boolean' && want === 'condition') { + return context.dialect.sqlValueAsCondition(ref); + } + return ref; } } @@ -850,13 +970,14 @@ export function generateParameterFragment( resultSet: FieldInstanceResult, context: QueryStruct, expr: ParameterNode, - state: GenerateState + state: GenerateState, + want: SQLKind = 'value' ): string { const name = expr.path[0]; context.eventStream?.emit('source-argument-compiled', {name}); const argument = safeRecordGet(context.arguments(), name); if (argument?.value) { - return exprToSQL(resultSet, context, argument.value, state); + return exprToSQL(resultSet, context, argument.value, state, want); } throw new MalloyCompileError( `Parameter '${expr.path.join('.')}' has no value supplied.`, @@ -892,12 +1013,13 @@ export function generateGivenFragment( resultSet: FieldInstanceResult, context: QueryStruct, expr: GivenRefNode, - state: GenerateState + state: GenerateState, + want: SQLKind = 'value' ): string { // The bound expr may itself be a `$OTHER`-bearing expression; recursive // compile handles default chains. const bound = resolveGivenBoundExpr(context, expr); - return exprToSQL(resultSet, context, bound, state); + return exprToSQL(resultSet, context, bound, state, want); } function unsatisfiedGivenMessage(refName: string): string { @@ -917,7 +1039,9 @@ export function generateFilterFragment( ): string { const allWhere = new AndChain(state.whereSQL); for (const cond of expr.kids.filterList) { - allWhere.add(exprToSQL(resultSet, context, cond.e, state.withWhere())); + allWhere.add( + exprToSQL(resultSet, context, cond.e, state.withWhere(), 'condition') + ); } return exprToSQL( resultSet, diff --git a/packages/malloy/src/model/field_instance.ts b/packages/malloy/src/model/field_instance.ts index a44ae9256..d82df73d9 100644 --- a/packages/malloy/src/model/field_instance.ts +++ b/packages/malloy/src/model/field_instance.ts @@ -64,7 +64,8 @@ export class FieldInstanceField implements FieldInstance { resultSet: FieldInstanceResult, context: QueryStruct, expr: Expr, - state?: GenerateState + state?: GenerateState, + want?: 'value' | 'condition' ) => string; static registerExpressionCompiler( @@ -72,7 +73,8 @@ export class FieldInstanceField implements FieldInstance { resultSet: FieldInstanceResult, context: QueryStruct, expr: Expr, - state?: GenerateState + state?: GenerateState, + want?: 'value' | 'condition' ) => string ) { FieldInstanceField.exprCompiler = compiler; @@ -110,7 +112,9 @@ export class FieldInstanceField implements FieldInstance { return FieldInstanceField.exprCompiler( this.parent, this.f.parent, - this.f.fieldDef.e + this.f.fieldDef.e, + undefined, + this.fieldUsage.type === 'where' ? 'condition' : 'value' ); } diff --git a/packages/malloy/src/model/query_query.ts b/packages/malloy/src/model/query_query.ts index a559e8039..1fec0cb49 100644 --- a/packages/malloy/src/model/query_query.ts +++ b/packages/malloy/src/model/query_query.ts @@ -590,7 +590,13 @@ export class QueryQuery extends QueryField { (which === 'having' && expressionIsCalculation(cond.expressionType)) || (which === 'where' && expressionIsScalar(cond.expressionType)) ) { - const sqlClause = exprToSQL(resultStruct, context, cond.e, undefined); + const sqlClause = exprToSQL( + resultStruct, + context, + cond.e, + undefined, + 'condition' + ); resultFilters.add(sqlClause); } } diff --git a/test/src/databases/sqlserver/boolean-coercion.spec.ts b/test/src/databases/sqlserver/boolean-coercion.spec.ts new file mode 100644 index 000000000..30b8c1580 --- /dev/null +++ b/test/src/databases/sqlserver/boolean-coercion.spec.ts @@ -0,0 +1,100 @@ +/* + * Copyright Contributors to the Malloy project + * SPDX-License-Identifier: MIT + */ + +import {RuntimeList} from '../../runtimes'; +import {describeIfDatabaseAvailable} from '../../util'; +import '@malloydata/malloy/test/matchers'; +import {wrapTestModel} from '@malloydata/malloy/test'; + +const [describe] = describeIfDatabaseAvailable(['sqlserver']); + +// In the boolean_coercion experiment a boolean used as a value is 1, 0 or +// NULL in SQL, which the result reader shows as a boolean, and a boolean +// column read back is a condition again. +describe('SQL Server boolean coercion', () => { + const runtimes = new RuntimeList(['sqlserver']); + const runtime = runtimes.runtimeMap.get('sqlserver'); + if (runtime === undefined) { + throw new Error("Couldn't build runtime"); + } + const tm = wrapTestModel( + runtime, + `##! experimental { sql_functions boolean_coercion } +source: s is sqlserver.sql("SELECT 1 AS n UNION ALL SELECT 2 UNION ALL SELECT 2") extend { + dimension: big is n > 1 + measure: c is count() +} +source: sn is sqlserver.sql("SELECT CAST(NULL AS INT) AS n UNION ALL SELECT 2") extend { + dimension: big is n > 1 + measure: c is count() +} +source: pk is sqlserver.sql("SELECT 1 AS n UNION ALL SELECT 2") extend { + dimension: big is n > 1 + primary_key: big +} +source: j is sqlserver.sql("SELECT 1 AS n UNION ALL SELECT 2") extend { + join_one: pk on pk.n = n +} +` + ); + + afterAll(async () => { + await runtimes.closeAll(); + }); + + test('a boolean result column', async () => { + await expect(` + run: s -> { group_by: big; aggregate: c; order_by: big } + `).toEqualResult(tm, [ + {big: false, c: 1}, + {big: true, c: 2}, + ]); + }); + + test('an unknown condition is NULL', async () => { + await expect(` + run: sn -> { group_by: big; aggregate: c; order_by: big } + `).toEqualResult(tm, [ + {big: null, c: 1}, + {big: true, c: 1}, + ]); + }); + + test('select: * includes the boolean', async () => { + await expect(` + run: s -> { select: *; where: n = 1 } + `).toEqualResult(tm, [{n: 1, big: false}]); + }); + + test('a boolean primary key', async () => { + await expect(` + run: j -> { aggregate: keys is pk.count(), total is pk.n.sum() } + `).toEqualResult(tm, [{keys: 2, total: 3}]); + }); + + test('a boolean column from an earlier stage is a condition again', async () => { + await expect(` + run: s -> { group_by: n, big } -> { where: big; select: n } + `).toEqualResult(tm, [{n: 2}]); + }); + + test('a boolean compared and null-tested', async () => { + await expect(` + run: sn -> { aggregate: nulls is c { where: big is null }, trues is c { where: big = true } } + `).toEqualResult(tm, [{nulls: 1, trues: 1}]); + }); + + test('a condition is still a condition', async () => { + await expect(` + run: s -> { where: big; aggregate: c } + `).toEqualResult(tm, [{c: 2}]); + await expect(` + run: s -> { where: not big; aggregate: c } + `).toEqualResult(tm, [{c: 1}]); + await expect(` + run: sn -> { where: not big; aggregate: c } + `).toEqualResult(tm, [{c: 1}]); + }); +}); From 30fe080115ed3a131abc5b4fc7d45c338d0d6f85 Mon Sep 17 00:00:00 2001 From: James Estes Date: Tue, 20 Oct 2026 18:59:06 -0600 Subject: [PATCH 15/15] A pick of conditions, raw SQL and a filter expression coerce correctly A CASE is a value, so the branches of a pick or case are compiled as values and the whole is tested with = 1 where a condition is wanted. SQL the author wrote in a sql_ function is left as written. A boolean filter expression on a dialect without boolean values is compiled in condition form. A cast to boolean stays refused, since there is no value to make. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01HXm9dCuXFseFX4FS2DK1My Signed-off-by: James Estes --- .../src/lang/ast/expressions/boolean-value.ts | 8 ++-- .../src/lang/ast/expressions/expr-cast.ts | 4 +- .../src/lang/ast/expressions/expr-func.ts | 1 + .../src/lang/test/boolean-coercion.spec.ts | 39 +++++++++++++++++++ .../malloy/src/model/expression_compiler.ts | 23 ++++++++--- packages/malloy/src/model/filter_compilers.ts | 18 ++++++++- packages/malloy/src/model/malloy_types.ts | 3 ++ 7 files changed, 86 insertions(+), 10 deletions(-) diff --git a/packages/malloy/src/lang/ast/expressions/boolean-value.ts b/packages/malloy/src/lang/ast/expressions/boolean-value.ts index e51cbb112..2a6a8fd34 100644 --- a/packages/malloy/src/lang/ast/expressions/boolean-value.ts +++ b/packages/malloy/src/lang/ast/expressions/boolean-value.ts @@ -20,15 +20,17 @@ export function booleanValueError( type: {type: string} | undefined, where: string, logTo: MalloyElement, - hint: string + hint: string, + evenWhenCoerced = false ): ExprValue | undefined { const dialect = fs.dialectObj(); if (type?.type !== 'boolean' || dialect?.booleanType !== 'none') { return undefined; } // The compiler coerces a boolean to a value in the boolean_coercion - // experiment, so no use of one is refused - if (logTo.inExperiment('boolean_coercion', true)) { + // experiment, so no use of one is refused, unless there is no value the + // use could coerce to + if (!evenWhenCoerced && logTo.inExperiment('boolean_coercion', true)) { return undefined; } return logTo.loggedErrorExpr('boolean-value-unsupported', { diff --git a/packages/malloy/src/lang/ast/expressions/expr-cast.ts b/packages/malloy/src/lang/ast/expressions/expr-cast.ts index 9b037df19..a0be659fc 100644 --- a/packages/malloy/src/lang/ast/expressions/expr-cast.ts +++ b/packages/malloy/src/lang/ast/expressions/expr-cast.ts @@ -49,12 +49,14 @@ export class ExprCast extends ExpressionDef { } const noCast = booleanValueError(fs, expr, 'in a cast', this, NUMBER_HINT) || + // A cast makes a value, and there is no boolean value to make booleanValueError( fs, dataType, 'as the type of a cast', this, - 'write a comparison' + 'write a comparison', + true ); if (noCast) return noCast; return computedExprValue({ diff --git a/packages/malloy/src/lang/ast/expressions/expr-func.ts b/packages/malloy/src/lang/ast/expressions/expr-func.ts index b18705e2f..6807a88fe 100644 --- a/packages/malloy/src/lang/ast/expressions/expr-func.ts +++ b/packages/malloy/src/lang/ast/expressions/expr-func.ts @@ -472,6 +472,7 @@ export class ExprFunc extends ExpressionDef { } const sqlExpr = composeSQLExpr(expr); + sqlExpr.authored = true; // The compiler needs to know this raw SQL is a condition if (func.name === 'sql_boolean') { sqlExpr.typeDef = {type: 'boolean'}; diff --git a/packages/malloy/src/lang/test/boolean-coercion.spec.ts b/packages/malloy/src/lang/test/boolean-coercion.spec.ts index 68ff9501b..53425c8f2 100644 --- a/packages/malloy/src/lang/test/boolean-coercion.spec.ts +++ b/packages/malloy/src/lang/test/boolean-coercion.spec.ts @@ -143,6 +143,15 @@ describe('boolean coercion on a dialect without boolean values', () => { sqlOf('run: m -> { group_by: ai, big } -> { where: big; select: ai }') ).toContain('WHERE (base."big" = 1)'); }); + test('a pick of conditions', () => { + expect( + sqlOf( + 'run: m -> { where: pick big when ai > 1 else false; aggregate: n }' + ) + ).toContain( + `WHERE (CASE WHEN (base."ai">1) THEN ${bigRef} ELSE CASE WHEN (1=0) THEN 1 WHEN NOT ((1=0)) THEN 0 END END = 1)` + ); + }); test('?? of booleans', () => { expect( sqlOf('run: m -> { where: big ?? false; aggregate: n }') @@ -202,12 +211,42 @@ describe('boolean coercion on a dialect without boolean values', () => { expect(sql).toMatch(/WHERE LEFT\(/); expect(sql).not.toContain('CASE'); }); + test('raw SQL decides for itself', () => { + const sql = sqlOf( + 'run: m -> { group_by: x is sql_number("""CASE WHEN ${big} THEN 1 ELSE 0 END""") }' + ); + expect(sql).toContain(`CASE WHEN ${bigCond} THEN 1 ELSE 0 END as "x"`); + }); + test('a boolean filter expression', () => { + expect( + sqlOf("run: m -> { where: big ~ f'true'; aggregate: n }") + ).toContain(`WHERE ${bigCond}`); + expect( + sqlOf("run: m -> { where: big ~ f'not true'; aggregate: n }") + ).toContain(`WHERE (CASE WHEN ${bigCond} THEN 1 ELSE 0 END = 0)`); + expect( + sqlOf("run: m -> { where: big ~ f'=false'; aggregate: n }") + ).toContain(`WHERE NOT (${bigCond})`); + expect( + sqlOf("run: m -> { where: big ~ f'null'; aggregate: n }") + ).toContain( + `WHERE CASE WHEN ${bigCond} THEN 1 WHEN NOT (${bigCond}) THEN 0 END IS NULL` + ); + }); test('a boolean parameter as a filter', () => { const sql = sqlOf('run: pm -> { where: pv; aggregate: n }'); expect(sql).toContain('WHERE ((1=1))'); }); }); + test('a cast to boolean is still refused: there is no value to make', () => { + const tt = new TestTranslator( + prefix + 'run: m -> { where: ai::boolean; aggregate: n }' + ); + const problems = tt.translate().problems ?? []; + expect(problems.map(p => p.code)).toContain('boolean-value-unsupported'); + }); + test('without the experiment a boolean value is still refused', () => { const tt = new TestTranslator( `##! experimental.dialect.sqlserver diff --git a/packages/malloy/src/model/expression_compiler.ts b/packages/malloy/src/model/expression_compiler.ts index a8bbda99d..e318b6c41 100644 --- a/packages/malloy/src/model/expression_compiler.ts +++ b/packages/malloy/src/model/expression_compiler.ts @@ -123,7 +123,7 @@ function deepCopyExpr(expr: T): T { * a value, so the compiler tracks which kind each edge wants and which kind * each node produced, and asks the dialect to coerce where they differ. */ -export type SQLKind = 'value' | 'condition'; +export type SQLKind = 'value' | 'condition' | 'raw'; /** * The kind a node wants from the child named `kid`, given the kind the @@ -139,12 +139,20 @@ function wantFromKid(expr: Expr, kid: string, want: SQLKind): SQLKind { case '()': return want; case 'case': + // A CASE is a value, so its branches are values; a when is a + // condition unless there is a subject to compare it with if (kid === 'caseWhen') { return expr.kids.caseValue ? 'value' : 'condition'; } - return kid === 'caseValue' ? 'value' : want; + return 'value'; case 'filteredExpr': return kid === 'filterList' ? 'condition' : 'value'; + case 'genericSQLExpr': + // SQL the author wrote says what it wants itself; a function's + // implementation wants values + return expr.authored ? 'raw' : 'value'; + case 'filterMatch': + return expr.dataType === 'boolean' ? 'condition' : 'value'; default: return 'value'; } @@ -187,7 +195,6 @@ function kindProduced(expr: Expr, want: SQLKind): SQLKind { case 'parameter': case 'given': case '()': - case 'case': return want; default: return 'value'; @@ -200,7 +207,7 @@ function asKind( produced: SQLKind, want: SQLKind ): string { - if (produced === want) { + if (produced === want || want === 'raw') { return sql; } return produced === 'condition' @@ -1325,7 +1332,13 @@ export function* stringsFromSQLExpression( yield str; const expr = subExprList.shift(); if (expr) { - yield exprToSQL(resultSet, context, expr, state); + yield exprToSQL( + resultSet, + context, + expr, + state, + e.authored ? 'raw' : 'value' + ); } } } diff --git a/packages/malloy/src/model/filter_compilers.ts b/packages/malloy/src/model/filter_compilers.ts index beb030148..1f9c84ba4 100644 --- a/packages/malloy/src/model/filter_compilers.ts +++ b/packages/malloy/src/model/filter_compilers.ts @@ -171,7 +171,23 @@ export const FilterCompilers = { // booleanCompile is self-safe (this if-chain has no default, and `none` // must not fall through to the null/false truth-table below). return noneToSQL(bc.not, d); - } else if (bc.operator === '=true') { + } + if (d.booleanType === 'none') { + // x is a condition, and the same truth table is written in conditions: + // an unknown condition already tests as false, and its negation as true + if (bc.operator === '=true') { + return bc.not ? `NOT ${px}` : x; + } else if (bc.operator === '=false') { + return bc.not ? x : `NOT ${px}`; + } else if (bc.operator === 'null') { + const v = d.sqlConditionAsValue(x); + return bc.not ? `${v} IS NOT NULL` : `${v} IS NULL`; + } else if (bc.operator === 'true') { + return bc.not ? d.sqlNot(x) : x; + } + return bc.not ? x : d.sqlNot(x); + } + if (bc.operator === '=true') { return bc.not ? `NOT ${px}` : x; } else if (bc.operator === '=false') { return bc.not ? x : `NOT ${px}`; diff --git a/packages/malloy/src/model/malloy_types.ts b/packages/malloy/src/model/malloy_types.ts index 45ffe45f0..06ee212a2 100644 --- a/packages/malloy/src/model/malloy_types.ts +++ b/packages/malloy/src/model/malloy_types.ts @@ -429,6 +429,9 @@ export interface GenericSQLExpr extends ExprWithKids { node: 'genericSQLExpr'; kids: {args: Expr[]}; src: string[]; + // SQL the model's author wrote (a sql_ function), as opposed to a + // function's implementation + authored?: boolean; } export interface NullNode extends ExprLeaf {