github-actions[bot] commented on code in PR #69414:
URL: https://github.com/apache/doris/pull/69414#discussion_r4348130457
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/VariantSubPathPruning.java:
##########
@@ -263,17 +264,18 @@ public Plan visitLogicalUnion(LogicalUnion union, Context
context) {
constExprs.get(j).add(union.getConstantExprsList().get(j).get(i));
}
outputs.add(union.getOutputs().get(i));
- // if not variant, no need to process
- if (!union.getOutput().get(i).getDataType().isVariantType()) {
+ // Whole-column reads have no sub-path requests, including in
constant-only unions.
+ if (!originalSlot.getDataType().isVariantType()
+ ||
!context.slotToSubPathsMap.containsKey(originalSlot)) {
continue;
}
+ Set<List<String>> requestedSubPaths =
context.slotToSubPathsMap.get(originalSlot);
// put new slots generated by sub path push down
Map<List<String>, List<SlotReference>> subPathSlots =
Maps.newHashMap();
for (int j = 0; j < regularChildrenOutputs.size(); j++) {
List<SlotReference> regularChildOutput =
union.getRegularChildOutput(j);
Expression output = regularChildOutput.get(i);
- if (!context.slotToSlotsMap.containsKey(output)
- ||
!context.slotToSubPathsMap.containsKey(outputs.get(i))) {
+ if (!context.slotToSlotsMap.containsKey(output)) {
Review Comment:
[P2] Handle branches without path slots before expanding later outputs
A later VARIANT output can now enter expansion even when a branch cannot
supply a path slot. For example, consumers of a twice-referenced CTE can retain
this reduced plan:
```text
Project(A['x'], B['x'])
UnionAll(outputs=[A,B], mappings=[[a1,e],[a2,b2]])
Project(a1,e)
Generate(explode_variant_array(arr) AS e)
OlapScan(a1,arr)
OlapScan(a2,b2)
```
`visitLogicalGenerate` does not register subpath slots for `e`, and a
forwarding project cannot create them. Expansion of `A` succeeds, but expansion
of `B` skips child 0 here; child 1 then supplies `b2['x']` and throws `cannot
find sub path for child 1` because no entry was initialized by child 0.
Reversing the branches instead leaves a short list indexed when appending child
slots. Before this patch, `outputs.get(1)` referred to the inserted `A['x']`
slot, so `B` was left unexpanded and the upper `B['x']` remained valid: this is
a newly exposed failure for later columns, even though the analogous
first-column limitation predates the PR.
Please synthesize missing branch expressions, or retain evaluation above
UNION for that output/path when a branch cannot supply it, and add a regression
with an earlier expandable VARIANT and a later exploded/scan VARIANT pair.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/VariantSubPathPruning.java:
##########
@@ -294,7 +296,7 @@ public Plan visitLogicalUnion(LogicalUnion union, Context
context) {
// because context only record sub path generated by
parent
List<String> parentPaths = entry.getKey()
.subList(subPathByChildren.size(),
entry.getKey().size());
- if
(!context.slotToSubPathsMap.get(outputs.get(i)).contains(parentPaths)) {
+ if (!requestedSubPaths.contains(parentPaths)) {
Review Comment:
[P2] Preserve one evaluation of volatile constant UNION values
The corrected lookup also activates later outputs whose constant-row
expression is volatile. Consider this reduced producer of a twice-referenced
CTE:
```text
UnionAll(outputs=[A,B], regular mappings=[[a,b]])
OlapScan(a,b)
constant row: [parse_to_variant('{"x":1}'), F]
F = parse_to_variant(concat('{"x":"', uuid(), '"}'))
```
When consumers request `A['x']`, whole `B`, and `B['x']`, expansion of `B`
now retains `F` and appends `ElementAt(F,'x')` as a separate constant field.
The UNION source executes those fields in separate expression contexts, so `B`
contains one UUID while its separately returned `B['x']` contains another. This
breaks same-row value consistency and can change predicate results. Before this
patch, the inserted `A['x']` occupied `outputs.get(1)`, so later `B` was left
unexpanded and its path was read from its single materialized value.
`MergeOneRowRelationIntoUnion` allows this row because `uuid()` initially
occurs only once. Volatile-expression extraction runs before this new
duplication and has no UNION handler; physical common-expression optimization
only handles projects, so neither restores sharing for these constant fields.
Please materialize a volatile value once before deriving paths, or retain its
path evaluation above UNION, and add a mixed-input regression checking that a
whole object's field agrees with its separately requested path.
--
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]
