github-actions[bot] commented on code in PR #68331:
URL: https://github.com/apache/doris/pull/68331#discussion_r4064997202
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/NestedColumnPruning.java:
##########
@@ -861,34 +861,49 @@ private static void stripNullSuffixPaths(
}
/**
- * Keep predicate access paths as a subset of final all access paths after
NULL/OFFSET cleanup.
- * Predicate paths are built from filter expressions first, but later
all-path rewrites may drop
- * metadata-only paths or collapse paths to whole-column access. Any
predicate path not present
- * in final all paths must be removed before sending access info to BE.
+ * Reconcile predicate access paths with the final all access paths.
Predicate paths are built
+ * from filter expressions first, but later all-path rewrites drop
redundant paths or collapse
+ * them to whole-column access, so a predicate path can end up outside the
final all paths.
*
- * <p>Examples:
- * <ul>
- * <li>All paths {@code [s]}, predicate paths {@code [s.city.NULL]}
becomes no predicate
- * paths after parent NULL removal.</li>
- * <li>All paths {@code [s.city.NULL, s.zip]}, predicate paths
- * {@code [s.NULL, s.city.NULL]} becomes {@code [s.city.NULL]}.</li>
- * </ul>
+ * <p>A NULL/OFFSET path is dropped when it is no longer one of the all
paths: BE switches the
+ * whole iterator to NULL_MAP_ONLY/OFFSET_ONLY when it sees such a path
and skips the children,
+ * so it must not come back through the predicate paths either.
+ *
+ * <p>Any other path is kept, because BE needs it to read the predicate
columns first and
+ * lazily materialize the rest. It is added to the all paths unless a
wider path already covers
+ * it, e.g. the whole-column path {@code [s]} covers the predicate path
{@code [s.city]}.
*/
- private static void retainPredicatePathsInFinalAllAccessPaths(
+ private static void alignPredicatePathsWithFinalAllAccessPaths(
List<TColumnAccessPath> predicatePaths, List<TColumnAccessPath>
allPaths) {
- if (predicatePaths.isEmpty()) {
- return;
- }
-
List<TColumnAccessPath> toRemove = new ArrayList<>();
for (TColumnAccessPath predicatePath : predicatePaths) {
- if (!allPaths.contains(predicatePath)) {
- toRemove.add(predicatePath);
+ if (isMetaOnlyAccessPath(predicatePath)) {
+ if (!allPaths.contains(predicatePath)) {
+ toRemove.add(predicatePath);
+ }
+ } else if (!isCoveredByAllPath(predicatePath, allPaths)) {
Review Comment:
[P2] Do not forward an unescaped literal `*` struct field
A struct field can legitimately be named `*` (for example from an external
schema, and ordinal `element_at` copies that real name). If the query also
outputs the whole struct, all paths collapse to `[s]`, but this branch leaves
predicate path `[s, *]` because the root covers it. FileScannerV2 parses
predicate paths independently, and
`AccessPathParser::build_struct_children_from_access_node` explicitly rejects
`*` under STRUCT as a reserved array/map traversal token, so a query that
previously read the full struct and evaluated the filter now fails during
scanner setup. Please preserve/escape segment provenance or suppress
unsupported covered subpaths, and add a file-scan regression for this field
name.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/NestedColumnPruning.java:
##########
@@ -861,34 +861,49 @@ private static void stripNullSuffixPaths(
}
/**
- * Keep predicate access paths as a subset of final all access paths after
NULL/OFFSET cleanup.
- * Predicate paths are built from filter expressions first, but later
all-path rewrites may drop
- * metadata-only paths or collapse paths to whole-column access. Any
predicate path not present
- * in final all paths must be removed before sending access info to BE.
+ * Reconcile predicate access paths with the final all access paths.
Predicate paths are built
+ * from filter expressions first, but later all-path rewrites drop
redundant paths or collapse
+ * them to whole-column access, so a predicate path can end up outside the
final all paths.
*
- * <p>Examples:
- * <ul>
- * <li>All paths {@code [s]}, predicate paths {@code [s.city.NULL]}
becomes no predicate
- * paths after parent NULL removal.</li>
- * <li>All paths {@code [s.city.NULL, s.zip]}, predicate paths
- * {@code [s.NULL, s.city.NULL]} becomes {@code [s.city.NULL]}.</li>
- * </ul>
+ * <p>A NULL/OFFSET path is dropped when it is no longer one of the all
paths: BE switches the
+ * whole iterator to NULL_MAP_ONLY/OFFSET_ONLY when it sees such a path
and skips the children,
+ * so it must not come back through the predicate paths either.
+ *
+ * <p>Any other path is kept, because BE needs it to read the predicate
columns first and
+ * lazily materialize the rest. It is added to the all paths unless a
wider path already covers
+ * it, e.g. the whole-column path {@code [s]} covers the predicate path
{@code [s.city]}.
*/
- private static void retainPredicatePathsInFinalAllAccessPaths(
+ private static void alignPredicatePathsWithFinalAllAccessPaths(
List<TColumnAccessPath> predicatePaths, List<TColumnAccessPath>
allPaths) {
- if (predicatePaths.isEmpty()) {
- return;
- }
-
List<TColumnAccessPath> toRemove = new ArrayList<>();
for (TColumnAccessPath predicatePath : predicatePaths) {
- if (!allPaths.contains(predicatePath)) {
- toRemove.add(predicatePath);
+ if (isMetaOnlyAccessPath(predicatePath)) {
+ if (!allPaths.contains(predicatePath)) {
+ toRemove.add(predicatePath);
+ }
+ } else if (!isCoveredByAllPath(predicatePath, allPaths)) {
+ allPaths.add(predicatePath);
}
}
predicatePaths.removeAll(toRemove);
}
+ private static boolean isMetaOnlyAccessPath(TColumnAccessPath accessPath) {
+ return accessPath.getType() == TAccessPathType.META
+ || isDataSkippingOnlyAccessPath(getAccessPathList(accessPath));
Review Comment:
[P2] Preserve literal Variant keys named NULL/OFFSET
This classifies metadata solely from the final path string, but Variant
string keys are copied verbatim. A valid predicate such as `v['NULL'] = 1` or
`v['OFFSET'] = 1` therefore produces an ordinary DATA path ending in this
token. When the query also projects the whole `v`, the final all path is `[v]`
and this branch drops the predicate leaf; on file scans,
`normalizeDataSkippingOnlyAccessPaths` similarly truncates it to `[v]` first.
The scan then loses the independent leaf projection/lazy split that this PR
restores for ordinary keys. Please preserve synthetic-suffix provenance (or
otherwise make this classification path-origin/type aware) and cover both
literal keys.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/NestedColumnPruning.java:
##########
@@ -861,34 +861,49 @@ private static void stripNullSuffixPaths(
}
/**
- * Keep predicate access paths as a subset of final all access paths after
NULL/OFFSET cleanup.
- * Predicate paths are built from filter expressions first, but later
all-path rewrites may drop
- * metadata-only paths or collapse paths to whole-column access. Any
predicate path not present
- * in final all paths must be removed before sending access info to BE.
+ * Reconcile predicate access paths with the final all access paths.
Predicate paths are built
+ * from filter expressions first, but later all-path rewrites drop
redundant paths or collapse
+ * them to whole-column access, so a predicate path can end up outside the
final all paths.
*
- * <p>Examples:
- * <ul>
- * <li>All paths {@code [s]}, predicate paths {@code [s.city.NULL]}
becomes no predicate
- * paths after parent NULL removal.</li>
- * <li>All paths {@code [s.city.NULL, s.zip]}, predicate paths
- * {@code [s.NULL, s.city.NULL]} becomes {@code [s.city.NULL]}.</li>
- * </ul>
+ * <p>A NULL/OFFSET path is dropped when it is no longer one of the all
paths: BE switches the
+ * whole iterator to NULL_MAP_ONLY/OFFSET_ONLY when it sees such a path
and skips the children,
+ * so it must not come back through the predicate paths either.
+ *
+ * <p>Any other path is kept, because BE needs it to read the predicate
columns first and
+ * lazily materialize the rest. It is added to the all paths unless a
wider path already covers
+ * it, e.g. the whole-column path {@code [s]} covers the predicate path
{@code [s.city]}.
*/
- private static void retainPredicatePathsInFinalAllAccessPaths(
+ private static void alignPredicatePathsWithFinalAllAccessPaths(
List<TColumnAccessPath> predicatePaths, List<TColumnAccessPath>
allPaths) {
- if (predicatePaths.isEmpty()) {
- return;
- }
-
List<TColumnAccessPath> toRemove = new ArrayList<>();
for (TColumnAccessPath predicatePath : predicatePaths) {
- if (!allPaths.contains(predicatePath)) {
- toRemove.add(predicatePath);
+ if (isMetaOnlyAccessPath(predicatePath)) {
+ if (!allPaths.contains(predicatePath)) {
+ toRemove.add(predicatePath);
+ }
+ } else if (!isCoveredByAllPath(predicatePath, allPaths)) {
+ allPaths.add(predicatePath);
}
}
predicatePaths.removeAll(toRemove);
}
+ private static boolean isMetaOnlyAccessPath(TColumnAccessPath accessPath) {
+ return accessPath.getType() == TAccessPathType.META
+ || isDataSkippingOnlyAccessPath(getAccessPathList(accessPath));
+ }
+
+ private static boolean isCoveredByAllPath(
+ TColumnAccessPath predicatePath, List<TColumnAccessPath> allPaths)
{
+ for (TColumnAccessPath allPath : allPaths) {
+ if (allPath.getType() == predicatePath.getType()
+ && pathCoversPrefix(getAccessPathList(allPath),
getAccessPathList(predicatePath))) {
Review Comment:
[P1] Normalize the root before testing prefix coverage
Complex access paths are collected with a lower-cased slot root, but a
collapsed whole-column path is rebuilt from the catalog name. For an OLAP
complex column named `S`, projecting `S` while filtering `S.city` therefore
reaches this check as all path `[S]` and predicate path `[s, city]`; this
case-sensitive comparison treats the leaf as uncovered and appends it to the
all paths. BE accepts both roots case-insensitively, removes `[S]` as the
whole-root marker, then uses the remaining `city` all-subpath to mark every
sibling `SKIP_READING`, so the projected whole struct can silently
omit/default-fill sibling values. Please canonicalize the root consistently (or
compare it case-insensitively) and add an uppercase OLAP complex-column
regression that asserts sibling values.
--
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]
