Gabriel39 commented on code in PR #69163:
URL: https://github.com/apache/doris/pull/69163#discussion_r4309709337
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/source/IcebergScanNode.java:
##########
@@ -2407,11 +2409,12 @@ protected List<FileScanTask>
loadFileScanTasksWithManifestCache(
}
ExternalTable targetExternalTable = (ExternalTable)
source.getTargetTable();
Schema scanSchema = scan.schema();
+ Set<Integer> unsafeNanosFields =
IcebergTimestampUtils.unsafeNanosFields(icebergTable, scanSchema);
Review Comment:
Fixed the remaining dropped-key and ordinary SDK planning cases in
1a0cfb1e8ea. The safeguard now lives in the shared DeleteFileIndex, which both
the SDK and manifest-cache planners use. Four-byte historical DATE bounds on
either the data-file or delete-file side are treated as incomparable to
timestamp_ns rather than widened as if they were already nanos. This works even
when the key is resolved only from historical schemas after being dropped.
Normal eight-byte nanos bounds and null-count pruning remain available; the
cache-only blanket copyWithoutStats workaround was removed.
The tests exercise both planners before/after dropping the promoted key,
historical DATE bounds on the delete side as well, and continued pruning for
disjoint native nanos bounds. The old code fails the applicability/pruning
assertions; the updated code passes. All 232 tests across the five relevant
Iceberg test classes passed.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergTimestampUtils.java:
##########
@@ -0,0 +1,88 @@
+// 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.
+
+package org.apache.doris.datasource.iceberg;
+
+import org.apache.doris.analysis.Expr;
+import org.apache.doris.analysis.SlotRef;
+import org.apache.doris.nereids.trees.expressions.literal.TimeStampNsLiteral;
+
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.Table;
+import org.apache.iceberg.types.Type;
+import org.apache.iceberg.types.TypeUtil;
+import org.apache.iceberg.types.Types;
+
+import java.math.BigDecimal;
+import java.time.LocalDateTime;
+import java.time.ZoneId;
+import java.time.ZoneOffset;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+
+/** Nanosecond conversion and retained DATE-metric safeguards shared by the
4.2 planners. */
+public final class IcebergTimestampUtils {
+ private IcebergTimestampUtils() {
+ }
+
+ public static long parseNanos(String value, ZoneId zone) {
+ // Static partitions must accept and round exactly the same literals
as the sink cast.
+ LocalDateTime timestamp = TimeStampNsLiteral.parseDateTime(value,
zone);
+ // Widen before combining: the minimum timestamp's whole second
overflows long nanos.
+ return BigDecimal.valueOf(timestamp.toEpochSecond(ZoneOffset.UTC))
+ .multiply(BigDecimal.valueOf(1_000_000_000L))
+ .add(BigDecimal.valueOf(timestamp.getNano())).longValueExact();
+ }
+
+ public static Set<Integer> unsafeNanosFields(Table table, Schema schema) {
+ Set<Integer> unsafe = new HashSet<>();
+ Map<Integer, Schema> history = table.schemas();
+ int maxId =
history.keySet().stream().mapToInt(Integer::intValue).max().orElse(-1);
+ // Removed history can leave live DATE files whose four-byte bounds
Iceberg reads as longs.
+ boolean incomplete = history.isEmpty() || history.size() != (long)
maxId + 1;
+ for (Types.NestedField field :
TypeUtil.indexById(schema.asStruct()).values()) {
+ if (field.type().typeId() != Type.TypeID.TIMESTAMP_NANO) {
+ continue;
+ }
+ if (incomplete || history.values().stream().anyMatch(old -> {
+ Type type = old.findType(field.fieldId());
+ return type != null && type.typeId() == Type.TypeID.DATE;
+ })) {
+ unsafe.add(field.fieldId());
+ }
+ }
+ return unsafe;
+ }
+
+ private static boolean unsafeName(String name, Schema schema, Set<Integer>
unsafe) {
+ Types.NestedField field = schema.caseInsensitiveFindField(name);
+ return field != null && unsafe.contains(field.fieldId());
+ }
+
+ public static boolean referencesUnsafe(Expr expr, Schema schema,
Set<Integer> unsafe) {
+ if (expr instanceof SlotRef && unsafeName(((SlotRef)
expr).getColumnName(), schema, unsafe)) {
+ return true;
+ }
+ return expr.getChildren().stream().anyMatch(child ->
referencesUnsafe(child, schema, unsafe));
+ }
+
+ public static boolean
referencesUnsafe(org.apache.doris.nereids.trees.expressions.Expression expr,
+ Schema schema, Set<Integer> unsafe) {
+ return expr.getInputSlots().stream().anyMatch(slot ->
unsafeName(slot.getName(), schema, unsafe));
Review Comment:
Fixed in 1a0cfb1e8ea. The unsafe-field guard now uses the raw single name
part of an UnboundSlot, matching IcebergNereidsUtils instead of looking up its
SQL-rendered/quoted name. Added parser-to-rewrite tests for names containing a
dot, a space, and an escaped backtick. Each rejects unsafe DATE-to-nanos
pruning before TableScan.filter is called. The original implementation fails
the new test; the fixed implementation passes, alongside all 232 focused FE
tests.
##########
be/src/format_v2/orc/orc_reader.cpp:
##########
@@ -741,6 +749,88 @@ Status find_projected_minmax_leaf(const ::orc::Type&
root_type,
} // namespace
+// OrcReaderScanState has external linkage, so GCC requires its member types
to stay
+// outside the anonymous namespace (-Wsubobject-linkage).
+// Keep SDK workarounds and their scan requirements in one plan. SerDes
receive only the
+// resulting decode context and never need to infer which reader strategy
produced a batch.
+class OrcTimestampReadPlan {
+public:
+ void configure(const ::orc::Type& file_schema,
+ const std::vector<format::LocalColumnId>& read_columns,
+ const cctz::time_zone& session_timezone) {
+ _context = OrcTimestampDecodeContext(session_timezone);
+ // An unprojected NANOS sibling can affect the SDK's ordinary
timestamp decoding.
+ const bool needs_writer_timezone =
+ orc_serde_utils::has_nanosecond_timestamp(file_schema) &&
+ std::ranges::any_of(read_columns, [&](const auto column_id) {
+ return !is_virtual_column(column_id) &&
+
has_timestamp(*file_schema.getSubtype(column_id.value()), false);
Review Comment:
Fixed in 1a0cfb1e8ea. OrcTimestampReadPlan now receives the actual
FileScanRequest projection and uses the same projected ORC type IDs as
includeTypes. Both writer-timezone discovery and dense-decoding eligibility
inspect only selected nested timestamp leaves.
The ASAN test writes 32 stripes with an unprojected NANOS sibling and checks
partial struct projections in predicate/non-predicate column roles. Before the
fix, reading only s.i performs 32 extra timestamp-footer reads and creates 6
RowReaders; after the fix, these counters are 0 and 1. Projecting the plain
timestamp still reads the required stripe timezones and returns the expected
values. The complete BE build and the focused test passed.
--
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]
