Repository navigation
Fix tree-model NOT predicate pushdown - #18982
Conversation
Remove NOT from tree-model WHERE and HAVING predicates before binding and pushdown, including the templated device path. Prevent non-invertible NOT predicates from reaching TsFile filters.
JackieTien97
left a comment
JackieTien97
left a comment
There was a problem hiding this comment.
Thanks for tracking this down. Rewriting NOT away before schema binding is the right direction. With this PR, WHERE NOT (...) over invertible predicates works on the raw, ALIGN BY DEVICE and aligned-template paths. It also fixes a few problems on master that the description doesn't mention:
- On the non-ALIGN BY DEVICE and GROUP BY LEVEL paths, composite
HAVING NOT (A AND B)returned an empty set. WHERE NOT (missing_series > 5) ALIGN BY DEVICEthrew an NPE.
I'm requesting changes because the rewrite, as implemented, also introduces regressions and semantic changes. Details and repros are in the inline comments.
- Regression.
ReversePredicateVisitorcannot invert some operands: a BOOLEAN series, a boolean aggregation,CASE WHEN, or a function/UDF.NOTover any of these now fails during analysis with anUnsupportedOperationExceptionthat has no message (305 ... executeStatement failed. null). This includes WHERE and HAVING queries that work on master. - NaN semantics. For FLOAT/DOUBLE NaN,
NOT (x > c)is not equivalent tox <= c. Queries that work on master now silently return different rows. - Silent wrong result with nested negation.
ReversePredicateVisitor#visitLogicNotExpressionleaves a residualNOT, which the binder then splits. As a result,NOT (NOT (NOT (A AND B)))is evaluated asNOT A AND NOT B. - Tests don't cover the fix. The template IT uses a non-aligned template, and both HAVING ITs already pass on master. There are no cases for non-invertible
NOT, nestedNOT, or NaN. Any of these would have caught the issues above.
As far as I can tell, two of the HAVING hunks don't fix anything and only add the regression in (1): AnalyzeVisitor#analyzeHaving for ALIGN BY DEVICE, and TemplatedAggregationAnalyze#analyzeHaving.
Not blocking, but worth calling out in the PR description: for a series that doesn't exist on the device, WHERE NOT (s2 > 5) used to return all rows (and threw an NPE with ALIGN BY DEVICE). It now returns an empty set. This matches three-valued logic and the behaviour of the positive predicate, but it is a user-visible change and deserves a test.
How I verified. I built this PR (8643565) and its merge-base with master (1b9e071), then ran the same SQL against a 1C1D instance, once with each DataNode jar. PredicateUtilsTest passes. The inline comments use this setup:
CREATE DATABASE root.rv;
CREATE TIMESERIES root.rv.d1.s1 WITH DATATYPE=INT32, ENCODING=PLAIN;
CREATE TIMESERIES root.rv.d1.s2 WITH DATATYPE=INT32, ENCODING=PLAIN;
CREATE TIMESERIES root.rv.d1.b WITH DATATYPE=BOOLEAN, ENCODING=PLAIN;
INSERT INTO root.rv.d1(time, s1, s2, b) VALUES (1, 1, 10, true);
INSERT INTO root.rv.d1(time, s1, s2, b) VALUES (2, 6, 20, false);
INSERT INTO root.rv.d1(time, s1, s2, b) VALUES (3, 3, 30, true);
INSERT INTO root.rv.d1(time, s1, s2, b) VALUES (4, 8, 40, false);
CREATE TIMESERIES root.rv.d2.s1 WITH DATATYPE=INT32, ENCODING=PLAIN;
INSERT INTO root.rv.d2(time, s1) VALUES (1, 2);
INSERT INTO root.rv.d2(time, s1) VALUES (2, 7);
CREATE ALIGNED TIMESERIES root.rv.ad(s1 INT32, b BOOLEAN);
INSERT INTO root.rv.ad(time, s1, b) ALIGNED VALUES (1, 1, true);
INSERT INTO root.rv.ad(time, s1, b) ALIGNED VALUES (2, 6, false);
-- non-aligned template
CREATE DATABASE root.tn;
CREATE DEVICE TEMPLATE tn_t (s1 INT64, b BOOLEAN);
SET DEVICE TEMPLATE tn_t TO root.tn.d1;
SET DEVICE TEMPLATE tn_t TO root.tn.d2;
INSERT INTO root.tn.d1(time, s1, b) VALUES (1, 3, true);
INSERT INTO root.tn.d1(time, s1, b) VALUES (2, 8, false);
INSERT INTO root.tn.d2(time, s1, b) VALUES (1, 4, false);
INSERT INTO root.tn.d2(time, s1, b) VALUES (2, 9, true);
-- aligned template
CREATE DATABASE root.ta;
CREATE DEVICE TEMPLATE ta_t ALIGNED (s1 INT64, b BOOLEAN);
SET DEVICE TEMPLATE ta_t TO root.ta.d1;
SET DEVICE TEMPLATE ta_t TO root.ta.d2;
INSERT INTO root.ta.d1(time, s1, b) ALIGNED VALUES (1, 3, true);
INSERT INTO root.ta.d1(time, s1, b) ALIGNED VALUES (2, 8, false);
INSERT INTO root.ta.d2(time, s1, b) ALIGNED VALUES (1, 4, false);
INSERT INTO root.ta.d2(time, s1, b) ALIGNED VALUES (2, 9, true);| Query | master (1b9e071) | this PR |
|---|---|---|
SELECT s1 FROM root.rv.d1 WHERE NOT (s1 > 5) |
301 This predicate contains a not! |
t1, t3 (fixed) |
SELECT s1 FROM root.ta.* WHERE NOT (s1 > 5) ALIGN BY DEVICE |
301 This predicate contains a not! |
2 rows (fixed) |
SELECT count(s1), sum(s2) FROM root.rv.d1 GROUP BY ([1,5),2ms) HAVING NOT (count(s1) > 1 AND sum(s2) > 50) |
empty (wrong) | 1 row (fixed) |
SELECT s1, b FROM root.rv.d1 WHERE NOT b OR s1 > 5 |
t2, t4 | 305 null (regression) |
SELECT s1, b FROM root.tn.* WHERE NOT b ALIGN BY DEVICE |
2 rows | 305 null (regression) |
SELECT count(s1) FROM root.rv.d1 GROUP BY ([1,5),1ms) HAVING NOT last_value(b) |
2 rows | 305 null (regression) |
SELECT s1, s2 FROM root.rv.d1 WHERE NOT (NOT (NOT (s1 > 5 AND s2 > 25))) |
301 This predicate contains a not! |
t1 only (wrong, expected t1, t2, t3) |
SELECT x, y FROM root.rv.d3 WHERE NOT (x > 5 OR y > 2) (x is NaN at t1, setup inline) |
t1, t2 | t2 (changed) |
| return; | ||
| } | ||
| Expression wherePredicate = | ||
| PredicateUtils.predicateRemoveNot(queryStatement.getWhereCondition().getPredicate()); |
There was a problem hiding this comment.
predicateRemoveNot throws when the NOT wraps something ReversePredicateVisitor can't invert. A BOOLEAN series, a function/UDF, CASE WHEN and a constant all fall through to ReversePredicateVisitor#visitExpression, which throws new UnsupportedOperationException() with no message.
This now runs on every WHERE predicate (here, at L1608, and in TemplatedAnalyze at L269). On master, the queries below work because the conjunct is never pushed into a scan. With this PR they fail during analysis (setup in the review summary):
-- non-aligned d1: the conjunct references two series, so it is never pushed down
SELECT s1, b FROM root.rv.d1 WHERE NOT b OR s1 > 5
-- master: t2, t4 this PR: 305 ... executeStatement failed. null
SELECT s1, b FROM root.rv.d1 WHERE NOT (CASE WHEN s1 > 5 THEN true ELSE false END) OR s2 > 35
-- master: t1, t3, t4 this PR: 305 ... null
-- non-aligned template: predicates are never pushed down on this path
SELECT s1, b FROM root.tn.* WHERE NOT b ALIGN BY DEVICE
-- master: (2, root.tn.d1, 8, false), (1, root.tn.d2, 4, false) this PR: 305 ... nullSELECT s1 FROM root.rv.d1 WHERE NOT b already fails on master, but with a clear 301 message. With this PR it becomes a 305 with the message null, and the DataNode logs an ERROR stack:
java.lang.UnsupportedOperationException
at ...ReversePredicateVisitor.visitExpression(ReversePredicateVisitor.java:45)
at ...PredicateUtils.reversePredicate(PredicateUtils.java:289)
at ...PredicateUtils.predicateRemoveNot(PredicateUtils.java:273)
at ...AnalyzeVisitor.analyzeWhere(AnalyzeVisitor.java:1580)
This also means these predicates never reach the new PredicateCanPushDownToSourceChecker#visitLogicNotExpression branch. So the description's claim that non-invertible NOT predicates remain in FilterOperator doesn't hold yet.
Suggestion: when the operand can't be inverted, keep NOT x instead of throwing (for example, return new LogicNotExpression(x)), and let the new checker keep it in FilterOperator.
There was a problem hiding this comment.
Addressed. The rewrite now runs after schema binding and ReversePredicateVisitor retains NOT when the operand is not safely invertible.
| UnaryOperator<Expression> havingExpressionAnalyzer, | ||
| MPPQueryContext queryContext) { | ||
| Expression havingPredicate = | ||
| PredicateUtils.predicateRemoveNot(queryStatement.getHavingCondition().getPredicate()); |
There was a problem hiding this comment.
HAVING hits the same exception. HAVING is never pushed down, so HAVING NOT <boolean aggregation> works on master:
SELECT count(s1) FROM root.rv.d1 GROUP BY ([1,5),1ms) HAVING NOT last_value(b)
-- master: (2, 1), (4, 1) this PR: 305 ... nullThat said, the rewrite here does fix a real bug. bindSchemaForPredicate(..., isRoot = true) splits NOT (A AND B) into NOT A AND NOT B, because isRoot passes through the NOT. As a result, these two queries return an empty set on master and the correct rows with this PR:
SELECT count(s1), sum(s2) FROM root.rv.d1 GROUP BY ([1,5),2ms) HAVING NOT (count(s1) > 1 AND sum(s2) > 50)
-- master: empty this PR: (1, 2, 30.0)
SELECT count(s1) FROM root.rv.* GROUP BY ([1,5),2ms), LEVEL=1 HAVING NOT (count(s1) > 4 AND count(s1) < 6)
-- master: empty this PR: (1, 6), (3, 2)So this hunk should stay, just with a NOT removal that doesn't throw.
There was a problem hiding this comment.
Fixed at the root instead. BindSchemaForPredicateVisitor now resets the root context when descending through unary expressions, so NOT (A AND B) is no longer split into NOT A AND NOT B. Composite HAVING tests were added for both normal and GROUP BY LEVEL paths.
There was a problem hiding this comment.
For the record, the HAVING NOT last_value(b) coverage I mentioned in my previous reply was not actually in the PR at that point. It is there now: both the raw and the ALIGN BY DEVICE variants landed in 1b9ddcf, and the aligned-template variant too.
|
|
||
| Expression havingExpression = queryStatement.getHavingCondition().getPredicate(); | ||
| Expression havingExpression = | ||
| PredicateUtils.predicateRemoveNot(queryStatement.getHavingCondition().getPredicate()); |
There was a problem hiding this comment.
I don't think this hunk is needed. ConcatDeviceAndBindSchemaForHavingVisitor doesn't split a root AND, and HAVING is never pushed down, so composite NOT already gives the right answer on master. For example, SELECT count(s1), sum(s2) FROM root.rv.d1 GROUP BY ([1,5),2ms) HAVING NOT (count(s1) > 1 AND sum(s2) > 50) ALIGN BY DEVICE returns (1, root.rv.d1, 2, 30.0) on both master and this PR.
The only observable effect of the rewrite here is the regression:
SELECT count(s1) FROM root.rv.d1 GROUP BY ([1,5),1ms) HAVING NOT last_value(b) ALIGN BY DEVICE
-- master: (2, root.rv.d1, 1), (4, root.rv.d1, 1) this PR: 305 ... nullI'd suggest reverting this hunk.
There was a problem hiding this comment.
Removed this hunk. The ALIGN BY DEVICE HAVING path now relies on the binder root-context fix, and HAVING NOT last_value(bool) is covered by tests.
| Set<Expression> aggregationExpressions = analysis.getAggregationExpressions(); | ||
| Expression havingExpression = queryStatement.getHavingCondition().getPredicate(); | ||
| Expression havingExpression = | ||
| PredicateUtils.predicateRemoveNot(queryStatement.getHavingCondition().getPredicate()); |
There was a problem hiding this comment.
Same situation as the ALIGN BY DEVICE HAVING path. The templated HAVING expression is never bound by a root-splitting binder and is never pushed down, so composite NOT already works on master. For example, SELECT count(s1) FROM root.ta.* GROUP BY ([1,3),1ms) HAVING NOT (count(s1) > 0 AND last_value(s1) > 5) ALIGN BY DEVICE returns the same rows on both.
With this change, the following aligned-template query fails:
SELECT count(s1) FROM root.ta.* GROUP BY ([1,3),1ms) HAVING NOT last_value(b) ALIGN BY DEVICE
-- master: (2, root.ta.d1, 1), (1, root.ta.d2, 1) this PR: 305 ... nullI'd suggest reverting this hunk as well.
There was a problem hiding this comment.
Removed this hunk. The aligned-template HAVING path is unchanged, and unsupported boolean HAVING predicates no longer go through predicateRemoveNot.
| if (!queryStatement.hasWhere()) { | ||
| return; | ||
| } | ||
| Expression wherePredicate = |
There was a problem hiding this comment.
For FLOAT/DOUBLE NaN, inverting the comparison is not equivalent to NOT. Both the column transformers and the pushed-down TsFile filters evaluate every ordered comparison on NaN to false. So NOT (NaN > 5) is true, but NaN <= 5 is false; the same holds for <, <= and >=.
The rewrite runs before schema binding, so it can't take the data type into account. On master, the queries below are evaluated in FilterOperator; with this PR they silently return different rows:
CREATE TIMESERIES root.rv.d3.x WITH DATATYPE=DOUBLE, ENCODING=PLAIN;
CREATE TIMESERIES root.rv.d3.y WITH DATATYPE=INT32, ENCODING=PLAIN;
INSERT INTO root.rv.d3(time, x, y) VALUES (1, NaN, 1);
INSERT INTO root.rv.d3(time, x, y) VALUES (2, 3.0, 2);
INSERT INTO root.rv.d3(time, x, y) VALUES (3, 10.0, 3);
SELECT x, y FROM root.rv.d3 WHERE NOT (x > 5 OR y > 2)
-- master: t1 (NaN, 1), t2 (3.0, 2) this PR: t2 only
SELECT x, y FROM root.rv.d3 WHERE NOT (x + y > 5)
-- master: t1, t2 this PR: t2 onlyI see two options:
- Don't invert ordered comparisons whose operands may be FLOAT/DOUBLE. This needs type information, so the rewrite would have to happen after binding.
- Treat this as an intentional semantic change: document it and add tests.
NaN filtering is already covered by tests such as IoTDBFilterIT#testFilterNaN.
There was a problem hiding this comment.
Addressed. Ordered comparisons are only inverted when both operand types are known and non-FLOAT/DOUBLE. NaN cases retain NOT and are evaluated by FilterOperator; regression tests cover both NOT (x > 5 OR y > 2) and NOT (x + y > 5) with NaN.
| List<Expression> conJunctions = | ||
| ExpressionAnalyzer.bindSchemaForPredicate( | ||
| queryStatement.getWhereCondition().getPredicate(), | ||
| wherePredicate, |
There was a problem hiding this comment.
predicateRemoveNot doesn't remove every NOT when negations are nested. ReversePredicateVisitor#visitLogicNotExpression returns the child unchanged, so predicateRemoveNot(NOT (NOT (NOT (A AND B)))) is still NOT (A AND B).
That residual NOT reaches bindSchemaForPredicate at the root. There, isRoot leaks through the NOT because CartesianProductVisitor#getResultsFromChild reuses the same context. The AND is therefore split, and the predicate becomes NOT A AND NOT B. On master this query errors out; with this PR it returns a wrong result and no error:
SELECT s1, s2 FROM root.rv.d1 WHERE NOT (NOT (NOT (s1 > 5 AND s2 > 25)))
-- expected (same as WHERE s1 <= 5 OR s2 <= 25): t1, t2, t3
-- this PR: t1 onlyThe HAVING variant, ... HAVING NOT (NOT (NOT (count(s1) > 1 AND sum(s2) > 50))), returns an empty set instead of one row.
Suggestion: have visitLogicNotExpression return PredicateUtils.predicateRemoveNot(logicNotExpression.getExpression()).
Fixing how isRoot propagates through unary expressions in BindSchemaForPredicateVisitor would go further and remove this whole class of bugs at the source. It is also the root cause of the composite NOT issue this PR works around.
There was a problem hiding this comment.
Addressed. Nested NOT is now recursively normalized, and the binding root-context leak through unary expressions is fixed. The triple-negation query is covered by an integration test.
| final ISchemaTree schemaTree, | ||
| final MPPQueryContext queryContext) { | ||
| Expression wherePredicate = | ||
| PredicateUtils.predicateRemoveNot(queryStatement.getWhereCondition().getPredicate()); |
There was a problem hiding this comment.
analyzeDeviceToWhere calls analyzeWhereSplitByDevice once per device, so the same predicate is rewritten N times for N devices. predicateRemoveNot also rebuilds the whole AND/OR spine even when the predicate contains no NOT. At a minimum, this should be hoisted out of the device loop.
More generally, I'd suggest normalizing NOT once, at the start of analyzeGlobalTimeFilter, instead of at six consumer sites; analyzeGlobalTimeFilter already installs a working predicate through mutationJournal. Besides removing the duplication, this would let extractGlobalTimePredicate see the rewritten predicate.
Today, WHERE NOT (time > 2 OR s1 > 5) is equivalent to time <= 2 AND s1 <= 5. But extraction runs on the original predicate, so the LOGIC_NOT branch yields no global time predicate. As a result, partition fetching (getTimePartitionSlotList(context.getGlobalTimeFilter(), ...)) covers the full time range.
There was a problem hiding this comment.
Moved away from the six pre-binding call sites. Rewriting is now type-aware and happens after binding, which is required to preserve NaN semantics; the duplicate binder bug is fixed directly. This necessarily runs on each device's bound predicate in the ALIGN BY DEVICE path.
There was a problem hiding this comment.
Good catch on the partition-pruning half — that part is now fixed, not just described.
PredicateUtils#extractGlobalTimePredicate now applies De Morgan when it meets a NOT: NOT (A OR B) is NOT (A) AND NOT (B), so every disjunct that is a pure time predicate still bounds the time range of the negated predicate. WHERE NOT (time > 2 OR s1 > 5) therefore extracts NOT (time > 2), which predicateRemoveNot turns into time <= 2, so partition fetching is limited to the partitions overlapping that range. Nested disjunctions are handled recursively.
Deliberately not extracted: NOT (A AND B) (NOT A OR NOT B is a disjunction, so it cannot bound time) and NOT NOT (A OR B) (a disjunction again).
On hoisting the rewrite out of the device loop: I kept it per device, because the rewrite is type-aware and needs each device's bound predicate — a series missing on one device becomes NullOperand, and the comparison operand types come from binding. The residual predicate and hasValueFilter are unchanged in the new extraction branch, so the mutationJournal handling and the later type-aware rewrite are untouched.
Covered by PredicateUtilsTest#testMixedNotOrExtractsNegatedTimePredicate, #testNestedMixedNotOrExtractsAllNegatedTimePredicates, #testNegatedOrWithoutPureTimeDisjunctDoesNotExtractTimePredicate and #testDoubleNegatedOrDoesNotExtractTimePredicate (34c39ad).
| "INSERT INTO " + DEVICE_ID + "(time, int32_col, int64_col) VALUES(10, -30, -30)" | ||
| "INSERT INTO " + DEVICE_ID + "(time, int32_col, int64_col) VALUES(10, -30, -30)", | ||
| "CREATE DATABASE " + TEMPLATE_DATABASE_NAME, | ||
| "CREATE DEVICE TEMPLATE not_predicate_template (s1 INT64)", |
There was a problem hiding this comment.
This template is non-aligned. For non-aligned templates, PredicatePushDown never pushes the predicate down: visitSeriesScan and visitFullOuterTimeJoin return early when isBuildPlanUseTemplate(). So testTemplatedWhereWithNot already passes on master and doesn't exercise the TemplatedAnalyze change. The failure only reproduces with an aligned template:
CREATE DEVICE TEMPLATE ta_t ALIGNED (s1 INT64, b BOOLEAN);
SET DEVICE TEMPLATE ta_t TO root.ta.d1;
SET DEVICE TEMPLATE ta_t TO root.ta.d2;
INSERT INTO root.ta.d1(time, s1, b) ALIGNED VALUES (1, 3, true);
INSERT INTO root.ta.d1(time, s1, b) ALIGNED VALUES (2, 8, false);
INSERT INTO root.ta.d2(time, s1, b) ALIGNED VALUES (1, 4, false);
INSERT INTO root.ta.d2(time, s1, b) ALIGNED VALUES (2, 9, true);
SELECT s1 FROM root.ta.* WHERE NOT (s1 > 5) ALIGN BY DEVICE
-- master: 301 This predicate contains a not! this PR: (1, root.ta.d1, 3), (1, root.ta.d2, 4)
SELECT count(s1) FROM root.ta.* WHERE NOT (s1 > 5) ALIGN BY DEVICE
-- master: 305 ... This predicate contains a not! this PR: (root.ta.d1, 1), (root.ta.d2, 1)Could you use an ALIGNED template here, or add a second case that uses one?
There was a problem hiding this comment.
Changed the template to ALIGNED and added a residual boolean NOT case on that aligned template.
| resultSetEqualTest( | ||
| "SELECT count(int32_col) FROM " | ||
| + DEVICE_ID | ||
| + " GROUP BY ([1,11),2ms) HAVING NOT (count(int32_col) > 1)", |
There was a problem hiding this comment.
Both HAVING NOT (count(int32_col) > 1) cases (with and without ALIGN BY DEVICE) already pass on master, so they don't cover the HAVING change. The HAVING bug this PR actually fixes is a composite NOT on the non-ALIGN BY DEVICE and GROUP BY LEVEL paths (see the comment on analyzeHavingBase).
It would be good to add:
- composite HAVING, e.g.
HAVING NOT (count(...) > 1 AND sum(...) > 50), with and withoutGROUP BY LEVEL - non-invertible
NOT:WHERE NOT bool_series,WHERE NOT bool_series OR s1 > 5,HAVING NOT last_value(bool_series) - nested negation:
WHERE NOT (NOT (NOT (A AND B))) - a FLOAT/DOUBLE series containing NaN under
NOT
Any of the last three would have caught the issues raised in the other comments. PredicateUtilsTest#testLogicNotCannotPushDownToSource covers only the checker, not the analyzer path.
There was a problem hiding this comment.
Added the requested coverage: composite HAVING with and without GROUP BY LEVEL, non-invertible boolean series/functions/CASE, triple negation, NaN, and aligned-template predicates.
There was a problem hiding this comment.
The missing HAVING NOT last_value(bool_series) coverage is added now — that was the one item from your list I had missed. Added in 1b9ddcf:
testHavingNotWithBooleanAggregation:HAVING NOT last_value(b)withGROUP BY ([1,5),1ms), plus the same query withALIGN BY DEVICE.testTemplatedHavingNotWithBooleanAggregation: the aligned-template path,SELECT count(s1) FROM root.test_not_template.* GROUP BY ([1,3),1ms) HAVING NOT last_value(b) ALIGN BY DEVICE.
The other cases you listed were already in the PR: composite HAVING with and without GROUP BY LEVEL, WHERE NOT bool_series, WHERE NOT bool_series OR s1 > 5, NOT (CASE WHEN ...) OR ..., triple negation and the NaN cases.
Verified locally on the latest head: PredicateUtilsTest 13/13 and IoTDBPredicateConversionTreeIT 11/11 (including the two new tests).
|
|
||
| @Override | ||
| public Boolean visitLogicNotExpression(LogicNotExpression logicNotExpression, Void context) { | ||
| // TsFile filters cannot represent NOT. Predicates that cannot be inverted during analysis must |
There was a problem hiding this comment.
Nit, not blocking:
-
Because of the
predicateRemoveNotissue inAnalyzeVisitor, analysis throws before predicates that can't be inverted ever reach this checker. So this comment doesn't describe the current behaviour. -
LogicalAndVisitorrecurses into comparison operands, function arguments andCASEbranches. As a result, this also stops push-down for conjuncts with a nestedNOT, e.g.(NOT (s1 > 5)) = true.Those were safe before.
PredicatePushIntoScanCheckeronly reachesvisitLogicNotExpressionthrough the top-level AND/OR structure, so for these conjuncts it returnedfalserather than throwing.OperatorTreeGeneratorthen evaluated them in a filter operator directly above the scan.This costs push-down, not correctness. Still, returning
FALSEonly forNOTs in the logical (AND/OR) structure would be more precise.
There was a problem hiding this comment.
Kept the checker conservative for any residual logical NOT: correctness is preferred because TsFile cannot represent it. Such predicates stay in FilterOperator; the comment now matches the actual behavior.
Make NOT rewriting total and type-aware, fix root predicate context propagation through unary expressions, and add regressions for NaN, nested negation, unsupported operands, composite HAVING, and aligned templates.
NOT (A OR B) is NOT(A) AND NOT(B), so every disjunct that is a pure time predicate still bounds the time range of the negated predicate. Hence WHERE NOT (time > 2 OR s1 > 5) now yields time <= 2 for partition pruning instead of scanning every time partition.
Add regressions for HAVING NOT last_value(bool) on the raw, ALIGN BY DEVICE and aligned-template paths, which is the non-invertible HAVING NOT shape that must stay in FilterOperator.
JackieTien97
left a comment
JackieTien97
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround. I re-tested 1b9ddcf against the same setup as my previous review. Every case from that review now behaves correctly:
- non-invertible
NOTin WHERE and HAVING - NaN
- nested negation
- composite HAVING, with and without GROUP BY LEVEL
- the aligned-template cases
PredicateUtilsTest and PredicateRemoveNotTest pass as well.
I'm keeping this at "request changes" because of one remaining issue, which I unfortunately missed in my first pass; it also reproduces on 8643565. When the query reads more than one non-aligned series, NOT (<time condition> OR <value condition>) fails with a NoSuchElementException. Details are inline.
| return predicate; | ||
| analyzeExpressionType(analysis, predicate); | ||
| Expression predicateWithoutNot = | ||
| PredicateUtils.predicateRemoveNot(predicate, analysis::getType); |
There was a problem hiding this comment.
Now that NOT is removed after binding, a negated disjunction that contains a time condition leaves a time-only conjunct in the residual WHERE predicate. For example, NOT (time > 2 OR s1 > 5) becomes time <= 2 AND s1 <= 5. The new De Morgan branch in extractGlobalTimePredicate copies time <= 2 into the global time filter, but the residual still contains it.
When the plan has a FullOuterTimeJoin, PredicatePushDown#extractPushDownConjunctsForEachChild calls PredicateUtils#extractPredicateSourceSymbol for every conjunct. That method assumes the conjunct references at least one series, so pathIterator.next() throws on the time-only conjunct.
Repro (same setup as my first review):
SELECT s1, s2 FROM root.rv.d1 WHERE NOT (time > 2 OR s1 > 5)
-- expected: (1, 1, 10) this PR: 305 ... executeStatement failed. null
SELECT s1, s2 FROM root.rv.d1 WHERE NOT (time > 2 OR s1 > 5) ALIGN BY DEVICE
-- expected: (1, root.rv.d1, 1, 10) this PR: 305 ... null
SELECT s1, s2 FROM root.rv.d1 WHERE NOT (time > 2 OR s1 > 5) AND s2 > 5
-- expected: (1, 1, 10) this PR: 305 ... null
SELECT s1, s2 FROM root.rv.d1 WHERE NOT ((time > 2 AND time < 4) OR s1 > 5)
-- expected: (1, 1, 10) this PR: 305 ... nulljava.util.NoSuchElementException
at java.base/java.util.HashMap$HashIterator.nextNode(HashMap.java:1607)
at java.base/java.util.HashMap$KeyIterator.next(HashMap.java:1628)
at ...PredicateUtils.extractPredicateSourceSymbol(PredicateUtils.java:487)
at ...PredicatePushDown$Rewriter.extractPushDownConjunctsForEachChild(PredicatePushDown.java:201)
at ...PredicatePushDown$Rewriter.visitFullOuterTimeJoin(PredicatePushDown.java:145)
These queries only work today in two cases:
- The query reads a single series. The conjuncts then go through
visitSeriesScanSource; for example,SELECT s1 FROM root.rv.d1 WHERE NOT (time > 2 OR s1 > 5)pushesTime <= 2 & root.rv.d1.s1 <= 5into the scan. - The device is aligned.
On master these queries fail with 301 This predicate contains a not!, so this isn't a regression. However, NOT (time > ... OR ...) is exactly the shape that 34c39ad targets, and it still fails for multi-series queries.
The underlying bug already exists on master. A time-only conjunct that can't be extracted as a global time filter crashes the same way there, e.g. SELECT s1, s2 FROM root.rv.d1 WHERE time % 2 = 0 or ... WHERE time > 2.5. So I'd suggest fixing it in extractPredicateSourceSymbol: return null when the conjunct references no series, so it stays in the FilterNode. Pushing time-only conjuncts to every child would also work. Either way, both cases are covered. Optionally, after the rewrite, you could also drop time-only conjuncts that are already implied by the global time filter from the residual.
Please also add an IT that reads at least two non-aligned series, e.g. SELECT s1, s2 ... WHERE NOT (time > 2 OR s1 > 5), with and without ALIGN BY DEVICE.
JackieTien97
left a comment
JackieTien97
left a comment
There was a problem hiding this comment.
I re-tested dd072c7 against the same setup as my previous reviews. LGTM, approving.
-
The time-only conjunct crash is fixed. The following now all return the expected rows:
SELECT s1, s2 ... WHERE NOT (time > 2 OR s1 > 5), with and withoutALIGN BY DEVICE- the
... AND s2 > 5variant NOT ((time > 2 AND time < 4) OR s1 > 5)
This also fixes a crash that already existed on master: a time-only conjunct over multiple non-aligned series, e.g.
WHERE time % 2 = 0orWHERE time > 2.5. -
Every case from my earlier reviews still behaves correctly: non-invertible
NOTin WHERE and HAVING, NaN, nested negation, composite HAVING with and without GROUP BY LEVEL, and the aligned-template paths. -
PredicateUtilsTest,PredicateRemoveNotTestandIoTDBPredicateConversionTreeIT(12/12, SimpleIT) pass locally, and CI is green.
One non-blocking nit, fine to leave for a follow-up: DataNodeQueryMessages.GROUP_BY_TIME_CANNOT_BE_REVERSED (in both the en and zh message files) is no longer referenced. ReversePredicateVisitor#visitGroupByTimeExpression now returns a NOT instead of throwing, so the constant can be removed.
predicateRemoveNot() rebuilt the tree even when the operand could not be inverted, so callers comparing instances to detect a rewrite always saw a change and re-simplified, re-normalized and re-analyzed the predicate once per device. Return the original node instead so the existing short-circuit works. Also remove DataNodeQueryMessages.GROUP_BY_TIME_CANNOT_BE_REVERSED (en and zh), which is no longer referenced since ReversePredicateVisitor retains the NOT instead of throwing.
Summary
Tree-model queries such as
SELECT s1 FROM root.sg.d1 WHERE NOT (s1 > 5)failed with:The analyzer left
LOGIC_NOTin the residual predicate. The predicate was then considered pushable to a series scan, but TsFile filters cannot representNOT.Changes
NOTafter schema binding so operand types are available.NOTfor operands that cannot be inverted safely, including boolean series, functions/UDFs,CASE, and ordered FLOAT/DOUBLE comparisons where NaN semantics differ.NOTfrom being pushed to a scan; it remains inFilterOperator.NOT,HAVING NOT,ALIGN BY DEVICE, aligned Device Templates, unsupported operands, nested negation, and NaN.Verification
mvn test -pl iotdb-core/datanode -am -Dtest=AnalyzeTest,PredicatePushDownTest,TemplatedAnalyzeRetryTest,PredicateRemoveNotTest,PredicateUtilsTest -Dsurefire.failIfNoSpecifiedTests=falsemvn verify -DskipUTs -Dit.test=IoTDBPredicateConversionTreeIT -DfailIfNoTests=false -Dfailsafe.failIfNoSpecifiedTests=false -pl integration-test -am -P with-integration-tests