ECSQL: support IS / IS NOT operator between operands (null-safe comparison)#9440
ECSQL: support IS / IS NOT operator between operands (null-safe comparison)#9440khanaffan wants to merge 12 commits into
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…us value expressions and update related documentation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add IS / IS NOT operator cases (string literal, IS NOT null-safe, integer literal, function call, and parameter binding) to the markdown-based ECSQL test runner in Comparison.ecsql.md. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
aruniverse
left a comment
There was a problem hiding this comment.
Adversarial review (automated) — 4 inline findings below. One is a blocker (result-table column mismatch that will fail the .ecsql.md test runner) and should be fixed before merge. The others are low/informational.
…lity rules and update test cases for value expressions
aruniverse
left a comment
There was a problem hiding this comment.
Two related follow-ups on the disambiguation note wording — same single-word fix needed in both NextVersion.md and Operators.md. The sqlbison.y grammar comment in imodel-native#1476 is already accurate; these two docs just need to match it.
|
|
||
| The ECSQL `IS` and `IS NOT` operators can now be used between two operands — for example `prop1 IS [NOT] prop2`, where each operand may be any value expression: a property, the `NULL` literal, a constant, a parameter, a function call, an arithmetic expression, etc. These map to SQLite's **null-safe** comparison operators, so `NULL IS NULL` is `TRUE` and `1 IS NULL` is `FALSE`, unlike `=`/`<>` which treat a `NULL` operand as _unknown_. | ||
|
|
||
| Previously `IS` / `IS NOT` only supported the right-hand operands `NULL`, the boolean literals `TRUE`/`FALSE`/`UNKNOWN`, and the [ECClass type predicate](../learning/ECSqlReference/ECClassFilter.md) (`IS (ClassName)`). Those forms still take precedence — a right-hand operand that is exactly `NULL`/`TRUE`/`FALSE`/`UNKNOWN`, or a parenthesized `(ClassName)`, keeps its original meaning. |
There was a problem hiding this comment.
Same 'qualified' gap as Operators.md — unqualified class names were never type predicates
"a right-hand operand that is exactly
NULL/TRUE/FALSE/UNKNOWN, or a parenthesized(ClassName), keeps its original meaning"
The grammar requires a qualified name (schema.Class), an ONLY/ALL prefix, or a comma-separated list to route to the type-predicate path. A bare unqualified name like IS (MyClass) reduces as a value_exp (null-safe comparison) regardless of whether MyClass is a real ECClass — it was never reachable as a type predicate.
Suggested fix:
or a parenthesized *qualified* class name (e.g. `(schema.Class)`), an `ONLY`/`ALL`-prefixed form, or a comma-separated list keeps its original meaning. A single unqualified name in parentheses is always parsed as a value expression.
There was a problem hiding this comment.
qualified keyword was added and the wording for unqualified names is correct. However, the IS (alias.prop) qualified-name fallback (added in imodel-native commit 593ff15eb9) is not mentioned here. A qualified name that doesn't resolve to a class — e.g. S1 IS (Foo.S2) or Status IS (ts.Status.Active) — previously failed with 'class not found' and now works as a null-safe comparison. This is the most developer-visible part of the latest commit and should appear in the changelog. New inline comment posted on line 185 with a suggested addition.
|
|
||
| Both operands must be type-compatible, following the same rules as `=` and `<>`: comparable primitive types (for example two strings, or numeric types compared with each other) or composite types of the same shape (`Point2d` with `Point2d`, a navigation property with a navigation property), with the `NULL` literal allowed against any type. Comparing unrelated types — for example a `string` against a `Point3d` — is rejected when the statement is prepared. | ||
|
|
||
| > Note: `IS [NOT]` is also used by the unrelated [ECClass filter](./ECClassFilter.md) predicate (`<classId> IS [NOT] (<class-name>, ...)`) and by the boolean truth tests `IS [NOT] TRUE`/`FALSE`/`UNKNOWN`. Those forms take precedence: a right-hand operand that is exactly `NULL`/`TRUE`/`FALSE`/`UNKNOWN`, or a parenthesized `(ClassName)`, keeps its original meaning. Write such a value expression without the outer parentheses (for example `x IS y + 1` rather than `x IS (y + 1)`) when it could be mistaken for a type predicate. |
There was a problem hiding this comment.
'qualified' missing — same issue as NextVersion.md line 185
"a right-hand operand that is exactly
NULL/TRUE/FALSE/UNKNOWN, or a parenthesized(ClassName), keeps its original meaning"
The note correctly warns about the disambiguation, but omits the key constraint: the type-predicate path requires a qualified class name (schema.Class). An unqualified name like IS (MyClass) — even if MyClass is a real ECClass — has always resolved as a value expression. The sqlbison.y comment (imodel-native#1476) already says this precisely; this note should match it.
Additionally, the example x IS y + 1 vs x IS (y + 1) doesn't illustrate the real disambiguation risk — (y + 1) cannot be parsed as a type predicate regardless of parentheses. The genuine risk is a qualified identifier: IS (schema.myProp) where schema.myProp looks like a class reference. Optionally reword to:
> Note: … Those forms take precedence: a right-hand operand that is exactly
> `NULL`/`TRUE`/`FALSE`/`UNKNOWN`, a **qualified** class name in parentheses
> (e.g. `(schema.Class)`), an `ONLY`/`ALL`-prefixed form, or a comma-separated list
> keeps its original meaning. A single unqualified name in parentheses (e.g. `(MyProp)`)
> is always a value expression, even if a class by that name exists. To avoid ambiguity,
> write qualified names without outer parentheses (e.g. `x IS schema.prop` rather than
> `x IS (schema.prop)`).There was a problem hiding this comment.
ECSQL does not support unqualified class names. So I am not sure why we have to explicitly say qualified.
There was a problem hiding this comment.
qualified was added and the Note is significantly improved. One remaining issue: the parenthetical "(if a name is both a class and a property, the property reading wins)" contradicts imodel-native's CHANGES.md which says "the class (type-predicate) reading wins" for the qualified case. Both can be reconciled in context (unqualified names are never classes in ECSQL, so 'property wins' is vacuously true for that sub-case), but a user reading Operators.md in isolation will take the parenthetical as the general rule and be wrong about qualified ambiguous names. New inline comment posted on line 78 with a rewrite suggestion.
hl662
left a comment
There was a problem hiding this comment.
Nambot 🤖 (powered by GPT-5.5)
| }); | ||
| }); | ||
|
|
||
| describe("IS / IS NOT operators between operands", () => { |
There was a problem hiding this comment.
This adds another feature-sized block to an already sprawling 3.7k-line catch-all test file. I think there's a code-judo move here: split this into a focused ECSQL IS-operator test module with small helpers for schema setup/inserts/count assertions, and put parser/normalization coverage next to the existing IS NULL / IS (type) cases in ECSqlAst.test.ts. That would make the new behavior easier to find and avoid continuing to grow this file as the default home for unrelated ECSQL coverage.
Nambot 🤖 (powered by GPT-5.5)
| assert.equal(await queryCount(ecdb, "SELECT ECInstanceId FROM ts.Foo WHERE ECClassId IS (ts.Foo)"), 5); | ||
|
|
||
| // operands of incompatible types are rejected (string vs point) | ||
| let threw = false; |
There was a problem hiding this comment.
This negative assertion is too loose: any exception makes the test pass, including unrelated failures from schema setup, query wrapping, or a typo. Can we assert the specific prepare failure/message/status instead, like the explicit failure checks elsewhere in this file? The test should prove that incompatible IS operands are rejected for the intended reason, not just that something threw.
Nambot 🤖 (powered by GPT-5.5)
…erand types and comparisons
… failure The negative test for 'string IS point' previously caught any exception, so unrelated failures (schema setup, typos) would pass it silently. Assert the specific native type-mismatch message instead, proving the IS operator is rejected for the intended reason. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
aruniverse
left a comment
There was a problem hiding this comment.
Two documentation gaps found after reviewing the IS (alias.prop) semantic fallback added in imodel-native commit 593ff15eb9. Both are in the itwinjs-core changelog/reference docs and don't reflect the full new behavior.
|
|
||
| Both operands must be type-compatible, following the same rules as `=` and `<>`: comparable primitive types (for example two strings, or numeric types compared with each other) or composite types of the same shape (`Point2d` with `Point2d`, a navigation property with a navigation property), with the `NULL` literal allowed against any type. Comparing unrelated types — for example a `string` against a `Point3d` — is rejected when the statement is prepared. | ||
|
|
||
| > Note: `IS [NOT]` is also used by the unrelated [ECClass filter](./ECClassFilter.md) predicate (`<classId> IS [NOT] (<class-name>, ...)`) and by the boolean truth tests `IS [NOT] TRUE`/`FALSE`/`UNKNOWN`. These keep their original meaning and take precedence only when the right-hand operand matches their shape: a bare `NULL`/`TRUE`/`FALSE`/`UNKNOWN`, or a parenthesized **qualified** class name — optionally with an `ONLY`/`ALL` prefix or written as a comma-separated list (for example `(bis.Element)`, `(ONLY bis.Element)`, or `(bis.Element, bis.Model)`). Any other parenthesized right-hand operand is a value expression: a parenthesized *unqualified* name such as `(MyProperty)` is read as that property (if a name is both a class and a property, the property reading wins), so `prop1 IS (prop2)` and `x IS (y + 1)` are null-safe value comparisons, while `ECClassId IS (bis.Element)` stays a type predicate. |
There was a problem hiding this comment.
Tie-breaking parenthetical says 'property reading wins' — contradicts iModelCore/ECDb/CHANGES.md which says 'class reading wins'
Current text in this Note:
"a parenthesized unqualified name such as
(MyProperty)is read as that property (if a name is both a class and a property, the property reading wins))"
The imodel-native CHANGES.md (commit 593ff15eb9) says the opposite for the qualified case:
"When a parenthesized qualified name is both a class and a valid property path, the class (type-predicate) reading wins."
Both statements can be technically reconciled (unqualified names are never classes in ECSQL, so 'property wins' is vacuously correct for unqualified names), but a reader who encounters the Operators.md parenthetical will take it as the general rule — and it's wrong for qualified names.
The parenthetical should either be removed or replaced with the correct general rule. Suggested rewrite:
Any other parenthesized right-hand operand is a value expression:
a parenthesized *unqualified* name such as `(MyProperty)` is read as that property (ECSQL
class names must always be schema-qualified, so there is no ambiguity here);
a parenthesized *qualified* name such as `(alias.prop)` that does **not** resolve to a class
is likewise a null-safe value comparison — if it **does** resolve to a class, the
type-predicate reading wins.|
|
||
| The ECSQL `IS` and `IS NOT` operators can now be used between two operands — for example `prop1 IS [NOT] prop2`, where each operand may be any value expression: a property, the `NULL` literal, a constant, a parameter, a function call, an arithmetic expression, etc. These map to SQLite's **null-safe** comparison operators, so `NULL IS NULL` is `TRUE` and `1 IS NULL` is `FALSE`, unlike `=`/`<>` which treat a `NULL` operand as _unknown_. | ||
|
|
||
| Previously `IS` / `IS NOT` only supported the right-hand operands `NULL`, the boolean literals `TRUE`/`FALSE`/`UNKNOWN`, and the [ECClass type predicate](../learning/ECSqlReference/ECClassFilter.md) (`IS (ClassName)`). Those forms still take precedence — a right-hand operand that is exactly `NULL`/`TRUE`/`FALSE`/`UNKNOWN`, or a parenthesized **qualified** class name such as `(bis.Element)` (optionally with an `ONLY`/`ALL` prefix or a comma-separated list), keeps its original meaning. A parenthesized *unqualified* name such as `(prop2)` is instead read as a value expression, so `prop1 IS (prop2)` is a null-safe comparison. |
There was a problem hiding this comment.
Missing: the IS (alias.prop) qualified-name fallback is not mentioned here
The current text only covers unqualified names:
"A parenthesized unqualified name such as
(prop2)is instead read as a value expression, soprop1 IS (prop2)is a null-safe comparison."
However, imodel-native commit 593ff15eb9 specifically added support for qualified names that don't resolve to a class — e.g. S1 IS (Foo.S2), Status IS (ts.Status.Active), f.S1 IS (f.Info.Code). These were previously rejected with a 'class not found' error. A user upgrading who writes x IS (alias.prop) would find no changelog mention that this is now valid.
Suggested addition after the existing unqualified-name sentence:
A parenthesized *qualified* name (e.g. `(alias.prop)` or `(ts.Status.Active)`) that does not
resolve to a known ECClass is also treated as a null-safe value expression instead of failing
with 'class not found'. When a qualified name is both a valid class and a valid property path,
the type-predicate (class) reading takes precedence.
imodel-native: iTwin/imodel-native#1476
closes: #9439
This pull request adds support for using the
ISandIS NOToperators between two operands in ECSQL, extending their previous usage (which was limited to comparisons withNULL, boolean literals, or type predicates). Now, these operators can be used for null-safe comparisons between properties, including multi-column types like points and navigation properties, with appropriate column-wise expansion. The changes include parser, lexer, and documentation updates, as well as comprehensive tests to verify the new behavior.ECSQL Syntax and Semantics Enhancements:
ISandIS NOTbetween two operands (e.g.,prop1 IS [NOT] prop2), with null-safe comparison semantics consistent with SQLite. For multi-column operands,ISjoins per-column comparisons withAND, andIS NOTwithOR.2.0.4.0in documentation, code, and tests to reflect the new syntax support.