github-actions[bot] commented on code in PR #68786:
URL: https://github.com/apache/doris/pull/68786#discussion_r4229820646
##########
be/src/core/data_type_serde/data_type_string_serde.cpp:
##########
@@ -568,7 +568,12 @@ Status
DataTypeStringSerDeBase<ColumnType>::write_column_to_iceberg_arrow(
arrow::ArrayBuilder* array_builder, int64_t start, int64_t end,
const cctz::time_zone& ctz) const {
if (!is_iceberg_uuid_field(field)) {
- // Keep the existing CHAR/STRING fixed-binary binding until external
type mappings change.
+ // Iceberg fixed(N) is a byte binding, so text padding must not change
its physical value.
Review Comment:
[P2] Preserve legacy FIXED writes during BE-first upgrades. An older FE with
the default `enable.mapping.varbinary=false` still plans Iceberg `FIXED(N)` as
`CHAR(N)` and sends that type to a new BE. This branch rejects its fixed-size
Arrow builder before the previous width-checked CHAR writer runs, so inserts
that worked before fail throughout the rolling upgrade. Keep the legacy CHAR
binding for old-FE plans or version the FE/BE write contract, and cover an
old-FE/new-BE fixed-column insert.
##########
fe/be-java-extensions/hadoop-hudi-scanner/src/test/java/org/apache/doris/hudi/HadoopHudiColumnValueTest.java:
##########
@@ -31,21 +31,89 @@
public class HadoopHudiColumnValueTest {
@Test
- public void testInt64TimestampUsesSessionTimezone() {
+ public void testInstantCarriersAcrossTimezonesAndPrecisions() {
+ java.util.TimeZone original = java.util.TimeZone.getDefault();
+ try {
+ for (String zone : new String[] {"UTC", "Asia/Shanghai",
"America/New_York"}) {
+
java.util.TimeZone.setDefault(java.util.TimeZone.getTimeZone(zone));
+ for (int precision : new int[] {3, 6}) {
+ for (long epoch : new long[] {-1, 0, 1636263000123L,
1636266600123L}) {
+ if (precision == 6 && epoch > 0) {
+ epoch = epoch * 1000 + 456;
+ }
+ long units = precision == 3 ? 1000L : 1_000_000L;
+ java.time.Instant instant =
java.time.Instant.ofEpochSecond(Math.floorDiv(epoch, units),
+ Math.floorMod(epoch, units) * (1_000_000_000L
/ units));
+ LocalDateTime expected =
LocalDateTime.ofInstant(instant, java.time.ZoneOffset.UTC);
+ HadoopHudiColumnValue value = new
HadoopHudiColumnValue(ZoneId.of(zone));
+ value.setField(ColumnType.parseType("ts",
"timestamptz(" + precision + ")"),
+
PrimitiveObjectInspectorFactory.writableTimestampObjectInspector);
+ value.setRow(new LongWritable(epoch));
+ Assertions.assertEquals(expected,
value.getTimeStampTz());
+ value.setRow(java.sql.Timestamp.from(instant));
+ Assertions.assertEquals(expected,
value.getTimeStampTz());
+ value.setRow(new
TimestampWritableV2(Timestamp.ofEpochSecond(
+ instant.getEpochSecond(), instant.getNano())));
+ Assertions.assertEquals(expected,
value.getTimeStampTz());
+ value.setRow(null);
+ Assertions.assertTrue(value.isNull());
+ }
+ }
+ }
+ } finally {
+ java.util.TimeZone.setDefault(original);
+ }
+ }
+
+ @Test
+ public void testInstantTimestampUsesUtcComponents() {
+ HadoopHudiColumnValue value = new
HadoopHudiColumnValue(ZoneId.of("Asia/Shanghai"));
+ value.setField(ColumnType.parseType("ts", "timestamptz(6)"), null);
+ value.setRow(new LongWritable(-1));
+ Assertions.assertEquals(LocalDateTime.of(1969, 12, 31, 23, 59, 59,
999999000), value.getTimeStampTz());
+ value.setField(ColumnType.parseType("ts", "timestamptz(6)"),
+
PrimitiveObjectInspectorFactory.writableTimestampObjectInspector);
+ value.setRow(new TimestampWritableV2(Timestamp.ofEpochSecond(1,
111333000)));
+ Assertions.assertEquals(LocalDateTime.of(1970, 1, 1, 0, 0, 1,
111333000), value.getTimeStampTz());
+ }
+
+ @Test
+ public void testInt64LocalTimestampIgnoresSessionTimezone() {
HadoopHudiColumnValue value = new
HadoopHudiColumnValue(ZoneId.of("America/Los_Angeles"));
value.setField(ColumnType.parseType("ts", "datetimev2(6)"), null);
value.setRow(new LongWritable(0));
- Assertions.assertEquals(LocalDateTime.of(1969, 12, 31, 16, 0),
value.getDateTime());
+ Assertions.assertEquals(LocalDateTime.of(1970, 1, 1, 0, 0),
value.getDateTime());
}
@Test
- public void testInt96TimestampUsesSessionTimezone() {
+ public void testInt96LocalTimestampIgnoresSessionTimezone() {
HadoopHudiColumnValue value = new
HadoopHudiColumnValue(ZoneId.of("America/Los_Angeles"));
value.setField(ColumnType.parseType("ts", "datetimev2(6)"),
PrimitiveObjectInspectorFactory.writableTimestampObjectInspector);
value.setRow(new TimestampWritableV2(Timestamp.ofEpochSecond(0)));
- Assertions.assertEquals(LocalDateTime.of(1969, 12, 31, 16, 0),
value.getDateTime());
+ Assertions.assertEquals(LocalDateTime.of(1970, 1, 1, 0, 0),
value.getDateTime());
}
+
+ @Test
+ public void testJniRejectsUtcYearOverflow() {
+ org.apache.doris.jni.spi.utils.OffHeap.setTesting();
+ org.apache.doris.jni.spi.vec.ColumnType columnType =
+ org.apache.doris.jni.spi.vec.ColumnType.parseType("ts",
"timestamptz(6)");
+ for (String text : new String[] {"0000-12-31T23:59:59Z",
"+10000-01-01T00:00:00Z"}) {
Review Comment:
[P1] Make the scanner boundary tests accept UTC year zero. This loop expects
`appendValue` to throw for `0000-12-31T23:59:59Z`, but Hudi converts it to a
UTC year-zero `LocalDateTime` and the corrected `VectorColumn.putTimeStampTz`
accepts years 0–9999; `VectorColumnTimestampTzTest` explicitly expects that
behavior. The same stale assertion exists in the new Fluss, Paimon, and Trino
scanner tests, so all four fail on their first iteration. Assert success for
year zero and reserve `assertThrows` for year 10000. This is separate from the
earlier encoder bug thread.
##########
regression-test/suites/external_table_p0/hive/test_hive_orc.groovy:
##########
@@ -275,7 +287,8 @@ suite("test_hive_orc", "p0,external") {
}
} finally {
+ // A failed assertion must not leave derived objects behind for
the next run.
+ sql "DROP DATABASE IF EXISTS internal.test_view_varbinary_db FORCE"
Review Comment:
[P3] Leave this fixture available after the suite. The database is already
dropped before creation at line 247, so this new `finally` force-drop removes
the view and other derived objects even when an assertion fails. Repository
regression rules preserve test tables for debugging; remove the post-test drop
and keep the setup drop.
##########
regression-test/suites/external_table_p0/hive/test_hive_orc.groovy:
##########
@@ -242,22 +244,32 @@ suite("test_hive_orc", "p0,external") {
order_qt_sql_topn_binary_col4 """ select
binary_col,cast(binary_col as string) from orc_all_types order by binary_col
desc,string_col desc limit 10; """
sql """ switch internal; """
- sql """ drop database if exists test_view_varbinary_db"""
+ sql """ drop database if exists test_view_varbinary_db force"""
sql """ create database if not exists test_view_varbinary_db"""
sql """use test_view_varbinary_db"""
+ // Binary mapping enables transport; compare hexadecimal strings
without binary hash keys.
+ def binaryQuery = ("SELECT binary_col FROM
`test_hive_orc_mapping_varbinary`.`default`.`orc_all_types` "
+ + "ORDER BY int_col, from_binary(binary_col) LIMIT 100")
+ def binarySource = "(${binaryQuery}) binary_src"
+ def expectedBinary = sql "SELECT from_binary(binary_col) FROM
${binarySource} ORDER BY from_binary(binary_col)"
+ // Views retain execution types; materialized objects still obey
native storage restrictions.
+ sql "CREATE VIEW test_view_varbinary AS SELECT binary_col FROM
${binarySource}"
+ assertEquals(expectedBinary,
+ sql("SELECT from_binary(binary_col) FROM
test_view_varbinary ORDER BY from_binary(binary_col)"))
test {
- sql " create view test_view_varbinary as select binary_col
from `test_hive_orc_mapping_varbinary`.`default`.`orc_all_types`; "
- exception " View does not support VARBINARY type: binary_col"
+ sql """CREATE TABLE test_ctas_varbinary DISTRIBUTED BY RANDOM
BUCKETS 2
+ PROPERTIES ('replication_num'='1') AS SELECT binary_col
FROM ${binarySource}"""
+ exception "varbinary"
}
-
test {
sql """ CREATE MATERIALIZED VIEW test_mv_varbinary
BUILD DEFERRED REFRESH AUTO ON MANUAL
DISTRIBUTED BY RANDOM BUCKETS 2
PROPERTIES ('replication_num' = '1')
- AS select binary_col from
`test_hive_orc_mapping_varbinary`.`default`.`orc_all_types`; """
- exception " MTMV do not support varbinary type : binary_col"
+ AS SELECT binary_col FROM ${binarySource}"""
+ exception "varbinary"
}
+ assertTrue(sql("DESC
test_view_varbinary")[0][1].toLowerCase().startsWith("varbinary"))
Review Comment:
[P3] Record this fixed view schema in generated output. `DESC
test_view_varbinary` has a deterministic VARBINARY result, but this assertion
keeps it outside the suite's `.out`. The regression rules require determined
results in a named `qt`/`order_qt` check with runner-generated output; please
add that check and regenerate this suite's expected output.
##########
fe/fe-connector/fe-connector-paimon/src/main/java/org/apache/doris/connector/paimon/PaimonScanPlanProvider.java:
##########
@@ -1927,6 +1947,73 @@ private static boolean projectsVariant(
.anyMatch(index -> containsVariant(rowType.getTypeAt(index)));
}
+ private static Set<Integer> scanReadFieldIds(RowType rowType,
List<ConnectorColumnHandle> columns,
+ Optional<ConnectorExpression> filter) {
+ Set<String> names =
columns.stream().filter(PaimonColumnHandle.class::isInstance)
+ .map(PaimonColumnHandle.class::cast)
+ .filter(column -> !column.isMetadataColumn())
+ .map(column -> column.getName().toLowerCase(Locale.ROOT))
+ .collect(Collectors.toSet());
+ filter.ifPresent(expression -> collectFilterColumnNames(expression,
names));
+ return rowType.getFields().stream()
+ .filter(field ->
names.contains(field.name().toLowerCase(Locale.ROOT)))
+ .map(DataField::id).collect(Collectors.toSet());
+ }
+
+ private static void collectFilterColumnNames(ConnectorExpression
expression, Set<String> names) {
+ if (expression instanceof ConnectorColumnRef) {
+ names.add(((ConnectorColumnRef)
expression).getColumnName().toLowerCase(Locale.ROOT));
+ }
+ expression.getChildren().forEach(child ->
collectFilterColumnNames(child, names));
+ }
+
+ static boolean requiresLegacyOrcTimestampReader(Table table,
Optional<List<RawFile>> rawFiles,
+ Set<Integer> readFieldIds, Map<Long, Boolean> schemaTimestamps) {
+ if (readFieldIds.isEmpty() || !rawFiles.isPresent()
+ || rawFiles.get().stream().noneMatch(f ->
f.path().endsWith(".orc"))
+ || !new org.apache.paimon.options.Options(table.options()).get(
+
org.apache.paimon.format.OrcOptions.ORC_TIMESTAMP_LTZ_LEGACY_TYPE)) {
+ return false;
+ }
+ // Only decoded fields require SDK timezone conversion. Unread LTZ
columns must not disable
+ // native splitting or metadata columns; stable field IDs also scope
historical schemas after renames.
+ if (readsTimestampLtz(table.rowType(), readFieldIds)) {
+ return true;
+ }
+ FileStoreTable fileStoreTable = (FileStoreTable) table;
+ for (RawFile file : rawFiles.get()) {
+ if (file.path().endsWith(".orc") &&
schemaTimestamps.computeIfAbsent(file.schemaId(),
+ id -> readsTimestampLtz(
+
fileStoreTable.schemaManager().schema(id).logicalRowType(), readFieldIds))) {
Review Comment:
[P2] Resolve historical ORC schemas from each fallback split's branch. A
`scan.fallback-branch` plan can contain raw ORC files from both branches, but
this lookup uses the pair's `schemaManager()`, which delegates only to its
wrapped branch, and caches by numeric schema ID. With
`orc.timestamp-ltz.legacy.type=true`, a fallback file whose schema ID exists
only on the other branch makes a scan of even a current INT field fail during
planning. Select the branch from `FallbackDataSplit` before schema lookup and
include it in the cache key; cover independently advanced branch schema IDs.
This is separate from the JNI metadata iterator threads.
##########
fe/fe-core/src/main/java/org/apache/doris/tablefunction/ExternalFileTableValuedFunction.java:
##########
@@ -222,15 +222,15 @@ protected Map<String, String>
parseCommonProperties(Map<String, String> properti
String formatString = getOrDefaultAndRemove(copiedProps,
FileFormatConstants.PROP_FORMAT, "").toLowerCase();
fileFormatProperties =
FileFormatProperties.createFileFormatProperties(formatString);
- // Parse enable_mapping_varbinary property
+ // The catalog property was removed, but this TVF-only option must
remain explicit because
+ // changing an ad-hoc TVF result schema also breaks CTAS type
inference.
String enableMappingVarbinaryStr = getOrDefaultAndRemove(copiedProps,
FileFormatConstants.PROP_ENABLE_MAPPING_VARBINARY, "false");
fileFormatProperties.enableMappingVarbinary =
Boolean.parseBoolean(enableMappingVarbinaryStr);
- // Parse enable_mapping_timestamp_tz property
- String enableMappingTimestampTzStr = getOrDefaultAndRemove(copiedProps,
- FileFormatConstants.PROP_ENABLE_MAPPING_TIMESTAMP_TZ, "false");
- fileFormatProperties.enableMappingTimestampTz =
Boolean.parseBoolean(enableMappingTimestampTzStr);
+ // Consume the legacy option, but let file logical types determine
timezone semantics.
+
copiedProps.remove(FileFormatConstants.PROP_ENABLE_MAPPING_TIMESTAMP_TZ);
Review Comment:
[P2] Preserve the output type of existing file-TVF views. A view created
over an ORC `TIMESTAMP_INSTANT` file with `enable_mapping_timestamp_tz=false`
(or the old default) stores a DATETIMEV2 column. This code now discards
explicit false and always infers TIMESTAMPTZ when the view expands, while
`LogicalView` keeps the new child type and the saved view schema still
advertises DATETIMEV2. Preserve the view's stored type contract or migrate
these views, and cover an upgraded TVF view. The catalog migration view thread
does not cover this independent TVF path.
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergWriteSchemaContext.java:
##########
@@ -158,6 +158,10 @@ private IcebergWriteSchemaContext(String tableName, Schema
schema, int formatVer
ConnectorColumn column = new ConnectorColumn(
field.name(), type, field.doc() == null ? "" : field.doc(),
field.isOptional(), null,
true).withUniqueId(field.fieldId());
+ if (enableMappingVarbinary && field.type().typeId() ==
Type.TypeID.UUID) {
Review Comment:
[P2] Apply UUID text conversion inside complex fields. `IcebergTypeMapping`
now maps nested UUID leaves to `VARBINARY(16)`, but this marker is set only
when the top-level column is UUID. An `ARRAY<UUID>` or struct UUID inserted
with canonical text therefore reaches the nested fixed-size Arrow writer as 36
bytes (or fails FE coercion), whereas the former nested STRING writer parsed it
as UUID. Carry the semantic conversion recursively and cover array and struct
UUID writes. This is separate from the scalar UUID thread.
##########
fe/fe-connector/fe-connector-paimon/src/main/java/org/apache/doris/connector/paimon/PaimonScanPlanProvider.java:
##########
@@ -1927,6 +1947,73 @@ private static boolean projectsVariant(
.anyMatch(index -> containsVariant(rowType.getTypeAt(index)));
}
+ private static Set<Integer> scanReadFieldIds(RowType rowType,
List<ConnectorColumnHandle> columns,
+ Optional<ConnectorExpression> filter) {
+ Set<String> names =
columns.stream().filter(PaimonColumnHandle.class::isInstance)
+ .map(PaimonColumnHandle.class::cast)
+ .filter(column -> !column.isMetadataColumn())
+ .map(column -> column.getName().toLowerCase(Locale.ROOT))
+ .collect(Collectors.toSet());
+ filter.ifPresent(expression -> collectFilterColumnNames(expression,
names));
+ return rowType.getFields().stream()
+ .filter(field ->
names.contains(field.name().toLowerCase(Locale.ROOT)))
+ .map(DataField::id).collect(Collectors.toSet());
+ }
+
+ private static void collectFilterColumnNames(ConnectorExpression
expression, Set<String> names) {
+ if (expression instanceof ConnectorColumnRef) {
+ names.add(((ConnectorColumnRef)
expression).getColumnName().toLowerCase(Locale.ROOT));
+ }
+ expression.getChildren().forEach(child ->
collectFilterColumnNames(child, names));
+ }
+
+ static boolean requiresLegacyOrcTimestampReader(Table table,
Optional<List<RawFile>> rawFiles,
+ Set<Integer> readFieldIds, Map<Long, Boolean> schemaTimestamps) {
+ if (readFieldIds.isEmpty() || !rawFiles.isPresent()
+ || rawFiles.get().stream().noneMatch(f ->
f.path().endsWith(".orc"))
+ || !new org.apache.paimon.options.Options(table.options()).get(
Review Comment:
[P2] Check the legacy ORC option on the split's branch. A fallback read can
combine branches with the same LTZ row type but different
`orc.timestamp-ltz.legacy.type` settings. This gate reads only the wrapped
table's option: if wrapped=false and fallback=true, it returns false before
inspecting a fallback ORC split, so FE chooses the native reader for bytes that
need Paimon's legacy LTZ conversion. Use the owning branch's option for each
split and cover mixed branch settings. This is independent of the historical
schema lookup at line 1987.
--
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]
