github-actions[bot] commented on code in PR #67783:
URL: https://github.com/apache/doris/pull/67783#discussion_r4011898008
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/CheckPolicy.java:
##########
@@ -105,9 +100,7 @@ public List<Rule> buildRules() {
relatedPolicy.rowPolicyFilter.ifPresent(expression
-> combineFilter.addAll(
ExpressionUtils.extractConjunctionToSet(expression)));
Plan result = upAgg != null ?
upAgg.withChildren(child) : child;
- if (upperFilter != null) {
-
combineFilter.addAll(upperFilter.getConjuncts());
- }
+ combineFilter.addAll(upperFilterConjuncts);
Review Comment:
[P1] Preserve the reconstruction filter as a row-policy barrier
This combines the row-policy predicate with the peeled reconstruction
predicates in one physical filter, so the operation/TSO predicate does not
prevent an error-raising row policy from observing rows outside the historical
image. For example, start with `(1, 1)`, capture the snapshot, update it to
`(1, -1)`, and use `USING (assert_true(k2 >= 0, 'bad'))`. The MIN_DELTA branch
produces `UPDATE_BEFORE(v=1)` plus `UPDATE_AFTER(v=-1)` and should discard the
latter, but BE evaluates each conjunct over the full block before ANDing their
masks, so `assert_true` still sees `-1` and aborts the historical query. The
base branch has the same failure for a post-snapshot row and `commit_tso`.
This is distinct from the fixed data-mask thread: the non-movable expression
is now in the row-policy `Filter`, not the mask `Project`. Please retain a
non-mergeable execution barrier so reconstruction runs before row-policy
evaluation, and add a post-snapshot/error-policy regression.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindRelation.java:
##########
@@ -625,9 +630,9 @@ private LogicalPlan buildMowTimeTravelUnion(LogicalOlapScan
baseScan, OlapTable
new TableScanParams(TableScanParams.INCREMENTAL_READ,
incrParams, Lists.newArrayList()));
LogicalPlan right = checkAndAddChangeScanFilter(binlogScan,
StreamScanType.MIN_DELTA, true);
- right = projectFromOriginSlots(right, visibleOutput);
+ right = projectFromUnboundSlots(new LogicalCheckPolicy<>(right),
visibleOutput);
Review Comment:
[P1] Map origin policy columns before checking the binlog branch
This marker retrieves the origin table's policy but binds it against the
row-binlog schema, and those schemas do not contain the same set of columns.
For example, a row-binlog MOW table has hidden `__DORIS_COMMIT_TSO_COL__` in
its full origin schema, so `CREATE ROW POLICY ... USING
(__DORIS_COMMIT_TSO_COL__ > 0)` passes the current `tableIf.getColumn`
validation and binds on ordinary scans. `generateTableRowBinlogSchema`,
however, drops invisible non-key origin columns, so this right branch has only
the distinct binlog TSO/operation slots and fails policy binding with `Unknown
column`.
Please resolve the origin policy against a semantic origin-to-branch column
mapping (or explicitly reject an unreconstructable policy without dropping it),
and add a hidden-origin-column policy regression for MOW time travel.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindRelation.java:
##########
@@ -1068,26 +1072,19 @@ public static LogicalPlan
checkAndAddChangeScanFilter(LogicalOlapScan scan,
if (scanType.equals(StreamScanType.APPEND_ONLY)) {
Preconditions.checkArgument(opSlot != null, "opSlot is null");
return new LogicalFilter<>(ImmutableSet.of(new EqualTo(opSlot,
- new BigIntLiteral(BinlogUtils.ROW_BINLOG_APPEND))), plan);
+ new BigIntLiteral(BinlogUtils.ROW_BINLOG_APPEND))), scan);
} else if (beforeImageOnly) {
return new LogicalFilter<>(ImmutableSet.of(new InPredicate(opSlot,
ImmutableList.of(
new BigIntLiteral(BinlogUtils.ROW_BINLOG_DELETE),
- new
BigIntLiteral(BinlogUtils.ROW_BINLOG_UPDATE_BEFORE)))), plan);
+ new
BigIntLiteral(BinlogUtils.ROW_BINLOG_UPDATE_BEFORE)))), scan);
}
- return plan;
+ return scan;
}
- private LogicalPlan projectFromOriginSlots(LogicalPlan plan, List<Slot>
wantedSlots) {
- Map<String, Slot> childSlotByName = new
HashMap<>(plan.getOutput().size());
- for (Slot slot : plan.getOutput()) {
- childSlotByName.put(slot.getName(), slot);
- }
+ private LogicalPlan projectFromUnboundSlots(LogicalPlan plan,
List<UnboundSlot> wantedSlots) {
List<NamedExpression> project = new ArrayList<>(wantedSlots.size());
- for (Slot wanted : wantedSlots) {
- Slot match = childSlotByName.get(wanted.getName());
- Preconditions.checkArgument(match != null,
- "column %s not found in child output", wanted.getName());
- project.add(new Alias(match, wanted.getName()));
+ for (UnboundSlot wanted : wantedSlots) {
+ project.add(new Alias(wanted, wanted.getName()));
Review Comment:
[P1] Keep the raw column name when creating the alias
`UnboundSlot.getName()` is a rendered identifier, not the raw name: for a
legal ordinary column declared as `` `v1.v2` INT ``, it returns `` `v1.v2` ``
including the backticks. Passing that string to `Alias` stores the backticks as
part of the output name; the union and outer subquery alias preserve them, so a
parsed reference such as `` t.`v1.v2` `` looks for raw `v1.v2` and fails to
bind. The replaced bound-slot helper used `Slot.getName()` and did not change
the catalog name.
Please carry the raw origin name separately (or use the single raw
`nameParts` element) and add a MOW time-travel regression that selects a dotted
primitive column, both qualified and unqualified.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
