github-actions[bot] commented on code in PR #66007:
URL: https://github.com/apache/doris/pull/66007#discussion_r3683314379
##########
be/src/format_v2/table_reader.h:
##########
@@ -1452,9 +1491,39 @@ class TableReader {
return column.get();
}
+ template <typename Offsets>
+ static const NullMap* _project_collection_parent_null_map(
+ const NullMap* container_null_map, const NullMap*
ancestor_null_map, const size_t rows,
+ const Offsets& offsets, const size_t child_rows, NullMap* const
projected_null_map) {
+ if (container_null_map == nullptr && ancestor_null_map == nullptr) {
+ return nullptr;
+ }
+ DORIS_CHECK(container_null_map == nullptr ||
container_null_map->size() == rows);
+ DORIS_CHECK(ancestor_null_map == nullptr || ancestor_null_map->size()
== rows);
+ DORIS_CHECK(offsets.size() == rows);
+ projected_null_map->resize(child_rows);
Review Comment:
[P2] Avoid entry-sized projection when both masks are clear
A nullable Array/Map always supplies a null-map pointer even when
`has_null()` is false. This helper treats pointer presence as sufficient,
allocates and zero-fills `child_rows` bytes, and repeats that work for each
batch/nested collection. For 4K rows with 10K entries each, an all-present scan
creates about 40 MB of scratch per mapping even though the projected mask
contains no set bit. Please scan the row-coordinate maps first and return
`nullptr` when both are clear, allocating the entry mask only when some
collection row is hidden, with a unit case for the clear-mask fast path.
##########
be/src/core/data_type_serde/orc_serde_utils.h:
##########
@@ -0,0 +1,77 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <cstring>
+#include <orc/Vector.hh>
+
+#include "core/arena.h"
+
+namespace doris {
+
+inline void copy_orc_string_data_to_arena(orc::ColumnVectorBatch* batch,
Arena& arena) {
+ if (auto* strings = dynamic_cast<orc::StringVectorBatch*>(batch)) {
+ size_t total_size = 0;
+ for (size_t i = 0; i < strings->numElements; ++i) {
+ const size_t length = static_cast<size_t>(strings->length[i]);
+ // Some serdes already allocate their payload in this Arena;
copying it again would
+ // double the serialized string memory without extending its
lifetime.
+ if (length > 0 && !arena.contains(strings->data[i], length)) {
Review Comment:
[P2] Skip payloads for absent ORC string positions
Nullable SerDe sets `hasNulls/notNull`, but String and Varbinary SerDes
still populate every physical `data/length` slot. Both loops here ignore
presence, so a logically NULL slot with a retained large payload is copied into
the Arena even though the [ORC writer filters those positions through
`notNull`](https://github.com/apache/orc/blob/rel/release-1.9.0/c%2B%2B/src/ColumnWriter.cc#L1007-L1035).
On the packed Array/Map path this can amplify peak memory or OOM without
affecting output. Please skip sizing and rewriting when `hasNulls &&
!notNull[i]`, normalize the absent pointer safely while preserving the
present-empty sentinel, and test that Arena growth excludes an absent borrowed
payload.
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergConnectorMetadata.java:
##########
@@ -671,10 +671,12 @@ private Table resolveTableForRead(ConnectorSession
session, IcebergTableHandle h
* already validated by {@code getSysTableHandle}, so {@code
MetadataTableType.from} (case-insensitive)
* never returns null.
*/
- private Table loadSysTable(IcebergTableHandle handle) {
+ private Table loadSysTable(ConnectorSession session, IcebergTableHandle
handle) {
try {
return context.executeAuthenticated(() -> {
- Table base = catalogOps.loadTable(handle.getDbName(),
handle.getTableName());
+ // Schema binding, column handles and split planning must all
see one base generation; an
+ // independently refreshed session catalog can otherwise pair
old tuple slots with new rows.
+ Table base = resolveTableForRead(session, handle);
Review Comment:
[P1] Freeze the statement's Iceberg metadata generation
`resolveTableForRead` memoizes the same raw `BaseTable` held by
`IcebergTableCache`, so different statements retain one mutable object. A
second DML statement can resolve that object during scan planning and then call
`beginWrite`/`newTransaction`, whose refresh advances the shared
`TableOperations`. If this system-table statement bound slots at T0 and reaches
this loader after that refresh, it constructs metadata rows at T1 and recreates
the old-slot/new-row mismatch this change is intended to prevent. Please retain
immutable `TableMetadata` or isolated read operations per statement, keep
writable operations separate, and add a two-statement barrier test that
refreshes between system-table binding and planning.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/util/TypeCoercionUtils.java:
##########
@@ -1168,7 +1172,11 @@ private static Optional<DataType>
findWiderComplexTypeForTwo(
leftFields.get(i).getDataType(),
rightFields.get(i).getDataType(),
overflowToDouble, stringIsHighPriority);
if (newDataType.isPresent()) {
-
newFields.add(leftFields.get(i).withDataType(newDataType.get()));
+ // The common layout must admit NULLs produced while
either child is cast to it.
+ boolean nullable = leftFields.get(i).isNullable() ||
rightFields.get(i).isNullable()
Review Comment:
[P2] Keep strict common Struct fields required
This reducer still applies the non-strict `Cast.castNullable` result
unconditionally. With `enable_strict_cast=true`, combining required
`STRUCT<d:DATETIMEV2(3)>` and required `STRUCT<d:DATETIMEV2(6)>` produces a
nullable common `d`, even though a failed strict conversion aborts rather than
yielding NULL. A later cast or INSERT into the required layout then fails
`CheckCast`'s earlier declared-source-nullable guard before its new strict
exemption can help. Please make conversion-induced nullability strict-aware
across all new and legacy Struct reducers, and add a strict UNION/CASE result
consumed by a required cast or INSERT.
##########
be/src/format_v2/table_reader.h:
##########
@@ -1274,23 +1308,24 @@ class TableReader {
return Status::OK();
}
- Status _materialize_present_child_mapping_column(const ColumnMapping&
mapping,
- const ColumnPtr&
file_column,
- const size_t rows,
ColumnPtr* column) {
+ Status _materialize_present_child_mapping_column(
+ const ColumnMapping& mapping, const ColumnPtr& file_column, const
size_t rows,
+ ColumnPtr* column, const NullMap* nullable_parent_null_map =
nullptr) {
DORIS_CHECK(column != nullptr);
DORIS_CHECK(mapping.file_type != nullptr);
DORIS_CHECK(mapping.table_type != nullptr);
*column = file_column;
if (!mapping.is_trivial) {
if (!mapping.child_mappings.empty()) {
- RETURN_IF_ERROR(
- _materialize_complex_mapping_column(mapping, *column,
rows, column));
+ RETURN_IF_ERROR(_materialize_complex_mapping_column(mapping,
*column, rows, column,
+
nullable_parent_null_map));
} else {
RETURN_IF_ERROR(_cast_column_to_type(column,
mapping.file_type, mapping.table_type,
mapping.file_column_name));
}
}
- RETURN_IF_ERROR(_align_column_nullability(column, mapping.table_type));
+ RETURN_IF_ERROR(
+ _align_column_nullability(column, mapping.table_type,
nullable_parent_null_map));
Review Comment:
[P1] Materialize the retained Map key under the entry mask
A value-only Map projection intentionally keeps the full physical key stream
for entry existence and offsets, but it omits key from `child_mappings`.
`_materialize_map_mapping_column` therefore applies the projected row-to-entry
mask only to value and carries the untouched key into this whole-Map alignment.
If an evolved nullable file-key descendant is required by the table, a NULL Map
row may retain an entry with a NULL key placeholder; the Map branch here
descends without projecting the row mask and rejects that logically hidden
value. This is distinct from the fixed fully mapped path. Please materialize
every retained structural stream under the projected mask (or project the Map
mask in this alignment), and add normal-finalize coverage for a value-only
projection with a hidden nullable-to-required key.
--
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]
