Repository navigation
Conversation
Collect phase DAG nodes once and clear each cached column before a new block or device batch. Rebuild missing schema UPDATE common expressions with the filter mask while preserving existing selection evaluation.
UserDefineScalarFunctionTransformer applied the row-selection mask inverted: selected rows were nulled and unselected rows were evaluated, so scalar functions in CASE branches returned null for the branch rows. Flip the condition to match every other transformer's selection semantics. DeviceAttributeUpdater now asks canEvaluateWithSelection() before recomputing a CASE-cleared common expression with the filter guard mask. Stateful transformers such as binary DIFF (and any subtree containing them, plus user-defined scalar/mappable functions with no purity contract) fall back to a full evaluation, which the input-boundary snapshot makes idempotent. The shape diff(CAST(attr AS DOUBLE), <non-literal flag>) makes binary DIFF reachable in attribute UPDATE despite STRING-only attribute columns. Tests: - capability propagation unit test in ColumnTransformerTest - scalar-function-in-CASE-branch IT in IoTDBUserDefinedScalarFunctionIT - attribute-UPDATE shared-DIFF IT in IoTDBCaseDeviceCacheIT - comments documenting the preserved-master expectations of the binary DIFF ITs in IoTDBCaseWhenThenTableIT
Use CUSTOM region-group extension for the single-region test fixture. Under PROACTIVE, the default count is only a minimum, so a 1C3D cluster can split devices across schema regions and invalidate the shared DIFF expectations.
JackieTien97
left a comment
There was a problem hiding this comment.
I built this head (24c7882) and the merge-base (673434b) and ran the same SQL against a 1C1D server of each, at the default config and with max_tsblock_line_number = 1 / 3 to force block and batch splits.
Fixed (master → this PR)
- #18792 and its variants (
CASE WHEN sum(c) > 0 …with a non-pushdown predicate, including multiple windows):301: Index 1 out of bounds/BufferUnderflowException→ correct. - DIFF shared by a CASE filter and the projection across blocks:
301→ correct. CASE + plain DIFF in one projection (CASE item first): AIOOBE / wrong values → correct, checked on 30,000 rows. coalesce/greatestin a CASE THEN branch: NULL → correct.- Scalar UDF in a CASE THEN branch: wrong NULL → correct (the selection-inversion fix, which the description doesn't mention).
- Schema UPDATE with a CASE-cleared common (
upper(src), guardedCAST(src AS INT32)): NPE → correct.
Issues (details inline)
- Guarded UPDATE with a binary-DIFF common fails as soon as one device in the batch matches. When it doesn't fail, the written value can contradict the WHERE, and the set of updated devices depends on batch boundaries.
- New on this PR: the masked recompute can leave a masked column cached that the stateful fallback then reuses, so SET silently writes NULL depending on the order of the SET items (master throws an NPE here).
- Binary DIFF carries the state of whichever evaluation ran last into the next block: results depend on the order of SELECT items, and adding an OR term can remove rows. Same on master, but it's the case the DIFF checkpoint is meant to cover.
- Several ITs assert binary-DIFF values that their own comments describe as inconsistent.
Same bug class, outside the diff (master too)
TernaryColumnTransformer.evaluateWithSelectionreads the first child after evaluating the other two — the hazard fixed inMultiColumnTransformerhere.CASE WHEN x > 0 THEN x BETWEEN x - 1 AND x + 1 ELSE false ENDover x = 1, 2, -1 returns null, null, false instead of true, true, false. Reading each child's column right after evaluating it (asMultiColumnTransformer.evaluateWithSelectiondoes) avoids it.- In-block sibling clearing (deferred per the description) still gives silent wrong results:
x + CASE WHEN x > 0 THEN 2 ELSE 3 END→ NULL;coalesce(x, CASE WHEN x > 0 THEN x ELSE 0.0 END)andx BETWEEN CASE WHEN x > 0 THEN 0.0 ELSE -10.0 END AND 10.0→ NULL for every row;SHOW DEVICES … WHERE src = CASE …→301: … "leftColumn" is null; andUPDATE t SET b = concat(src, CASE WHEN src = 'match' THEN '-m' ELSE '-o' END)reports success but writes NULL to every device (bwas'seed'before). The last one is silent data loss — could we track these in an issue? - Query path:
SELECT CAST(s AS INT32) + 1 FROM t WHERE CASE WHEN s <> 'n/a' THEN CAST(s AS INT32) > 10 ELSE false ENDstill fails with701: Cannot cast n/a to INT32 type, while the UPDATE form now works. The description says this is intended; a follow-up issue would help since SELECT and UPDATE now disagree. - Unrelated, tree model:
LogicAndColumnTransformer.evaluateWithSelectioncachesrightColumninstead ofbuilder.build()(LogicOrColumnTransformercaches the result).CASE WHEN s3 > 0 THEN s1 > 0 AND s2 > 0 ELSE false ENDreturns null for (s1 = -5, s2 = 5) and true for s1 = null;CASE WHEN s1 > 100 THEN 'high' WHEN s1 > 0 AND s2 > 0 THEN 'mid' ELSE 'low' ENDreturns 'mid' for s1 = null.
Description
- "Apply the final filter mask when an UPDATE common expression must be recomputed": commons containing a binary DIFF are recomputed over all rows instead. The no-match early return and the UDSF selection fix aren't mentioned.
- "an outer CASE still protects invalid CAST input for binary diff(value, dynamic_boolean)" holds for SELECT, not for UPDATE (issue 1).
- Test counts are outdated (6 schema IT methods plus a UDF IT), and the benchmark predates the DIFF checkpoint and the per-batch
canEvaluateWithSelectionwalk.
| // Stateful transformers must be re-evaluated over every row: a masked evaluation | ||
| // would read the masked rows as nulls and corrupt their running state. The snapshot | ||
| // taken at the input boundary makes the full re-evaluation idempotent. | ||
| transformer.tryEvaluate(); |
There was a problem hiding this comment.
This fallback breaks the CASE guard as soon as one device in the batch matches.
Default config, so all devices are in one batch:
CREATE TABLE g3 (id STRING TAG, src STRING ATTRIBUTE, a STRING ATTRIBUTE, v DOUBLE FIELD);
INSERT INTO g3(time, id, src, a, v) VALUES (1,'d0','1','seed',1), (2,'d1','5','seed',1), (3,'d2','n/a','seed',1);
UPDATE g3 SET a = CAST(diff(CAST(src AS DOUBLE), src = 'x') AS STRING)
WHERE CASE WHEN src = 'n/a' THEN false ELSE diff(CAST(src AS DOUBLE), src = 'x') > 0 END;This PR: 305 … SemanticException: Cannot cast n/a to DOUBLE type, nothing updated (master: NPE). Expected: d1.a = '4.0'; the filter itself evaluates fine and matches d1. testGuardedCommonDiffWithoutMatchingDevices only covers a batch with no match (the early return) and a batch without 'n/a'. With 1000-device batches, a guarded device plus a match in the same batch is the normal case.
When nothing throws, the full recompute still disagrees with what the WHERE saw:
-- d0 '1', d1 '3', d2 '6'
SHOW DEVICES FROM g5 WHERE CASE WHEN src = '3' THEN false ELSE diff(CAST(src AS DOUBLE), src = 'x') IS NULL END; -- d0, d2
UPDATE g5 SET a = coalesce(CAST(diff(CAST(src AS DOUBLE), src = 'x') AS STRING), 'NULL') WHERE <same>;
-- d0 = 'NULL', d2 = '3.0': d2 was selected because the diff IS NULLAnd the set of updated devices depends on batch boundaries, because the full recompute replaces DIFF's carried state while the no-match early return keeps the masked one:
-- d0 '10', d1 '20', d2 '0', d3 '5'
UPDATE u SET a = CAST(diff(CAST(src AS DOUBLE), src = 'x') AS STRING)
WHERE CASE WHEN src = '0' THEN false ELSE diff(CAST(src AS DOUBLE), src = 'x') > 0 END;
-- max_tsblock_line_number = 1000: only d1 updated ('10.0')
-- max_tsblock_line_number = 3, batches [d0,d1,d2] | [d3]: d1 = '10.0' and d3 = '5.0'I don't see a recompute strategy that is right for a stateful common: a masked recompute changes its values, a full one runs the guarded rows. Keeping the column the filter produced is the consistent option, but that's the deferred redesign. Until then, could we add a test with a guarded row and a match in one batch, and drop the claim that the stateful case is handled?
| // CASE may clear a common expression. Recompute only rows whose filter guard passed. | ||
| for (ColumnTransformer transformer : commonTransformerList) { | ||
| if (transformer.canEvaluateWithSelection()) { | ||
| transformer.tryEvaluateWithSelection(selection); |
There was a problem hiding this comment.
New on this PR: a masked recompute here can feed the stateful fallback, so SET silently writes NULL.
-- w1: d0 '1', d1 '9', d2 '5' (one batch)
UPDATE w1 SET a = CAST(CAST(src AS DOUBLE) AS STRING),
b = CAST(diff(CAST(src AS DOUBLE), src = 'x') AS STRING)
WHERE CASE WHEN src = 'n/a' THEN false ELSE diff(CAST(src AS DOUBLE), src = 'x') > 0 END;
-- this PR: d1 a = '9.0', b = null (master: NPE)
-- with the two SET items swapped: d1 b = '8.0' (correct)commonTransformerList is [CAST(src AS DOUBLE), diff(...)]. The CAST is recomputed here with the filter mask ([null, 9, null]); its refcount is 2 (SET a plus DIFF's child), so the column stays cached after getColumn(). The DIFF then takes the tryEvaluate() fallback, but BinaryColumnTransformer.evaluate() calls leftTransformer.tryEvaluate(), which reuses the masked CAST column, and the masked nulls (with flag false) reset the DIFF. So the fallback isn't a full evaluation, the result depends on the order of the SET items, and unlike the exception above nothing reports it.
| } | ||
|
|
||
| @Override | ||
| protected void resetForNewInput() { |
There was a problem hiding this comment.
The state carried into the next block is whatever the last evaluation left, so a masked CASE pass that runs after the full one still corrupts the next block.
The snapshot makes re-evaluation within a block start from the same state, but the state handed to the next block comes from whichever evaluation ran last. If the plain diff is evaluated before a CASE that masks it, the masked pass wins: masked rows are read as null with a null flag, which sets lastValueIsNull.
-- max_tsblock_line_number = 3; x = 1..7, f = true, FLUSH after rows 3, 6, 7 -> blocks [1,2,3] | [4,5,6] | [7]
SELECT time, diff(x, f) AS d, CASE WHEN x = 7 THEN diff(x, f) ELSE 100.0 END AS a FROM fv4;
-- d = null,1,1,null,1,1,null ; a(7) = null
SELECT time, CASE WHEN x = 7 THEN diff(x, f) ELSE 100.0 END AS a, diff(x, f) AS d FROM fv4;
-- d = null,1,1,1,1,1,1 ; a(7) = 1.0Same at the default config: on 30,000 rows, SELECT diff(x, ignore_null) AS d, CASE WHEN x > 0 THEN x ELSE diff(x, ignore_null) END AS a returns d = null at the first row of every later block (rows 7282, 10001, 15001, 22282, 25001); the swapped order is correct. Filters too: on blocks x = [1,3,4] | [10,11], WHERE diff(x, f) IS NOT NULL returns 3, 4, 10, 11, but WHERE diff(x, f) IS NOT NULL OR CASE WHEN x > 5 THEN diff(x, f) > 0 ELSE false END returns 3, 4, 11 — the extra OR term drops x = 10.
Master behaves the same, so these aren't regressions, but it's the "preserving the previous block's contribution" case in the description, and the ITs only cover the CASE-first order. One option: record the end state of the unmasked evaluation and have the masked path restore it after its pass, so the next block starts from the full evaluation when there was one, and from the masked state only when the DIFF is used solely inside a CASE (master's behavior). That should fix the projection and OR cases above, though not the UPDATE batch dependence, which comes from the fallback.
| "null,200.0,5.0," | ||
| }, | ||
| DATABASE); | ||
| // The expected values preserve master's masked WHEN evaluation of the binary diff: a full |
There was a problem hiding this comment.
These expectations (here, L858 and L903) assert values the comments say a full evaluation wouldn't produce. The inconsistency also shows directly, on master and this PR, with f = false in every row: CASE WHEN x > 0 THEN x ELSE diff(x, false) END gives 1, -3, 3, -7, but CASE WHEN x > 0 THEN x ELSE diff(x, f) END gives 1, null, 3, null. In one statement, WHERE and SELECT can disagree: over x = 1, 3, 6, SELECT diff(x, f) AS d … WHERE CASE WHEN x = 3 THEN false ELSE diff(x, f) IS NULL END returns the x = 6 row with d = 3.0.
Freezing these as expected output will make a later fix look like a regression. Could these cases assert only the well-defined rows, or be marked as current behavior with an issue link?
| * evaluation, for this node and its whole subtree. Stateful transformers override this to return | ||
| * {@code false} so that callers re-evaluate them over every row. | ||
| */ | ||
| public boolean canEvaluateWithSelection() { |
There was a problem hiding this comment.
Nit: this is a plan property, but DeviceAttributeUpdater asks it for every common on every batch, and the recursion has no visited set — on the shared DAG from testProjectionCollectsSharedDagOnlyOnce it visits the input 4096 times, allocating a lambda and a boolean[] per visit. Computing it once in the constructor (next to projectCacheTransformers) avoids that.
Also, the Javadoc says stateful transformers return false "so that callers re-evaluate them over every row", but only the schema updater asks; CASE, AND and OR still mask binary DIFF without consulting it.
| } | ||
|
|
||
| @Override | ||
| public boolean canEvaluateWithSelection() { |
There was a problem hiding this comment.
Nit: the only caller can't reach this. Table-model scalar UDFs can't be used in UPDATE at all: building the updater fails on both builds (IllegalArgumentException: IoTDBLocalFactory must not be null for UDF execution in SchemaRegionMemoryImpl.constructDevicePredicateUpdater), and MappableUDFColumnTransformer is tree-model only. Fine as a safe default, but nothing tests it.
| protected TsBlock curBlock; | ||
| protected Column curFilterColumn; | ||
|
|
||
| // Retain the original constructor descriptor for precompiled subclasses without common roots. |
There was a problem hiding this comment.
Nit: which precompiled subclasses is this for? The in-tree callers (DevicePredicateFilter, DeviceBlackListConstructor) could pass Collections.emptyList(). If it's for out-of-tree code, note that the new abstract ColumnTransformer.forEachChild (L180) breaks precompiled ColumnTransformer subclasses the same way. Same for DeviceUpdater L42.
| commonTransformerList.forEach( | ||
| columnTransformer -> filterResultColumns.add(columnTransformer.getColumn())); | ||
| if (!commonTransformerList.isEmpty()) { | ||
| boolean[] selection = new boolean[deviceSchemaBatch.size()]; |
There was a problem hiding this comment.
Nit: this re-runs satisfy(curFilterColumn, i) for every row, but prepareBatchResult already put exactly those rows in indexes. Building the mask from indexes avoids two copies of the predicate, and it could be built lazily, only when a common isn't cached.
| try (Connection connection = EnvFactory.getEnv().getConnection(BaseEnv.TABLE_SQL_DIALECT); | ||
| Statement statement = connection.createStatement()) { | ||
| createTable(statement, "predicate_cache"); | ||
| for (String predicate : new String[] {CASE_RESULT + " = src"}) { |
There was a problem hiding this comment.
Nit: one-element loop. Also, only the CASE … = src order is covered; the other order still fails on both builds (SHOW DEVICES FROM cc WHERE src = CASE WHEN src = 'match' THEN 'match' ELSE 'other' END → 301: … "leftColumn" is null, same for COUNT DEVICES). Fine if in-block sibling clearing is out of scope, but a comment or issue link would make that explicit.
Reuse the DIFF values observed by schema filters when materializing SET expressions, preserving guarded CASTs and the filter state between batches. Retain child columns before later CASE evaluations can clear shared caches. Add regression coverage, limit assertions to defined DIFF results, and materialize schema filter masks only when needed. Validation: 73 focused unit tests, 18 table-model integration tests, and full reactor test compilation in both English and Chinese passed.
A CASE can leave shared transformer columns cached after one TsBlock or device batch is consumed. Reusing the DAG for the next input can then return stale values or use a column with the wrong length. This change resets the collected nodes before each input and rebuilds missing schema UPDATE common expressions only for devices that passed the filter. Re-evaluating a DIFF within the same block starts from that block's saved incoming state, preserving the previous block's contribution.
This is a narrow extraction from #18826. It keeps the existing selection evaluation and evaluation-time cache clearing mechanism.
Changes
The broader selection-cache redesign from #18826 remains deferred.
Validation
Performance
The new-input path clears a precomputed flat array in O(V) per evaluation phase, without a per-block graph walk or visited-set allocation. Schema UPDATE allocates a boolean selection mask and scans its rows only when common expressions exist. The original CASE/common recomputation cost remains.
The following exploratory JMH measurements were taken for the initial boundary-reset extraction, before the DIFF state checkpoint was added. These benchmarks contain no DIFF expressions and were not rerun for this commit. Comparison against master 673434b (JDK 17, 512 MiB/G1, 3 x 1 s warmup, 4 x 1 s measurement, two runs per version in baseline/new/new/baseline order):
All ten benchmark shapes (five scenarios x 1/1000 rows) validated every result across 16 blocks before measurement. Timings were noisy on the local machine, especially the wide filter scenario; these numbers are not evidence of a general speedup or absence of regressions.
The CASE/common allocation increase was stable: +192 bytes per one-row block and +9176 bytes per 1000-row block. A deterministic counting probe explains the extra work: shared x + 1 was computed 17 times across 16 blocks on master and 32 times here. Master partly benefits from a leftover cache whose predicate happens to remain true for these benchmark inputs; the reset removes that stale-cache reuse, while CASE still clears the result before common materialization. Expensive expressions can therefore amplify this cost.