github-actions[bot] commented on code in PR #69163:
URL: https://github.com/apache/doris/pull/69163#discussion_r4306499958
##########
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:
[P2] Restrict timestamp planning to projected nested leaves
For an ORC schema
`struct<ns:timestamp[NANOS],s:struct<i:int,unused:timestamp>>`, `SELECT s.i`
reaches this check with root column `s`. `has_timestamp` visits the unread
`s.unused`, enabling `STRIPE_WRITER` even though `includeTypes` later selects
only `s.i` and no timestamp decoder runs. The scan then discovers every visited
stripe's writer timezone and creates windowed RowReaders unnecessarily. In the
pinned SDK, discovery reads/decompresses/parses a fresh stripe footer that the
normal RowReader reads again; lower caches may absorb I/O, but the duplicate
parsing remains. This adds avoidable synchronous work to large nested
projections. Pass the actual nested projection into this check so only a
projected plain timestamp enables the workaround.
##########
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:
[P2] Resolve raw slot names before checking unsafe fields
`rewrite_data_files` passes parser `UnboundSlot`s here. For a quoted
top-level column named `ts.day`, `getName()` adds backticks, while
`IcebergNereidsUtils.extractColumnName()` uses the raw single name part. This
lookup therefore misses an unsafe DATE-to-nanos field although subsequent
conversion resolves it. After promoting that field and committing a
nanos-schema snapshot, a rewrite predicate on the quoted column for
`2024-01-02` silently skips a retained DATE file containing that date: Iceberg
interprets its four-byte bound as 19724 nanos instead of comparing with
1704153600000000000. Resolve raw unbound names consistently before the guard so
this operation rejects unsafe historical bounds just as it does for an ordinary
identifier.
--
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]
