Skip to content

Reset CASE expression caches at input block boundaries - #18889

Open
ColinLeeo wants to merge 7 commits into
apache:masterfrom
ColinLeeo:fix/case-cache-block-boundary
Open

ColinLeeo wants to merge 7 commits into
apache:masterfrom
ColinLeeo:fix/case-cache-block-boundary

Conversation

@ColinLeeo

@ColinLeeo ColinLeeo commented Oct 15, 2026 •

Copy link
Copy Markdown
Collaborator

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

  • Collect each phase's DAG once, deduplicating nodes by identity; reset each node's own cache at the start of a new input.
  • Cover FilterAndProjectOperator, DevicePredicateHandler, and DeviceAttributeUpdater, preserving the original schema constructor descriptors.
  • Apply the final filter mask when an UPDATE common expression must be recomputed, including guarded CAST inputs such as 'n/a'. Query common-expression evaluation retains its existing behavior.
  • Read the selected MultiColumnTransformer input length from the retained column, because consuming the cache can release its stored length.
  • Snapshot the incoming lastValue and null flag for both unary and binary DIFF at each input boundary. Restore that state before transforming the same block again, so CASE cache clearing cannot make a repeated evaluation start from the block's final value. This resolves the shared DIFF failure identified in the review of Fix CASE expression caches leaking across TsBlocks #18826: the second block's common projection returns 4 rather than -1 for inputs [1, 3, 6] followed by [10, 11].
  • Preserve master's DIFF argument evaluation behavior: unary DIFF still requests all rows, while binary DIFF passes the supplied selection to both arguments. In particular, an outer CASE still protects invalid CAST input for binary diff(value, dynamic_boolean). Each DIFF stores only a double and a null flag for the incoming state.
  • Add cross-block/batch, aggregation, effective upper(src) common-expression, guarded UPDATE, nested DIFF, dynamic NULL flags, and outer CASE/CAST compatibility tests. The pending-sibling IT guards existing within-block behavior; it is not the original bug reproduction.

The broader selection-cache redesign from #18826 remains deferred.

Validation

  • 33 calc unit tests and 10 DataNode unit tests passed, including 15 DIFF lifecycle tests and the four existing BlobLength Mockito tests.
  • Ten table-model integration test methods passed: six query methods and four schema methods, including the original three aggregation query variants, the pending-sibling guard, shared unary/binary DIFF across blocks, and binary CASE/CAST protection. The unary literal-flag CAST behavior is also checked against master.
  • A deterministic comparison of 1,200 input blocks matched master's single-evaluation results for unary/binary DIFF, NULL policies, dynamic flags, and selections. Repeating each block on this branch produced the same result; the master implementation reproduced the repeated-evaluation mismatch.
  • Separate baseline/new runs of six schema and six operator probes: eleven failures on master 673434b, all twelve passing on the boundary-reset extraction. The upper(src) test reproduces the null common Column on master.
  • Full reactor test compilation passed in both English and Chinese (53 modules each).
  • Spotless and reactor license checks passed.

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):

1000 rows per block master initial boundary-reset extraction
Addition projection 3.536 us 3.503 us
16 projections sharing an expression 40.932 us 41.549 us
CASE with a common expression 22.749 us 26.916 us

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.

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.
@ColinLeeo
ColinLeeo requested a lite review from Copilot October 15, 2026 10:15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

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
@ColinLeeo
ColinLeeo marked this pull request as ready for review October 22, 2026 02:43
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 JackieTien97 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 / greatest in 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), guarded CAST(src AS INT32)): NPE → correct.

Issues (details inline)

  1. 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.
  2. 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).
  3. 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.
  4. Several ITs assert binary-DIFF values that their own comments describe as inconsistent.

Same bug class, outside the diff (master too)

  • TernaryColumnTransformer.evaluateWithSelection reads the first child after evaluating the other two — the hazard fixed in MultiColumnTransformer here. CASE WHEN x > 0 THEN x BETWEEN x - 1 AND x + 1 ELSE false END over x = 1, 2, -1 returns null, null, false instead of true, true, false. Reading each child's column right after evaluating it (as MultiColumnTransformer.evaluateWithSelection does) 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) and x 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; and UPDATE t SET b = concat(src, CASE WHEN src = 'match' THEN '-m' ELSE '-o' END) reports success but writes NULL to every device (b was '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 END still fails with 701: 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.evaluateWithSelection caches rightColumn instead of builder.build() (LogicOrColumnTransformer caches the result). CASE WHEN s3 > 0 THEN s1 > 0 AND s2 > 0 ELSE false END returns 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' END returns '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 canEvaluateWithSelection walk.

// 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();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 NULL

And 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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.0

Same 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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"}) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants