924060929 commented on code in PR #65805:
URL: https://github.com/apache/doris/pull/65805#discussion_r3621131279
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/AccessPathPlanCollector.java:
##########
@@ -401,7 +403,7 @@ static List<CollectAccessPathResult>
normalizeDataSkippingOnlyAccessPaths(
List<CollectAccessPathResult> normalizedAccessPaths = new
ArrayList<>();
for (CollectAccessPathResult accessPath : accessPaths) {
List<String> path = accessPath.getPath();
- if (isDataSkippingOnlyAccessPath(path) && path.size() > 1) {
+ if (path.size() > 1 && accessPath.getType() ==
ColumnAccessPathType.META) {
Review Comment:
**Blocking: this regresses nested pruning on external tables.**
I invoked `NestedColumnPruning.pruneDataType` reflectively with the real
`tbl.s` slot (`struct<city:text,
data:array<map<int,struct<a:int,b:double>>>>`), feeding it exactly what the
collector emits for `WHERE element_at(s,'city') IS NULL` — `META:s.NULL` +
`META:s.city.NULL`, the shape this PR's own `testFilter` asserts:
```
[A] OLAP (no normalize) : prunedType=STRUCT<city:TEXT>
all=[META:s.NULL, META:s.city.NULL]
[B] External post-PR (normalized):
prunedType=STRUCT<city:TEXT,data:ARRAY<MAP<INT,STRUCT<a:INT,b:DOUBLE>>>>
all=[DATA:s]
[C] External pre-PR baseline : prunedType=STRUCT<city:TEXT>
all=[DATA:s.city]
```
B vs C is the regression. The newly added struct-parent `META:[s,NULL]` is
stripped here to `DATA:[s]`; length 1 then triggers the whole-column collapse
in `buildColumnAccessPaths` (`accessWholeColumn`), and
`setAccessByPath([s],0,DATA)` sets `accessAll`, so the pruned type reverts as
well. Both channels collapse.
Net effect: any Hive/Iceberg/Paimon query containing `element_at(struct,'f')
IS NULL` reads the full struct — here that drags back the entire
`data:ARRAY<MAP<INT,STRUCT<a,b>>>` subtree. This is exactly the workload nested
pruning exists for.
Suggested fix, in this method: after stripping the marker, drop the result
when it is a **proper prefix** of another path in the same collection — the
deeper path already covers that read. Keep it when there is no deeper path,
since `WHERE s IS NULL` alone still needs the full column (external readers
have no null-map-only mode).
Unrelated nit in the same collapse logic: `buildColumnAccessPaths`
initialises `accessWholeColumnType = ColumnAccessPathType.META`, which is
unreachable — META paths always carry a terminal marker, so their length is >=
2 and the `else` branch always forces DATA. It reads as if a whole-column META
read were supported.
##########
regression-test/suites/nereids_rules_p0/column_pruning/null_column_pruning.groovy:
##########
@@ -220,14 +220,13 @@ suite("null_column_pruning") {
order_qt_12 "select struct_col from ncp_tbl where struct_col is null";
// ─── Nested struct field IS NULL
────────────────────────────────────────────
- // element_at(struct_col, 'city') IS NULL should produce a null-flag-only
- // predicate path [struct_col.city.NULL] while the projection reads city
data.
- // [struct_col.city.NULL] remains in predicateAccessPaths beside the
projected city data path.
+ // element_at(struct_col, 'city') IS NULL needs both the parent Struct
null map and the
+ // selected field null map, while the projection reads city data.
explain {
sql "select element_at(struct_col, 'city') from ncp_tbl where
element_at(struct_col, 'city') is null"
contains "nested columns"
contains "struct_col.city"
- contains "predicate access paths: [struct_col.city.NULL]"
+ contains "predicate access paths: [struct_col.NULL,
struct_col.city.NULL]"
Review Comment:
**These assertions cannot pin the thing this PR introduces.**
EXPLAIN drops the access-path type:
```java
// PlanNode.java:957
.map(a -> StringUtils.join(a.getPath(), "."))
```
`DATA:["s","NULL"]` and `META:["s","NULL"]` both render as `s.NULL`. Every
assertion in this file that mentions `.NULL` or `.OFFSET` would still pass if
FE emitted the wrong type — blind to exactly the DATA/META distinction the PR
is built on.
Suggest rendering the type (`META:struct_col.NULL`) so these assertions
actually constrain it. That also makes the plan output diagnosable for the
`struct<\`null\`:int>` case, where the same string means two different things.
##########
regression-test/suites/nereids_rules_p0/column_pruning/null_column_pruning.groovy:
##########
@@ -183,23 +183,23 @@ suite("null_column_pruning") {
order_qt_10 "select int_col from ncp_tbl where int_col is null";
// ─── Mixed: struct IS NULL + partial field access
───────────────────────────
- // struct_col IS NULL in WHERE + element_at in SELECT needs struct data
for projection, while
- // the predicate keeps the parent null map separately.
+ // struct_col IS NULL in WHERE + element_at in SELECT needs only the
projected field data,
+ // while the predicate keeps the parent null map separately.
explain {
sql "select element_at(struct_col, 'city') from ncp_tbl where
struct_col is null"
contains "nested columns"
- contains "all access paths: [struct_col]"
+ contains "all access paths: [struct_col.city, struct_col.NULL]"
Review Comment:
**The behaviour change here has no data-level coverage.**
Only EXPLAIN assertions changed in this file, and no `.out` file changed
anywhere in the PR. None of the three tables in this suite has a row where a
STRUCT column is itself NULL — `ncp_tbl` holds one row, `named_struct('city',
null, 'zip', 10001)`, which is a non-null struct with a null field.
`ncp_nested_tbl` and `struct_map_pruning_lazy_read` are the same.
So the new two-layer null rule (`element_at(s,'f') IS NULL` always emitting
`[s,NULL]`) is asserted as a plan shape and never as a result. If the previous
behaviour returned wrong rows for a NULL struct, no case here would have failed
before, and none passes differently now.
More importantly, the wrong result this PR actually fixes has no case at
all. Please add, with data and result comparison:
1. `STRUCT<\`null\`:INT>` + plain projection (the repro in the summary).
2. An `offset` variant — `STRUCT<\`offset\`:INT>` takes the `OFFSET_ONLY`
branch rather than the NULL branch, so the symptom may be worse than returning
NULL.
3. `v['NULL']` on a VARIANT column. The variant path never lowercases
(`AccessPathExpressionCollector:356` vs `:397`), so it collides regardless of
case — pre-PR it was misclassified as metadata by the last-component string
rule. This is a real bug this PR fixes, and it is currently untested.
4. A row where the struct column itself is NULL, to give the two-layer rule
actual coverage.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/AccessPathExpressionCollector.java:
##########
@@ -334,6 +362,29 @@ public Void visitElementAt(ElementAt elementAt,
CollectorContext context) {
Expression fieldName = arguments.get(1);
DataType fieldType = fieldName.getDataType();
if (fieldName.isLiteral() && (fieldType.isIntegerLikeType() ||
fieldType.isStringLikeType())) {
+ // element_at(s, 'field') IS NULL has two-layer null semantics:
Review Comment:
The two-layer rule is applied only in the `IS NULL` branch, which makes it
internally inconsistent.
If a struct's own null map is *not* implied by a child path, then these have
identical exposure and get no `[s,NULL]`:
- `length(element_at(s,'f'))` — META OFFSET context, falls through without
emitting the parent path
- plain `element_at(s,'f')` — pruned to `struct<f>`, no parent path either
If it *is* implied — which the long-standing correctness of plain projection
suggests — then `[s,NULL]` is redundant, and it is what triggers the
external-table collapse flagged on `AccessPathPlanCollector.java`.
Both cannot be true. Whichever holds, please state the BE invariant in a
comment here: whether reading any descendant of a nullable struct always
materialises the parent null map. That single sentence decides whether this
block is required or removable.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/AccessPathExpressionCollector.java:
##########
@@ -365,6 +416,7 @@ public Void visitMapKeys(MapKeys mapKeys, CollectorContext
context) {
= new CollectorContext(context.statementContext,
context.bottomFilter);
removeStarContext.accessPathBuilder.accessPath.addAll(suffixPath.subList(1,
suffixPath.size()));
removeStarContext.accessPathBuilder.addPrefix(AccessPathInfo.ACCESS_MAP_KEYS);
+ removeStarContext.setType(context.type);
Review Comment:
**Please confirm this shape.** Measured on this head:
```
SQL: select 1 from str_tbl where map_keys(map_col)[1] is null
all =[META:map_col.KEYS.NULL] pred=[META:map_col.KEYS.NULL]
```
Two concerns:
1. Semantics — `map_keys(m)[1] IS NULL` asks whether the map has a first
entry (an out-of-range subscript yields NULL). The KEYS null map cannot answer
that; offsets are what encode it.
2. Safety — map key sub-columns are not nullable, and the scalar
`NULL_MAP_ONLY` path asserts `DORIS_CHECK(is_column_nullable(*dst))`
(`column_reader.cpp:2708`), which throws in Release and cores in Debug.
The `isUnderIsNull` early return above only covers `map_keys(m) IS NULL`;
the subscripted form arrives with a two-element suffix and falls into this
`ACCESS_ALL` branch instead. This is most likely pre-existing rather than
introduced here, but the PR is touching this exact routing, so it is worth
settling.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/NestedColumnPruning.java:
##########
@@ -624,17 +602,17 @@ public boolean
replacePathByAnotherTree(DataTypeAccessTree cast, List<String> pa
String originFieldName = ((StructType)
type).getFields().get(i).getName();
path.set(index, originFieldName);
return
children.get(originFieldName).replacePathByAnotherTree(
- cast.children.get(castFieldName), path, index
+ 1
+ cast.children.get(castFieldName), path, index
+ 1, pathType
);
}
}
} else if (cast.type instanceof ArrayType) {
return
children.values().iterator().next().replacePathByAnotherTree(
- cast.children.values().iterator().next(), path, index
+ 1);
+ cast.children.values().iterator().next(), path, index
+ 1, pathType);
} else if (cast.type instanceof MapType) {
String fieldName = path.get(index);
return
children.get(AccessPathInfo.ACCESS_MAP_VALUES).replacePathByAnotherTree(
- cast.children.get(fieldName), path, index + 1
+ cast.children.get(fieldName), path, index + 1, pathType
Review Comment:
Pre-existing, but this change makes it load-bearing.
The Map branch hardcodes `children.get(ACCESS_MAP_VALUES)` on the origin
side while resolving the cast side with `fieldName`, which can be `KEYS`. For a
path like `[m, KEYS, OFFSET]` the recursion therefore pairs the origin
**value** subtree against the cast **key** subtree.
The new terminal check then returns `type.equals(cast.type)` on that
mismatched pair. It fails safe — an unequal result just falls back to reading
the original data — but the comparison it performs is not the one intended, and
for `map<string,string>` it returns true by coincidence rather than by
correctness. Worth fixing while this code is open.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/AccessPathExpressionCollector.java:
##########
@@ -164,17 +182,23 @@ public Void visitSlotReference(SlotReference
slotReference, CollectorContext con
// For any other nullable column type (e.g. INT, BIGINT) accessed via
IS NULL / IS NOT NULL:
// record the [col_name, NULL] path so NestedColumnPruning can emit
null-only access paths.
// Skip NestedColumnPrunable types (already handled above) and string
types (handled above).
+ // Check getOriginalColumn() rather than slotReference.nullable(): the
latter may be
+ // inflated by outer join, while the former reflects the physical
column's null map.
+ // Only NULL paths need the null-map check; OFFSET paths don't depend
on nullability.
if (!(dataType instanceof NestedColumnPrunable) &&
!dataType.isStringLikeType()
- && !context.accessPathBuilder.isEmpty() &&
slotReference.nullable()) {
+ && isUnderIsNull(context.accessPathBuilder.accessPath)
Review Comment:
`isUnderIsNull` still decides purely by raw string:
```java
private static boolean isUnderIsNull(List<String> suffixPath) {
return suffixPath.size() == 1 &&
AccessPathInfo.ACCESS_NULL.equals(suffixPath.get(0));
}
```
6 of its 10 call sites carry no `context.type == META` guard (`:189`,
`:409`, `:429`, `:471`, `:492`, `:511`, `:730`); only `:148`, `:165` and `:370`
do. It is currently safe only because a DATA context cannot present a
single-element suffix spelled `"NULL"` — which follows from `StructField`
lowercasing every field name, not from anything local to this file.
Now that the type is carried per path, worth closing here too. And since the
whole design rests on it, the "struct field names are always lowercase, markers
are always uppercase" invariant deserves an explicit comment — `StructField`'s
constructor is the only thing enforcing it, three modules away.
##########
fe/fe-core/src/test/java/org/apache/doris/nereids/rules/rewrite/PruneNestedColumnTest.java:
##########
@@ -1041,6 +1136,51 @@ public void testDataTypeAccessTree() {
);
}
+ @Test
+ public void testDataPathNamedLikeMetadataComponent() {
Review Comment:
This test does not exercise the defect, in two ways.
`new StructField("NULL", ...)` is lowercased to `null` by the constructor —
the test's own `assertEquals("STRUCT<null:TEXT>", ...)` below confirms it. A
struct field literally named `NULL` cannot be constructed.
More importantly, it drives `setAccessByPath` directly, which is the FE side
— and FE was never wrong here. FE compares case-sensitively, so
`"null".equals("NULL")` never matched. The defect was BE's `StringCaseEqual`
consuming `["s","null"]` as a sentinel. **This test passes both before and
after the fix.**
What is missing is an end-to-end case: create the table, insert a row,
select the field, compare results with pruning on and off. Suggested in more
detail on `null_column_pruning.groovy`.
--
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]
