[fix](be) Fix Parquet timestamp decoding and export defaults#65446
[fix](be) Fix Parquet timestamp decoding and export defaults#65446Gabriel39 wants to merge 1 commit into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated review completed. I found one correctness issue that should be fixed before merge: plain ORC timestamp SARG pruning is not using the same session-timezone semantics as row decoding and aggregate min/max.
Critical checkpoint conclusions: the PR is focused on format v2 timestamp decoding and pruning; row decode, ORC statistics rounding, and Parquet INT96 row materialization were reviewed. No new concurrency, lifecycle, persistence, protocol-compatibility, or observability risk was found. The main gap is a parallel-path inconsistency in ORC timestamp SARG pruning, and the tests do not cover non-UTC plain ORC timestamp SARG behavior. git diff --check passed; build/unit tests were not run in this checkout because .worktree_initialized, thirdparty/installed, and thirdparty/installed/bin/protoc are missing.
Subagent conclusions: TSC-001 was accepted as MAIN-001 and submitted inline. OPT-001 was dismissed with evidence from the ORC C++ public statistics/writer shape. Convergence round 2 ended with both optimizer-rewrite and tests-session-config reporting NO_NEW_VALUABLE_FINDINGS for the same current ledger/comment set.
User focus: no additional user-provided focus was present.
Gabriel39
force-pushed
the
agent/fix-format-v2-timestamp-decoding
branch
2 times, most recently
from
e8f9938 to
4f5fa22
Compare
|
/review |
|
run buildall |
|
Codex automated review failed and did not complete. Error: Review step was skipped (possibly timeout or cancelled) Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
/review |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
There was a problem hiding this comment.
Review result: request changes.
I reviewed the changed FE/thrift wiring, Parquet/ORC format-v2 reader paths, timestamp decoding/rounding behavior, and the added BE/FE tests. Validation was static because this runner is a shallow checkout and thirdparty/installed/bin/protoc is missing; I did verify the live PR head, GitHub file scope, existing inline comments, and git diff --check for the GitHub-listed changed files against the fetched base.
Critical checkpoints:
- Goal/test coverage: The PR mostly covers top-level Parquet INT96 timezone behavior and ORC nanosecond rounding, but it misses nested INT96 materialization and shorthand timezone propagation.
- Scope/focus: The implementation is mostly focused, but the new timezone contract is not applied uniformly across all Parquet INT96 value materialization paths.
- Concurrency/lifecycle: No new concurrent mutable state or lifecycle hazard found in the reviewed paths.
- Config/session propagation: Found one FE/BE normalization mismatch for
hive.parquet.time-zoneshorthand offsets. - Compatibility: The thrift field is optional, so old senders remain compatible; BE still validates invalid values. The accepted shorthand case is the compatibility gap.
- Parallel paths: Top-level INT96 and nested INT96 paths diverge; this is covered by an inline comment.
- Test results: I did not run builds/tests in this runner due missing thirdparty/protoc prerequisites. Static changed-file whitespace check passed.
- Observability/performance/persistence: No additional blocking issue found.
Subagent conclusions:
- optimizer-rewrite proposed the nested Parquet INT96 timezone gap; it was accepted as M1.
- tests-session-config proposed the FE/BE shorthand timezone normalization gap; it was accepted as M2.
- The existing ORC timestamp SARG timezone concern is already covered by discussion_r3556745988 and was not resubmitted.
- Convergence round C2 ended with both live subagents replying
NO_NEW_VALUABLE_FINDINGSfor the final M1/M2 comment set.
User focus: no additional user-provided review focus was supplied.
Gabriel39
force-pushed
the
agent/fix-format-v2-timestamp-decoding
branch
from
4f5fa22 to
05da9fa
Compare
|
run buildall |
|
/review |
FE UT Coverage ReportIncrement line coverage |
|
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at 10:57 AM. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
/review |
FE UT Coverage ReportIncrement line coverage |
There was a problem hiding this comment.
Review result: request changes.
I found one additional non-duplicate correctness issue: the new HMS Parquet INT96 timezone property is honored by scanner v2, but the legacy FileScanner path is still reachable and ignores the new scan parameter. Existing inline threads already cover the ORC plain TIMESTAMP SARG timezone issue, nested INT96 forwarding, and shorthand timezone canonicalization, so I did not duplicate those comments.
Critical checkpoint conclusions:
- Goal/test: the PR addresses format v2 Parquet INT96 and ORC timestamp rounding, with targeted BE/FE tests, but the new catalog property is not proven across the legacy scanner path.
- Scope/focus: the change is mostly focused, but it misses a parallel read path that users can still select.
- Concurrency/lifecycle: no new shared-state or lifecycle issue found in the reviewed changes.
- Config/session propagation: FE-to-BE propagation is present for scanner v2, but not for legacy scanner fallback/disablement.
- Compatibility/protocol: the new thrift field is optional and absence is handled; mixed-version field absence did not show a separate issue.
- Parallel paths: legacy Hive Parquet scanning remains inconsistent with scanner v2 for INT96 timezone semantics.
- Tests: new tests cover scanner v2 behavior, nested INT96, canonicalized timezone params, and ORC rounding; missing coverage for
enable_file_scanner_v2=false. Existing review context already requests non-UTC ORC TIMESTAMP SARG coverage. - Performance/observability: no additional issue found.
- Transaction/persistence/data writes: not applicable to this PR.
Subagent conclusions: OPT-1 became the inline comment below. OPT-2 and TSC-1 were verified as duplicates of existing GitHub thread discussion_r3556745988 and were not resubmitted. Convergence round 1 ended with both live subagents replying NO_NEW_VALUABLE_FINDINGS for the same ledger/comment set after the accepted candidate update.
User focus: no additional user-provided review focus.
Validation: static review only. This checkout is shallow/grafted and is missing .worktree_initialized, thirdparty/installed, and thirdparty/installed/bin/protoc, so I did not run BE/FE tests or style checks.
TPC-H: Total hot run time: 29435 ms |
TPC-DS: Total hot run time: 179312 ms |
ClickBench: Total hot run time: 24.77 s |
FE Regression Coverage ReportIncrement line coverage |
Gabriel39
force-pushed
the
agent/fix-format-v2-timestamp-decoding
branch
from
05da9fa to
5c21eda
Compare
Gabriel39
changed the title
|
run buildall |
|
/review |
There was a problem hiding this comment.
Automated review completed for the current PR head.
No additional non-duplicate, diff-scoped blocking issues were found beyond the existing inline review context. I am not adding new inline comments in this review.
User focus: no additional user-provided review focus.
Critical checkpoint conclusions:
- Goal and tests: the patch focuses on Parquet INT96 timezone decoding and ORC timestamp rounding/SARG behavior; current head includes targeted FE and BE tests for catalog timezone canonicalization/propagation, scanner-v2 and legacy scanner paths, nested INT96 decode, ORC timestamp rounding, and conservative SARG behavior.
- Scope: reviewed changed FE scan parameter plumbing, thrift field addition, BE Parquet legacy/v2 readers, ORC decode/stat/SARG paths, and related tests.
- Parallel paths: scanner-v2 and legacy Parquet paths both propagate
hive.parquet.time-zone; nested INT96 decode also receives the configured timezone. - Config/protocol compatibility: the new scan parameter is an optional thrift field; FE canonicalizes shorthand timezone values before sending them and BE validates/uses the configured zone when present.
- Concurrency/lifecycle: no new shared concurrency or lifecycle risk was identified; timezone state is carried on per-reader state and used within reader lifetimes.
- Data correctness: ORC timestamp rounding and SARG bounds appear conservative in the reviewed paths, and Parquet INT96 decode uses the catalog timezone in the covered scalar/nested paths.
- Transactions/persistence/writes: not applicable to this read-path change.
- Observability/performance: no new logging/metric requirement was identified; pruning remains conservative for the timestamp edge cases reviewed.
- Validation: static review only. I did not run BE/FE builds or tests because this checkout is missing
thirdparty/installedandthirdparty/installed/bin/protoc.
Subagent conclusions:
- optimizer-rewrite: no candidates; convergence round returned
NO_NEW_VALUABLE_FINDINGS. - tests-session-config: no candidates; convergence round returned
NO_NEW_VALUABLE_FINDINGS.
Existing review threads were treated as already-submitted context and were not duplicated.
|
/review |
There was a problem hiding this comment.
Automated review complete for head d53edf539f1fd5cc44b8d796f52694727510470f.
I found no new inline findings after duplicate suppression. The one verified defect I still see is the EXPORT enable_int96_timestamps strict-boolean validation gap, but it is already covered by existing inline thread discussion_r3569980146, so I did not add a duplicate comment. The earlier mixed-case export-property candidate was dismissed because ExportCommand.fileProperties is an ImmutableSortedMap with String.CASE_INSENSITIVE_ORDER.
Checkpoint summary:
- Scope/focus: no additional user focus was provided. I reviewed ORC timestamp rounding/SARG/aggregate fallback, Parquet INT96 timezone propagation, FE Hive/TVF property scoping and canonicalization, export-property persistence, and changed tests/baselines.
- Correctness/compatibility: ORC unsafe pushdown paths now fall back; Parquet INT96 scanner-v2 propagation reaches the intended top-level and nested DATETIMEV2 readers while TIMESTAMPTZ and metadata-stat paths stay conservative; FE sends canonical timezone values only on the intended Hive/TVF paths.
- Lifecycle/concurrency/performance: I found no new shared concurrency issue. Parquet timezone parsing is once per reader with stable scan-state lifetime, and I found no per-row timezone parsing.
- Tests: I reviewed the added unit/regression coverage statically. I did not run builds or tests because this review prompt is review-only and the checkout is not worktree-initialized (
.worktree_initialized/thirdparty setup unavailable).
Convergence: main review plus two subagent rounds completed. Round 2 full-diff, candidate/dedup, and risk-sweep agents all returned NO_NEW_VALUABLE_FINDINGS against the final ledger/comment set.
924060929
left a comment
924060929
left a comment
There was a problem hiding this comment.
Thanks for taking this on — the INT96 problem is real, and following Trino's explicit hive.parquet.time-zone model rather than guessing the writer convention from metadata is the right call. Excluding Iceberg (and testing it) is a nice catch too.
I reviewed this focusing on the FE side. The main issue I found is cross-cutting, and I think it blocks the merge.
1. Doris can no longer read back its own default Parquet export
The write and read paths now disagree on INT96 by default.
Write (not touched by this PR): enable_int96_timestamps defaults to true (ParquetFileFormatProperties.java:64), and vparquet_transformer.cpp:241 builds the arrow type with _state->timezone(), so the INT96 that gets written is a UTC-normalized instant.
Read (changed by this PR): parquet_leaf_reader.cpp:387
view.timezone = batch._value_kind == DecodedValueKind::INT96 ? _int96_timezone : _timezone;_int96_timezone stays nullptr unless hive.parquet.time-zone is set (parquet_reader.cpp:356, and :653 passes nullptr otherwise), so INT96 is read as a raw wall clock.
Round trip on default settings with time_zone = Asia/Shanghai:
INSERT INTO t VALUES (1, '2017-10-01 00:00:00');
SELECT * FROM t INTO OUTFILE "s3://.../" FORMAT AS PARQUET PROPERTIES(...); -- INT96 = 2017-09-30T16:00:00Z
SELECT * FROM S3("uri" = ".../0.parquet", "format" = "parquet", ...);
-- before: 2017-10-01 00:00:00
-- after: 2017-09-30 16:00:00 <-- silent -8hThis PR's own diff is the evidence:
- Pre-PR
test_outfile_parquet_array_type.outhadselect_base_datetime=2017-10-01 00:00:00andselect_load_datetime=2017-10-01 00:00:00.000000. The round trip was exact — writer-UTC and reader-session-tz cancelled out. - This PR adds
"enable_int96_timestamps" = "false"to 8 export tests (test_show_export×2,test_outfile_parquet{,_array_type,_complex_type,_map_type},test_export_data_types,test_export_parquet,test_export_external_table). test_show_export.outis not regenerated — it still reads2017-10-01T00:00. That is only possible because the groovy now forces INT96 off; on the old default that line would have become2017-09-30T16:00.- The
.outfiles that were regenerated go.000000→.000. That is the INT96→INT64 type change (scale-0datetime→ arrow SECOND →TIMESTAMP_MILLIS), i.e. those files now record the output of a different statement, not a corrected value.
Net effect: after this PR, no test covers DATETIME with the default INT96 write→read path. The only surviving OUTFILE test on the true default is test_iceberg_export_timestamp_tz.groovy, whose columns are timestamptz, which the leaf reader explicitly exempts.
test_hive_read_parquet.groovy states the conflict out loud — the same table now has to be exported twice, INT64 for Doris to read and INT96 for Hive to read.
Could you confirm whether the writer staying UTC-normalized while the reader moves to raw wall clock is intentional? Two ways out:
- (a) Reader default stays on session tz;
hive.parquet.time-zonebecomes a pure opt-in override for Trino/UTC-style writers. Worth noting Trino's ownhive.parquet.time-zonedefaults to the JVM zone, not to "no conversion" — so""meaning UTC is a defensible but distinct choice. - (b) Keep the new reader default and stop normalizing on the INT96 write path so the round trip closes — but that breaks Hive/Impala/Spark interop in the other direction, so probably not what you want.
Either way, please keep at least one regression test on the default round trip.
2. v1 and v2 now return different timestamps for the same file
enable_file_scanner_v2 is a session variable defaulting to true (SessionVariable.java:1172), and it is the standard escape hatch when someone hits a v2 issue. But v1 still applies the session timezone to INT96:
file_scanner.cpp:1373passes&_state->timezone_obj()into the v1 reader- INT96 carries no
logicalType, so theisAdjustedToUTCbranch inConvertParams::initnever fires andctzstays the session tz parquet_column_convert.h:877Int96toTimestamp→from_unixtime(epoch_seconds, *_convert_params->ctz)
So SET enable_file_scanner_v2 = false silently shifts INT96 by 8h. I don't think "v1/v2 behavioral parity is outside this PR's scope" holds for a user-flippable, default-on session variable — at minimum this deserves an explicit callout.
3. Every existing Hive INT96 user changes behavior on upgrade
This PR adds hive.parquet.time-zone = 'Asia/Shanghai' to 17 suites, 14 of them Hive catalogs — and zero .out files under regression-test/data/external_table_p0/hive/ changed. The property was added purely to keep the existing expected output valid; without it those 14 suites would fail.
Real users' Hive catalogs are no different from those tests. There is no session-level switch and no error — just an 8h shift until they run per-catalog DDL. If that is the intended migration, it needs a prominent release note and an upgrade guide, not only "Behavior changed: Yes".
Minor but useful detail: the observed shift in test_hdfs_parquet_group2.out is 17:07:46 → 09:02:03 = 8h05m43s, not a round 8h — that is Asia/Shanghai's historical LMT offset for year 1001. Users will report odd numbers like this.
4. CST is silently remapped to Asia/Shanghai
getHiveParquetTimeZone() resolves via ZoneId.of(std, TimeUtils.timeZoneAliasMap), and TimeUtils.java:111:
// set CST to +08:00 instead of America/Chicago
timeZoneMap.put("CST", DEFAULT_TIME_ZONE); // Asia/Shanghai
timeZoneMap.put("PRC", DEFAULT_TIME_ZONE);A user migrating from Trino who sets hive.parquet.time-zone = 'CST' meaning US Central gets +08:00 — a silent 14h skew, accepted without error. This property exists precisely to prevent that class of bug.
Related: the error message says the value "must be an IANA timezone or UTC offset", but testHiveParquetTimeZoneCanonicalizesJavaShortAlias asserts AET is accepted, and AET/CST/PRC are not IANA IDs — message and behavior disagree. Suggest ZoneId.of(std) without the alias map, or explicitly rejecting ZoneId.SHORT_IDS keys.
FE details (non-blocking)
- Duplicated constant.
FileFormatConstants.PROP_HIVE_PARQUET_TIME_ZONEandHMSExternalCatalog.HIVE_PARQUET_TIME_ZONEhold the same literal with no cross-reference and no test pinning them equal. Note the sibling convention is deliberately the opposite:enable_mapping_varbinary(TVF) vsenable.mapping.varbinary(catalog) are different strings on purpose. - Duplicated validation. The ~10-line validate/canonicalize block is byte-identical in
HMSExternalCatalogandExternalFileTableValuedFunction. The TVF copy has no negative test; the catalog one does. - Layering.
FileQueryScanNode(the generic base) now doesinstanceof HMSExternalTable/HMSExternalCatalog. The siblinggetEnableMappingTimestampTz()goes through genericExternalTable/ExternalCatalog+CatalogProperty. Canonicalizing once at DDL time and storing the result inCatalogPropertywould fix this and the next point together. - Query-time
DdlException.getHiveParquetTimeZone()re-parses and re-validates on everyinitSchemaParams().CatalogMgr.java:559skipscheckProperties()on replay/image load, and unknown catalog keys aren't rejected at CREATE — so a bad value persisted by an older FE would make every query on that catalog fail after upgrade, rather than failing at DDL. - HUDI gap. The new javadoc says a configured value "applies to every INT96 TIMESTAMP file in this catalog", but the gate is
DLAType.HIVEonly. Spark-written Hudi COW base files (INT96 was Spark's pre-3.0 default) in the same HMS catalog get no correction, while byte-identical files registered as a Hive table do. Iceberg exclusion is correct and tested; Hudi looks unintentional — either include it or narrow the javadoc. getFetchTableStructureRequestreads the private field rather than the overridablegetHiveParquetTimeZone(). Works today only becauseFileTableValuedFunction.getTableColumns()also delegates, so schema fetch happens on the delegate. Latent.enable_int96_timestampshas no value validation.Boolean.valueOfgivesfalsefor"yes","1"," true". Unlikewith_bom, garbage here flips away from atruedefault and silently produces INT64 that Hive 2/3 cannot read at all. AlsoconvertOutfileProperties()adds the key unconditionally, unlike theformat-gated block above it, soformat=csv+enable_int96_timestampsis now silently accepted and ignored (previouslyInvalid property key). Consider a strict boolean parse inParquetFileFormatProperties.analyzeFileFormatProperties, and surfacing the value inSHOW EXPORT(ExportMgr.composeExportJobInfo) — an operator currently cannot tell whether a finished job produced INT96 or INT64.HiveScanNodeTest.testHiveParquetTimeZoneComesFromCatalogInsteadOfSessionstubsgetHiveParquetTimeZone()itself and never sets or varies a session timezone, so it does not test what its name claims.- Two
.outfiles (test_hdfs_parquet_group4.out,test_hdfs_orc_group6_orc_files.out) are binary and unreviewable in the diff. I decoded them separately — they look consistent with the intended change (8h shift; ORC07:04:05.999999→07:04:06second carry) — but flagging since reviewers can't see them.
Merge state
The base predates #65562 ("fix Fix ORC V2 pushdown correctness and fallback"), which has landed on master and also touches be/src/format_v2/orc/orc_search_argument.cpp. git merge-tree against current master reports conflicts in 9 files, so this needs a rebase and the ORC SARG portion may now partly overlap. Thrift field 34 is still free on master — no id collision.
Issue Number: None Related PR: apache#65446 Problem Summary: Timestamp decoding had two correctness problems in scanner v2. Parquet INT96 does not carry a timezone annotation, so applying the Doris SQL session timezone shifted raw wall-clock fields written by Trino/UTC-style writers. ORC nanoseconds were truncated inconsistently across row decoding, statistics, and SARG predicates, which could also produce unsafe pruning at millisecond statistic boundaries. Add an explicit `hive.parquet.time-zone` compatibility property for scanner-v2 Parquet INT96 reads. By default scanner v2 preserves raw INT96 wall-clock fields; when configured, it interprets them with the requested timezone. The legacy v1 reader remains unchanged, INT64 logical timestamps retain their standard behavior, and INT96 values mapped to TIMESTAMPTZ preserve their UTC instant. Apply half-up nanosecond rounding consistently to ORC rows and statistics, use conservative SARG bounds, and fall back to exact row filtering where negating a widened timestamp envelope would be unsafe. Update export and interoperability coverage so Doris paths exercise INT64 logical timestamps, legacy Hive 2/3 compatibility checks use INT96 only where required, and expected timestamp precision reflects the physical Parquet time unit without changing timezone-offset expectations. Make external regression cases deterministic by explicitly selecting the compatibility timezone for legacy INT96 fixtures and the expected session timezone for Paimon timestamp_ltz data. Add `hive.parquet.time-zone` for scanner-v2 Parquet INT96 compatibility and fix ORC timestamp rounding and SARG pruning. - Test: - Build: `./build.sh --fe` passed on the designated remote host. - Unit Test: targeted Parquet and ORC BE unit tests passed on the designated remote host. - Unit Test: FileQueryScanNodeTest, HiveScanNodeTest, and ExternalFileTableValuedFunctionTest passed on the designated remote host. - Regression test: test_hive_read_parquet and test_hive_read_parquet_complex_type passed on the designated remote host. - Regression test: test_hdfs_parquet_group0 passed completely on an isolated FileScannerV2 cluster on the designated remote host. - Regression test: paimon_timestamp_types passed completely for JNI and native readers on the isolated cluster on the designated remote host. - Regression test: TeamCity builds 994160 and 994162 produced matching millisecond-precision real results for the updated ARRAY, MAP, and STRUCT expectations. - Regression test: the three S3 outfile suites were attempted on the designated remote host but were blocked before the changed assertions because the configured OSS credential returned InvalidAccessKeyId. - Behavior changed: Yes. Scanner v2 preserves raw INT96 wall-clock values by default, supports explicit timezone conversion, and ORC timestamp rounding and pruning are made consistent and conservative. - Does this need documentation: No
Gabriel39
force-pushed
the
agent/fix-format-v2-timestamp-decoding
branch
from
d53edf5 to
e7cffba
Compare
|
run buildall |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
TPC-H: Total hot run time: 29523 ms |
TPC-DS: Total hot run time: 178396 ms |
ClickBench: Total hot run time: 25.16 s |
### What problem does this PR solve? Issue Number: None Related PR: apache#65446 Problem Summary: Timestamp decoding had two correctness problems in scanner v2. Parquet INT96 does not carry a timezone annotation, so applying the Doris SQL session timezone shifted raw wall-clock fields written by Trino/UTC-style writers. ORC nanoseconds were truncated inconsistently across row decoding, statistics, and SARG predicates, which could also produce unsafe pruning at millisecond statistic boundaries. Add an explicit `hive.parquet.time-zone` property for scanner-v2 Parquet INT96 reads. By default scanner v2 preserves raw INT96 wall-clock fields; when configured, it interprets them with the requested unambiguous IANA timezone or valid UTC offset. Reject ambiguous short aliases such as `CST`, share timezone parsing across HMS catalogs and file TVFs, and propagate the setting through generic external-table and TVF interfaces for Hive and Hudi while leaving Iceberg semantics unchanged. Change Parquet export to write INT64 logical timestamps by default and require `enable_int96_timestamps` to be exactly `true` or `false`. Update export regressions to exercise the new default directly while retaining explicit INT96 only for legacy Hive interoperability coverage. Apply half-up nanosecond rounding consistently to ORC rows and statistics, use conservative SARG bounds, and fall back to exact row filtering where negating a widened timestamp envelope would be unsafe. The legacy v1 reader remains unchanged by design. ### Release note Parquet export now writes INT64 logical timestamps by default. Set `enable_int96_timestamps=true` to request INT96. The `hive.parquet.time-zone` property accepts unambiguous IANA timezone IDs or valid UTC offsets; ambiguous short aliases such as `CST` are rejected. ### Check List (For Author) - Test: - Build: `./build.sh --fe` passed on the designated remote host. - Checkstyle: `cd fe && mvn checkstyle:check -pl fe-core` passed on the designated remote host with 0 violations. - Unit Test: 39 targeted FE tests covering scan propagation, HMS/TVF timezone validation, Parquet properties, and export commands passed on the designated remote host. - Unit Test: targeted Parquet and ORC BE unit tests passed on the designated remote host. - Regression test: `test_outfile_parquet` passed on an isolated scanner-v2 cluster on the designated remote host and verified default Parquet export/stream-load round-trip results. - Regression test: `test_hive_read_parquet` and `test_hive_read_parquet_complex_type` passed on the designated remote host. - Regression test: `test_hdfs_parquet_group0` passed completely on an isolated scanner-v2 cluster on the designated remote host. - Regression test: `paimon_timestamp_types` passed completely for JNI and native readers on the isolated cluster on the designated remote host. - Regression test: TeamCity builds 994160 and 994162 produced matching millisecond-precision real results for the updated ARRAY, MAP, and STRUCT expectations. - Regression test: the three S3 outfile suites were attempted on the designated remote host but were blocked before the changed assertions because the configured OSS credential returned `InvalidAccessKeyId`. - Behavior changed: Yes. Scanner v2 preserves raw INT96 wall-clock values by default, Parquet export defaults to INT64 timestamps, timezone aliases are rejected, and ORC timestamp rounding and pruning are consistent and conservative. - Does this need documentation: Yes. apache/doris-website#3982
Gabriel39
force-pushed
the
agent/fix-format-v2-timestamp-decoding
branch
from
e7cffba to
4a6e5e7
Compare
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
|
@924060929 Thanks for the detailed review. I addressed the actionable items in commit
Remote validation completed:
|
|
run buildall |
924060929
left a comment
924060929
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround. The FE side is in much better shape: FileFormatUtils.parseHiveParquetTimeZone as a single shared validator, short aliases (AET/CST/PRC) now explicitly rejected with ZoneId.of() no longer going through the alias map, the ExternalTable/TableValuedFunctionIf base methods removing the HMS-specific instanceof from FileQueryScanNode, Hudi folded in with the javadoc updated to match, strict parseEnableInt96Timestamps validated eagerly in ExportCommand, and the rebase past #65562. That all reads well.
Three things on the new revision.
1. The export default flipped from INT96 to INT64, and nothing in the PR says so
ParquetFileFormatProperties.java:64:
- private boolean enableInt96Timestamps = true;
+ private boolean enableInt96Timestamps = false;This is how the round trip got closed — not by making the writer and reader agree, but by moving the default off the INT96 path. The write path itself is untouched (no transformer/arrow files in the PR), and all seven "enable_int96_timestamps" = "false" test compensations are gone because false is now the default.
The round trip does close for the default, and that's a real improvement. But this is now the single most user-visible change in the PR, and it is invisible to anyone reading it:
Behavior changed: Yeslists only the three read-side items. The default flip isn't there.- The release note says only "Add
hive.parquet.time-zonefor scanner-v2 Parquet INT96 compatibility and fix ORC timestamp rounding and SARG pruning." Does this need documentation: No, though apache/doris-website#3982 exists for exactly this.- The PR body still records
HiveScanNodeTestpassing (17 tests), butHiveScanNodeTest.javawas dropped from the PR in this revision.
What it costs users: Hive 2 and 3 can no longer read Doris's default Parquet export. That's your own comment in test_hive_read_parquet.groovy — "Hive 2 and 3 do not support Parquet INT64 logical timestamps." And true has been the default since #51384, backported to branch-3.1 (fdf5d055972), so this changes behavior that has been released since 3.1 — it isn't correcting a recent regression.
For a PR titled "Fix Parquet INT96 timezone decoding", flipping the FE export encoding default is a big rider. I'd suggest splitting it into its own PR: it's independently reviewable, it's the part most likely to need a revert, and bundled here it will not get the attention it deserves. At minimum it needs a release note and a migration note.
Related, and worth deciding explicitly: with enable_int96_timestamps = true — which Hive 2/3 interop still requires — the round trip is still broken. Writer normalizes to UTC, reader takes the raw wall clock, so Doris reading back its own INT96 export is off by the session offset unless hive.parquet.time-zone is also set. This is self-consistent with the PR's model (Doris's INT96 output is a Hive-convention file), but it's a sharp edge and it has no coverage: test_hive_read_parquet.groovy now exports twice, and the INT96 file is only ever read by Hive (qt_hive_docker_02), never by Doris. The comment documents the gap rather than closing it. A test that reads the INT96 export back through Doris with hive.parquet.time-zone set would pin the intended contract.
2. apache/doris-website#3982 is now factually wrong
That PR is still on 66d97811 (last updated 2026-07-14) and this revision landed after it, so two of its claims now contradict the code — and both were correct against the previous revision:
- "The default is
true, which writesINT96" / "默认值为true，写入INT96" — nowfalse. - "It does not affect Hudi or Iceberg tables in the same Hive Metastore" / "不影响同一 Hive Metastore 中的 Hudi 或 Iceberg 表" — Hudi now does get the property, and this PR's own javadoc says "applies to Hive and Hudi Parquet files in this catalog".
Its advice to "disable INT96 output with enable_int96_timestamps = false" is also redundant now that it's the default. The two PRs need to move together.
3. Orphaned javadoc
HMSExternalCatalog.java:70-78 — removing the duplicate HIVE_PARQUET_TIME_ZONE constant (good change) left its javadoc behind, and it now documents the thread-pool size:
/**
* Timezone used to reverse legacy Parquet INT96 writer normalization. This intentionally
* follows Trino's hive.parquet.time-zone catalog property instead of the SQL session timezone.
* ...
*/
private static final int FILE_SYSTEM_EXECUTOR_THREAD_NUM = 16;Still open from the previous round
Both unchanged, both previously discussed — restating only so they're not lost:
- v1/v2 divergence.
SET enable_file_scanner_v2 = falsestill shifts INT96 by the session offset, on a session variable that defaults totrueand is the standard escape hatch for v2 issues. - Existing Hive catalogs. 14 Hive suites still carry
hive.parquet.time-zone = 'Asia/Shanghai'purely to keep their expected output valid, andregression-test/data/external_table_p0/hive/still has zero.outchanges. Real users' catalogs are in the same position: an 8h shift on upgrade, no error, no session-level switch. That needs an upgrade note whatever else happens here.
Also minor, from last round: HMSExternalCatalog.getHiveParquetTimeZone() still re-parses and re-validates on every initSchemaParams(), and checkProperties() is skipped on replay/image load. Canonicalizing once at DDL time and storing the result in CatalogProperty would make the query path a plain map read and remove the query-time DdlException risk.
FE UT Coverage ReportIncrement line coverage |
### What problem does this PR solve? Issue Number: None Related PR: apache#65446 Problem Summary: Timestamp decoding had two correctness problems in scanner v2. Parquet INT96 does not carry a timezone annotation, so applying the Doris SQL session timezone shifted raw wall-clock fields written by Trino/UTC-style writers. ORC nanoseconds were truncated inconsistently across row decoding, statistics, and SARG predicates, which could also produce unsafe pruning at millisecond statistic boundaries. Add an explicit `hive.parquet.time-zone` property for scanner-v2 Parquet INT96 reads. By default scanner v2 preserves raw INT96 wall-clock fields; when configured, it interprets them with the requested unambiguous IANA timezone or valid UTC offset. Reject ambiguous short aliases such as `CST`, share timezone parsing across HMS catalogs and file TVFs, and propagate the setting through generic external-table and TVF interfaces for Hive and Hudi while leaving Iceberg semantics unchanged. Change Parquet export to write INT64 logical timestamps by default and require `enable_int96_timestamps` to be exactly `true` or `false`. Update export regressions to exercise the new default directly while retaining explicit INT96 only for legacy Hive interoperability coverage. Add an end-to-end regression that exports INT96 in one session timezone and reads it through scanner v2 in another session timezone with `hive.parquet.time-zone` configured. Apply half-up nanosecond rounding consistently to ORC rows and statistics, use conservative SARG bounds, and fall back to exact row filtering where negating a widened timestamp envelope would be unsafe. The legacy v1 reader remains unchanged by design. ### Release note Parquet export now writes INT64 logical timestamps by default. Set `enable_int96_timestamps=true` to request INT96. When scanner v2 reads legacy INT96 files that were normalized by a writer timezone, set `hive.parquet.time-zone` to that timezone. The property accepts unambiguous IANA timezone IDs or valid UTC offsets; ambiguous short aliases such as `CST` are rejected. Existing Hive and Hudi catalogs containing normalized INT96 files should be checked during upgrade. ### Check List (For Author) - Test: - Build: `./build.sh --fe` passed on the designated remote host. - Checkstyle: `cd fe && mvn checkstyle:check -pl fe-core` passed on the designated remote host with 0 violations. - Unit Test: 39 targeted FE tests covering scan propagation, HMS/TVF timezone validation, Parquet properties, and export commands passed on the designated remote host. - Unit Test: targeted Parquet and ORC BE unit tests passed on the designated remote host. - Regression test: `test_outfile_parquet` passed on an isolated scanner-v2 cluster on the designated remote host and verified default Parquet export/stream-load round-trip results. - Regression test: `test_hive_read_parquet` and `test_hive_read_parquet_complex_type` passed on the designated remote host. - Regression test: `test_doris_int96_round_trip` passed in generated-output and comparison modes on the designated remote host, verifying that the configured compatibility timezone overrides a different read session timezone. - Regression test: `test_hdfs_parquet_group0` passed completely on an isolated scanner-v2 cluster on the designated remote host. - Regression test: `paimon_timestamp_types` passed completely for JNI and native readers on the isolated cluster on the designated remote host. - Regression test: TeamCity builds 994160 and 994162 produced matching millisecond-precision real results for the updated ARRAY, MAP, and STRUCT expectations. - Regression test: the three S3 outfile suites were attempted on the designated remote host but were blocked before the changed assertions because the configured OSS credential returned `InvalidAccessKeyId`. - Behavior changed: Yes. Scanner v2 preserves raw INT96 wall-clock values by default, Parquet export defaults to INT64 timestamps, timezone aliases are rejected, and ORC timestamp rounding and pruning are consistent and conservative. - Does this need documentation: Yes. apache/doris-website#3982
Gabriel39
force-pushed
the
agent/fix-format-v2-timestamp-decoding
branch
from
4a6e5e7 to
ef8f1e8
Compare
Gabriel39
changed the title
|
已按这轮 review 的前 5 项处理，最后一项 catalog replay/query-time 重复解析留到后续 PR：
本次 Doris 更新已 squash 为单提交 按约定， |
|
/review |
|
run buildall |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
There was a problem hiding this comment.
Reviewed the current PR head against the provided authoritative diff and existing review threads. I did not find additional actionable issues beyond the already-known threads.
Critical checkpoint conclusions:
- Goal/scope: the PR focuses on scanner-v2 Parquet INT96 timezone compatibility, ORC timestamp rounding/SARG safety, and Parquet export INT96 defaults/properties. The current changes appear focused for that scope.
- Data correctness: scanner-v2 Parquet INT96 DATETIMEV2 materialization now receives
hive.parquet.time-zonethrough the FE -> thrift -> TableReader -> ParquetReader -> scheduler -> leaf-reader chain, including nested values. TIMESTAMPTZ and non-INT96 paths remain unaffected. ORC timestamp row decoding, statistics conversion, SARG envelopes, and aggregate pushdown fallbacks appear conservative for the reviewed TIMESTAMP/TIMESTAMP_INSTANT cases. - Compatibility/scope: HMS Iceberg is excluded from the Hive Parquet timezone property; legacy scanner-v1 parity remains an already-discussed non-scope item in the existing threads.
- Persistence/property handling:
enable_int96_timestampsis strictly parsed before EXPORT job creation, persisted through the job, and replayed back into OUTFILE properties when explicitly set. The default Parquet writer behavior is updated and covered by tests/regression outputs. - Concurrency/lifecycle: I did not find new concurrency-sensitive shared state. The parsed Parquet INT96 timezone is owned by the reader scan state and borrowed only during scanner-v2 reader lifetime.
- Tests: the PR adds/updates focused BE/FE unit tests and regression expectations for the changed timestamp behavior. I did not run builds or tests in this review environment.
- User focus: no additional user-provided review focus was present.
The round-1 full-review subagents and risk-focused subagents all returned NO_NEW_VALUABLE_FINDINGS, and the main-agent reconciliation found no unresolved candidate points.
|
run buildall |
TPC-H: Total hot run time: 29553 ms |
TPC-DS: Total hot run time: 177910 ms |
ClickBench: Total hot run time: 24.9 s |
3 participants
What problem does this PR solve?
Issue Number: None
Problem Summary:
Timestamp decoding had two correctness problems in scanner v2:
This PR follows Trino's explicit connector-configuration model and adds
hive.parquet.time-zonefor scanner v2:-12:00..+14:00: convert INT96TIMESTAMPvalues with that configured timezone.8:00is canonicalized to+08:00; ambiguous short aliases such asCSTare rejected.TIMESTAMPTZkeeps the same UTC instant and ignores this compatibility conversion.The legacy v1 Parquet reader is intentionally unchanged and can return different results. v1/v2 behavioral parity is outside this PR's scope, and switching to v1 is not the migration mechanism for this change.
Parquet Export and Outfile now write INT64 logical timestamps by default. Set
enable_int96_timestamps=trueonly when a legacy downstream reader such as Hive 2 or Hive 3 requires INT96. Doris INT96 export normalizes timestamps using the export session timezone; when scanner v2 reads that file back, configurehive.parquet.time-zonewith the export session timezone. An end-to-end regression exports underAsia/Shanghai, reads under anAmerica/Los_Angelessession, and verifies that the compatibility property restores the original wall-clock values.For existing Hive and Hudi catalogs, upgrading users should inspect representative INT96 files. If a legacy writer normalized timestamps using a known timezone, set the catalog property before relying on scanner-v2 results:
For ORC, this PR rounds nanoseconds to microseconds with second carry for both
DATETIMEV2andTIMESTAMPTZ, applies the same rounding to statistics, and uses conservative millisecond SARG boundaries. TimestampNE,NOT IN, and negated timestamp subtrees fall back to exact Doris row filtering because negating a widened statistics envelope is unsafe.Release note
Parquet Export and Outfile now write INT64 logical timestamps by default. Set
enable_int96_timestamps=trueto request INT96 for legacy readers. Scanner v2 preserves raw Parquet INT96 wall-clock values by default; sethive.parquet.time-zonefor Hive/Hudi files normalized by a known writer timezone. Existing Hive and Hudi catalogs containing normalized INT96 files should be checked during upgrade. FileScannerV1 remains unchanged. ORC timestamp nanoseconds are rounded consistently to microseconds and timestamp SARG pruning is conservative.Check List (For Author)
gabriel@10.26.20.3:/mnt/disk3/gabriel/Workspace/dev2/doris:./build.sh --fepassed.cd fe && mvn checkstyle:check -pl fe-corepassed with 0 violations.test_outfile_parquet,test_hive_read_parquet,test_hive_read_parquet_complex_type,test_hdfs_parquet_group0, andpaimon_timestamp_typespassed in their validated environments.test_doris_int96_round_trippassed in generated-output and comparison modes. It verifies that the configured compatibility timezone, rather than a different read session timezone, restores explicitly exported INT96 values.InvalidAccessKeyId; no expected output was generated manually.