github-actions[bot] commented on code in PR #66007:
URL: https://github.com/apache/doris/pull/66007#discussion_r3674521659
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergTransaction.java:
##########
@@ -289,14 +291,23 @@ public void beginDelete(ExternalTable dorisTable) throws
UserException {
}
}
- /**
- * Begin merge operation for Iceberg UPDATE (single scan RowDelta).
- */
- public void beginMerge(ExternalTable dorisTable) throws UserException {
+ private Table createTransactionTable(ExternalTable dorisTable, Table
retainedTable) {
Review Comment:
[P1] Pin DELETE to the scanned table generation
This new retained/writable transaction path still excludes `beginDelete()`:
`IcebergDeleteCommand` first plans its scan from the relation-frozen S0 table,
but `IcebergDeleteExecutor.beforeExec()` later reloads the live table and
records its current snapshot as the RowDelta base. If a matching row is
inserted as S1 in between, BE deletes only the S0 rows while
`validateFromSnapshot(S1)` treats that insert as preceding the transaction and
does not report it; DELETE can succeed with a row still satisfying its
predicate. Please carry the retained target through the delete sink/executor
too, and add a barrier test with a data commit between planning and
`beginDelete()`.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/source/PaimonScanNode.java:
##########
@@ -177,6 +180,11 @@ public PaimonScanNode(PlanNodeId id,
@Override
protected void doInitialize() throws UserException {
+ Optional<MvccSnapshot> relationSnapshot = getRelationSnapshot();
+ if (desc.getTable() instanceof PaimonExternalTable) {
+ // Rebuild the source before applying query options so both layers
use this relation's snapshot.
+ source = new PaimonSource(desc, relationSnapshot);
Review Comment:
[P1] Preserve an empty Paimon snapshot at execution
This relation-local rebuild still cannot fence an empty latest table.
`PaimonLatestSnapshotProjectionLoader` stores snapshot ID `-1` but, when no
snapshot exists, retains the live base `FileStoreTable` without
`scan.snapshot-id`; `getProcessedTable()` then scans that handle normally. If
the first commit lands after binding but before this initialization, the
relation planned as empty reads the new rows. Please make the invalid-snapshot
state executable as an empty scan (or otherwise pin it), and add an
empty-bind/first-commit barrier test for normal Paimon scans.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergExternalMetaCache.java:
##########
@@ -232,7 +232,8 @@ private IcebergSnapshotCacheValue
loadSnapshotProjection(ExternalTable dorisTabl
latestIcebergSnapshot.getSnapshotId(),
latestIcebergSnapshot.getSchemaId());
}
return new IcebergSnapshotCacheValue(
- icebergPartitionInfo, latestIcebergSnapshot,
IcebergUtils.getNameMapping(icebergTable));
+ icebergPartitionInfo, latestIcebergSnapshot,
IcebergUtils.getNameMapping(icebergTable),
+ icebergTable);
Review Comment:
[P1] Freeze the table before deriving this projection
The table is not frozen until the `IcebergSnapshotCacheValue` constructor,
after the latest snapshot/schema, partitions, and name mapping have already
been read from the shared `BaseTable`. A concurrent `ADD COLUMN ... DEFAULT`
can land between those reads and this line, yielding S0 bound outputs but a
frozen T1 target; `IcebergTableSink` then serializes T1's schema and
`VIcebergTableWriter::open` rejects the S0/T1 column-count mismatch. Optimistic
validation cannot help because the retained base already starts at T1. Please
capture one `TableMetadata` first and derive every projection field plus the
frozen table from it (also in `IcebergUtils.loadSnapshotCacheValue`), with a
barrier test inside construction.
##########
be/src/format_v2/table_reader.h:
##########
@@ -1515,15 +1585,34 @@ class TableReader {
Status _materialize_array_mapping_column(const ColumnMapping& mapping,
const ColumnPtr& file_column,
const size_t rows,
- ColumnPtr* column) {
+ ColumnPtr* column,
+ const NullMap*
nullable_parent_null_map = nullptr) {
DORIS_CHECK(mapping.child_mappings.size() == 1);
const auto full_file_column =
file_column->convert_to_full_column_if_const();
const NullMap* parent_null_map = nullptr;
const auto* nested_file_column =
_nested_column_if_nullable(full_file_column, &parent_null_map);
+ if (parent_null_map != nullptr && !mapping.table_type->is_nullable()) {
+ DORIS_CHECK(parent_null_map->size() == rows);
+ if (nullable_parent_null_map != nullptr) {
+ DORIS_CHECK(nullable_parent_null_map->size() == rows);
+ }
+ for (size_t i = 0; i < rows; ++i) {
+ // ARRAY row masks cannot be forwarded to elements because
they use different
+ // coordinates, so validate the container before dropping its
nullable wrapper.
+ if ((*parent_null_map)[i] &&
+ (nullable_parent_null_map == nullptr ||
!(*nullable_parent_null_map)[i])) {
+ return Status::InternalError(
+ "Source array contains NULL for non-nullable table
column");
+ }
+ }
+ }
const auto* file_array = assert_cast<const
ColumnArray*>(nested_file_column);
ColumnPtr nested_column = file_array->get_data_ptr();
- const auto& element_mapping = mapping.child_mappings[0];
+ auto element_mapping = mapping.child_mappings[0];
+ // Keep the descriptor type for schema matching. ARRAY's nullable
element wrapper is a
+ // storage invariant, so add it only at the materialization boundary.
+ element_mapping.table_type = make_nullable(element_mapping.table_type);
RETURN_IF_ERROR(_materialize_present_child_mapping_column(
Review Comment:
[P1] Mask hidden collection entries during descendant validation
The container's row-level null map is still absent from this recursive
element call; Map likewise validates every key/value without projecting its row
mask through the offsets. The [Arrow
format](https://arrow.apache.org/docs/format/Columnar.html#support-for-null-values)
permits a null slot to occupy nonempty physical storage, and Doris's Arrow
SerDes copy that full child span. A valid null array whose hidden `STRUCT<a INT
NOT NULL>` element has a null `a` placeholder is therefore rejected by
required-child alignment even though the container masks it. Please compact
masked spans or propagate an entry-coordinate hidden mask, and add nullable
Array/Map tests with nonempty hidden struct payload.
##########
be/src/core/data_type_serde/orc_serde_utils.h:
##########
@@ -0,0 +1,74 @@
+// 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)) {
+ total_size += length;
+ }
+ }
+ char* cursor = total_size == 0 ? nullptr : arena.alloc(total_size);
+ for (size_t i = 0; i < strings->numElements; ++i) {
+ const size_t length = static_cast<size_t>(strings->length[i]);
+ const char* source = strings->data[i];
+ if (length > 0 && !arena.contains(source, length)) {
+ std::memcpy(cursor, source, length);
+ strings->data[i] = cursor;
+ cursor += length;
+ } else if (length == 0) {
Review Comment:
[P1] Preserve empty strings in ORC statistics
This branch changes a visible zero-length string from a non-null pointer to
`nullptr`. In the pinned ORC 1.9.0 writer, every non-null string is passed to
`StringColumnStatisticsImpl::update`, but that routine updates min/max only
when the pointer is non-null
([writer](https://github.com/apache/orc/blob/rel/release-1.9.0/c%2B%2B/src/ColumnWriter.cc#L1097-L1134),
[statistics](https://github.com/apache/orc/blob/rel/release-1.9.0/c%2B%2B/src/Statistics.hh#L1181-L1205)).
Thus a compacted collection leaf containing `""` and `"z"` records minimum
`"z"`, which can make statistics pruning skip present data. Please preserve a
non-null, lifetime-safe pointer for zero-length values (for example, an
Arena-owned or static empty sentinel) and add a masked Array/Map case that
verifies empty-string leaf statistics.
--
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]
