Repository navigation
Fix nullable AND results in tree-model CASE - #18875
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
No unresolved review issues remain, and validation passed.
0 open findings
What changed in this PR
Fixes nullable three-valued AND evaluation in tree-model CASE expressions by caching the computed result.
Changes:
- Corrects
LogicAndColumnTransformercaching. - Adds nullable
ANDunit coverage. - Adds a
CASEintegration regression test.
| File | Description |
|---|---|
iotdb-core/datanode/src/test/java/org/apache/iotdb/db/queryengine/transformation/dag/column/binary/LogicBinaryColumnTransformerTest.java |
Tests nullable AND combinations and selection masks. |
iotdb-core/calc-commons/src/main/java/org/apache/iotdb/calc/transformation/dag/column/binary/LogicAndColumnTransformer.java |
Caches the computed AND result. |
integration-test/src/test/java/org/apache/iotdb/db/it/query/IoTDBCaseWhenThenIT.java |
Adds SQL-level regression coverage. |
🧠 Review effort: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
JackieTien97
left a comment
JackieTien97
left a comment
There was a problem hiding this comment.
The fix looks right; it now matches LogicOrColumnTransformer.evaluateWithSelection. The comments below are about test coverage.
|
Thanks @JackieTien97! Addressed all three comments in 3316fa6: stronger NULL/selection assertions, AND/OR coverage, and both SQL regressions. All 7 unit tests and the targeted integration test pass. |
JackieTien97
left a comment
JackieTien97
left a comment
There was a problem hiding this comment.
Thanks for the update. The new assertions now catch the FALSE-to-NULL regression, and OR has the same coverage: I ran the updated LogicBinaryColumnTransformerTest against the pre-fix code and against one-line mutations of LogicAndColumnTransformer / LogicOrColumnTransformer (short-circuit branch writing NULL, dropping selection.clone(), initializeColumnCache(rightColumn)), and each one fails the corresponding tests. LGTM.
The dual-tree-auto-basic (..., 2) failure is Maven Central returning 429 while resolving surefire-junit3 in external-api, unrelated to this change.
Tree-model CASE can take the wrong branch when a later WHEN contains nullable AND. For example, when
s1is NULL ands2is 1,CASE WHEN s1 > 100 THEN 1 WHEN s1 > 0 AND s2 > 0 THEN 2 ELSE 3 ENDreturns 2 instead of 3.LogicAndColumnTransformer.evaluateWithSelection()builds the correct three-valued AND result but caches the right operand. Cache the built result instead. This is a one-line production change and requires no cache-lifecycle or selection API changes.Regression coverage includes all nine NULL/false/true operand combinations with full and partial selection masks, plus the CASE SQL example with NULL, true, false, and earlier-branch rows.
Closes #18864.
Validation:
verifyfrom master8effe5d6ce, selecting onlyLogicBinaryColumnTransformerTest(5 tests) andIoTDBCaseWhenThenIT#testNullableAndInLaterCaseBranch(1 test, rebuilt 1C1D nodes): passed.