morrySnow commented on code in PR #65805:
URL: https://github.com/apache/doris/pull/65805#discussion_r3765966189
##########
be/src/storage/segment/column_reader.cpp:
##########
@@ -951,44 +1186,122 @@ void
ColumnIterator::_recovery_from_place_holder_column(MutableColumnPtr& dst) {
}
}
-Result<TColumnAccessPaths> ColumnIterator::_get_sub_access_paths(
- TColumnAccessPaths sub_access_paths, bool is_predicate) {
- // Access paths passed to a complex iterator always start with the current
- // column name. Strip that component and return the remaining
child-relative
- // paths to the caller. For example, when this iterator is for column `s`,
- // path `s.a.b` is converted to `a.b` and then dispatched to child `a`.
- //
- // If stripping the current column consumes the whole path, the current
- // iterator itself is requested rather than one of its children. Mark the
- // current iterator according to the path source: predicate paths must be
read
- // in the predicate phase, while all/output paths become lazy output
targets.
- // Empty or mismatched paths indicate an FE/BE access-path contract
violation.
- for (auto it = sub_access_paths.begin(); it != sub_access_paths.end();) {
- TColumnAccessPath& name_path = *it;
- if (name_path.data_access_path.path.empty()) {
+Result<ColumnIterator::AccessPathSplit> ColumnIterator::_split_access_paths(
+ TColumnAccessPaths access_paths) const {
+ AccessPathSplit split;
+ for (auto& path : access_paths) {
+ const bool uses_legacy_encoding =
uses_legacy_access_path_encoding(path);
+ if (!uses_legacy_encoding &&
+ path.version !=
g_Descriptors_constants.TCOLUMN_ACCESS_PATH_VERSION_TYPED) {
+ return ResultError(
+ Status::InternalError("Unsupported access path version:
{}", path.version));
+ }
+
+ std::vector<std::string>* components = nullptr;
+ if (uses_legacy_encoding) {
+ if (path.type != TAccessPathType::DATA) {
+ return ResultError(Status::InternalError("Invalid legacy
access path type: {}",
+
static_cast<int>(path.type)));
+ }
+ if (!path.__isset.data_access_path) {
+ return ResultError(Status::InternalError(
+ "Invalid legacy access path: data_access_path payload
is not set"));
+ }
+ components = &path.data_access_path.path;
+ } else {
+ switch (path.type) {
+ case TAccessPathType::DATA:
+ if (!path.__isset.data_access_path) {
+ return ResultError(Status::InternalError(
+ "Invalid DATA access path: data_access_path
payload is not set"));
+ }
+ components = &path.data_access_path.path;
+ break;
+ case TAccessPathType::META:
+ if (!path.__isset.meta_access_path) {
+ return ResultError(Status::InternalError(
+ "Invalid META access path: meta_access_path
payload is not set"));
+ }
+ components = &path.meta_access_path.path;
+ break;
+ default:
+ return ResultError(Status::InternalError("Invalid access path
type: {}",
+
static_cast<int>(path.type)));
+ }
+ }
+
+ if (components->empty()) {
return ResultError(Status::InternalError(
"Invalid access path for column '{}': path is empty",
_column_name));
}
- if (!StringCaseEqual()(name_path.data_access_path.path[0],
_column_name)) {
+ if (!StringCaseEqual()((*components)[0], _column_name)) {
return ResultError(Status::InternalError(
R"(Invalid access path for column: expected name "{}", got
"{}")", _column_name,
- name_path.data_access_path.path[0]));
+ (*components)[0]));
}
-
name_path.data_access_path.path.erase(name_path.data_access_path.path.begin());
- if (!name_path.data_access_path.path.empty()) {
- ++it;
- } else {
- if (is_predicate) {
- set_read_requirement(ReadRequirement::PREDICATE);
- } else {
- set_lazy_output_requirement();
+ components->erase(components->begin());
+ if (components->empty()) {
+ split.reads_current_data = true;
+ continue;
+ }
+
+ const bool is_current_level_meta =
Review Comment:
Legacy-encoding regression for structs that have a field literally named
`OFFSET` (the PR itself adds `meta_name_tbl struct<`NULL`: string, `OFFSET`:
string>` as a supported schema).
A legacy DATA path `[s, OFFSET]` from an old FE now matches
`is_current_level_meta` here and is consumed as OFFSET_ONLY metadata, so it
never reaches `descendant_paths`. The struct then skips every child
(`need_to_read` is false for all of them in
`StructFileColumnIterator::set_access_paths`), and the field data is silently
not read.
The pre-PR struct `set_access_paths()` behaved differently:
`remove_current_level_meta_access_paths(sub_all_access_paths)` only ran when a
current-level data path existed, so `[OFFSET]` stayed in `sub_all_access_paths`
and was routed to the child field named `OFFSET`; the OFFSET_ONLY mode was
harmless for structs because only NULL_MAP_ONLY skipped children. So `select
element_at(s, 'OFFSET') from t` returned correct data before, and now silently
returns wrong values with old FE + new BE — the exact compatibility case stated
in the PR description ("New BEs continue to decode the legacy all-DATA format
from old FEs") — during the rolling-upgrade window.
For legacy-encoded paths, consider treating a single NULL/OFFSET component
as current-level metadata only when the current struct has no child field with
that exact name, preserving the old routing; the typed-path behavior
(`TypedDataFieldsNamedMetaComponentsAreNotTreatedAsMetaPaths`) already handles
the unambiguous case.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/AccessPathExpressionCollector.java:
##########
@@ -334,6 +362,23 @@ public Void visitElementAt(ElementAt elementAt,
CollectorContext context) {
Expression fieldName = arguments.get(1);
DataType fieldType = fieldName.getDataType();
if (fieldName.isLiteral() && (fieldType.isIntegerLikeType() ||
fieldType.isStringLikeType())) {
+ // Only emit META [s, field, NULL] when the selected field
itself is nullable.
+ if (context.type == ColumnAccessPathType.META
+ &&
isUnderIsNull(context.accessPathBuilder.getPathList())) {
+ StructField field = resolveStructField(
+ (StructType) first.getDataType(), fieldName);
+ if (field == null || !field.isNullable()) {
Review Comment:
When the selected struct field is not nullable, this branch replaces the
META context with a fresh DATA context, which drops the trailing NULL suffix
and emits `[s, f]` DATA instead of `[s, f, NULL]` META.
This is correct for evaluation but misses a more precise read: for
`element_at(s, 'f') IS NULL` with s physically nullable and f NOT NULL, the
predicate is equivalent to `s IS NULL`, so `[s, NULL]` META alone would suffice
— it also preserves the full struct type in pruneDataType, which is the stated
purpose of this fallback — while the current code reads the entire field
column. Consider emitting `[s, NULL]` META when the field is not nullable and
`hasPhysicalNullMap(slotReference)` is true, instead of falling back to full
field data.
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/DescriptorToThriftConverter.java:
##########
@@ -88,6 +89,7 @@ public static TSlotDescriptor toThrift(SlotDescriptor
slotDesc) {
public static TColumnAccessPath toThrift(ColumnAccessPath accessPath) {
TColumnAccessPath result = new TColumnAccessPath(
accessPath.getType() == ColumnAccessPathType.DATA ?
TAccessPathType.DATA : TAccessPathType.META);
+
result.setVersion(DescriptorsConstants.TCOLUMN_ACCESS_PATH_VERSION_TYPED);
Review Comment:
All access paths now unconditionally carry version=1, including queries that
produce only DATA paths. A pre-upgrade BE ignores the version field but fails
hard on any META path (`_get_sub_access_paths()` dereferences
`data_access_path.path`, which is unset), so a new FE running against
not-yet-upgraded BEs turns every IS NULL / length() / cardinality()
nested-column query into an error instead of degrading gracefully. Combined
with the legacy path ambiguity on the BE side, only the documented BE-first
upgrade order is safe; the failure mode of the reverse order (hard query
errors, not just missed optimizations) may be worth stating in the PR
description.
--
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]
