Gabriel39 commented on PR #65446: URL: https://github.com/apache/doris/pull/65446#issuecomment-4977678332
@924060929 Thanks for the detailed review. I addressed the actionable items in commit `4a6e5e747b0` and revalidated the updated behavior. Here is the item-by-item result for [this review](https://github.com/apache/doris/pull/65446#pullrequestreview-4700903110): 1. **Default Parquet export round trip** — Fixed. `enable_int96_timestamps` now defaults to `false`, so Doris writes INT64 logical timestamps unless the user explicitly requests INT96. The affected export/OUTFILE regressions no longer set `false` explicitly, which makes them exercise the real default. Legacy Hive interoperability branches still set `true` explicitly where Hive INT96 is required. The default `test_outfile_parquet` export -> stream-load round trip passed on an isolated scanner-v2 cluster. 2. **Scanner v1/v2 behavior difference** — Intentionally unchanged. This PR defines the expected scanner-v2 behavior; maintaining parity with the old scanner-v1 interpretation is not a goal. 3. **Existing Hive INT96 upgrade compatibility** — Intentionally no legacy fallback or migration mode was added. The previous implicit session-timezone conversion was incorrect, so scanner v2 now preserves the raw wall-clock value unless the catalog/TVF explicitly configures `hive.parquet.time-zone`. 4. **`CST` mapping to `Asia/Shanghai`** — Fixed by removing Doris/Java short-alias resolution from this property. `CST`, `PRC`, `AET`, and other ambiguous short aliases are rejected. Users must specify an unambiguous IANA ID such as `Asia/Shanghai`/`America/Chicago`, or a valid UTC offset. `UTC` and `GMT` remain valid. 5. **Duplicated property constant and timezone validation** — Fixed. The property name comes only from `FileFormatConstants.PROP_HIVE_PARQUET_TIME_ZONE`, and HMS catalogs plus file TVFs share `FileFormatUtils.parseHiveParquetTimeZone`. UTC/GMT-prefixed offsets are also checked against the `-12:00` to `+14:00` range. 6. **Generic scan-node dependency on HMS concrete classes** — Fixed. `FileQueryScanNode` now consumes generic `ExternalTable`/`TableValuedFunctionIf` accessors and no longer imports or branches on `HMSExternalCatalog`/`HMSExternalTable`. 7. **Hudi did not receive the catalog property** — Fixed. HMS-backed Hive and Hudi tables propagate the property; HMS Iceberg explicitly ignores it because Iceberg defines its own timestamp semantics. 8. **Fetch-schema path read the private TVF field directly** — Fixed. It now obtains the value through the overridable accessor, matching the scan path. 9. **`Boolean.valueOf("yes")` silently became `false`** — Fixed. `enable_int96_timestamps` now accepts only case-insensitive `true` or `false`; values such as `yes`, `1`, and whitespace-padded values fail analysis. I did not add a separate CSV-only rejection or new `SHOW EXPORT` column in this change; the correctness fix is strict validation plus the new INT64 default. 10. **Session-timezone test did not actually verify session independence** — Fixed. The test now creates two scan nodes with `America/Los_Angeles` and `Asia/Tokyo` sessions and verifies both receive the same catalog `+08:00` value. Separate tests cover Hive, Hudi, Iceberg, file TVFs, and unrelated TVFs. 11. **Query-time validation failure concern** — DDL/catalog property validation still rejects invalid values during catalog checking. The shared accessor intentionally continues validating when materializing scan parameters so replayed or otherwise pre-existing invalid metadata fails explicitly instead of being silently accepted. 12. **Regression output changes** — This follow-up does not change `.out` files for the default-export switch; the existing expected values remain unchanged. Earlier precision-only expected-result updates remain, while timezone-offset expectations are preserved. 13. **Merge state** — Rebased on current `master`; the PR now contains one squashed commit. Remote validation completed: - `cd fe && mvn checkstyle:check -pl fe-core`: passed, 0 violations. - Targeted FE unit tests: 39 passed, 0 failures/errors. - `./build.sh --fe`: passed. - `./run-regression-test.sh --run -d export_p0/outfile/parquet -s test_outfile_parquet`: passed, 1 suite/0 failures. -- 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]
