Skip to content

fix: support parquet cross-type conversions - #25510

Merged
XuPeng-SH merged 6 commits into
matrixorigin:4.1-devfrom
iamlinjunhong:d1-24914
Jul 13, 2026
Merged

fix: support parquet cross-type conversions#25510
XuPeng-SH merged 6 commits into
matrixorigin:4.1-devfrom
iamlinjunhong:d1-24914

Conversation

@iamlinjunhong

Copy link
Copy Markdown
Contributor

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #24914

What this PR does / why we need it:

fix: support parquet cross-type conversions

@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@mergify
mergify Bot requested a review from XuPeng-SH July 7, 2026 08:29
@matrix-meow matrix-meow added the size/L Denotes a PR that changes [500,999] lines label Jul 7, 2026
@matrix-meow matrix-meow added size/XL Denotes a PR that changes [1000, 1999] lines and removed size/L Denotes a PR that changes [500,999] lines labels Jul 7, 2026
@iamlinjunhong

Copy link
Copy Markdown
Contributor Author

I found one blocking correctness issue in the new Parquet cross-type conversion path.

For Parquet timestamps with isAdjustedToUTC=false, the new TIMESTAMP mapping computes the timezone offset with time.Now().In(loc).Zone(). That makes the result depend on the wall clock of the load job instead of the wall clock of the value being decoded. In DST-sensitive zones this is wrong for historical/future data: e.g. a January timestamp loaded in July in America/New_York will use -04:00 instead of the value's real -05:00 and land one hour off.

This path is newly introduced here and the current coverage doesn't catch it because the tests/BVT run in UTC. I'd fix it before merge by deriving the offset from the represented local instant itself, then add a regression test with a non-UTC session timezone and a value that crosses a DST boundary.

fixed

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request changes on the latest head. Two newly supported cross-type paths still have reproducible correctness failures:

  1. [High] TIMESTAMP(isAdjustedToUTC=true) -> DATETIME ignores the session timezone. The DATETIME mapper at parquet.go:1268-1323 directly converts the physical Unix value with types.Datetime(types.Unix*ToTimestamp(...)) for both plain and dictionary pages. It never inspects dtT.IsAdjustedToUTC and never uses the session location. This is inconsistent with the new TIMESTAMP -> DATE/TIME paths, which correctly call parquetTimestampValueToDatetime; that helper converts an adjusted UTC instant through ts.ToDatetime(loc). I reproduced this on the current head: loading 2024-01-01 00:00:00 UTC into DATETIME with Asia/Shanghai produces 00:00:00, while the correct session-local DATETIME is 08:00:00. Please route all timestamp units and both encodings through the common adjusted/unadjusted helper and add non-UTC tests for adjusted=true and adjusted=false.

  2. [High] DOUBLE/DECIMAL -> FLOAT32 silently writes infinity instead of rejecting overflow. The scalar FLOAT32 mapper at parquet.go:966-970 narrows with float32(val) without checking abs(val) > math.MaxFloat32; the new LIST -> VECF32 mapper at parquet.go:428-432 has the same issue. A finite Parquet DOUBLE value 1e100 maps successfully with nil error and becomes +Inf. MatrixOne cast handling and the STRING -> FLOAT32 path reject out-of-range values, so LOAD DATA must not silently manufacture infinity. Please centralize a checked FLOAT32 conversion and use it for scalar decimal/double sources and list-double elements. Add positive/negative overflow tests, including dictionary/plain scalar pages and LIST.

The current CI and the complete pkg/sql/colexec/external test suite pass, but the BVT fixes time_zone to UTC and uses only small float values, so neither failure is covered. I also checked NULL/error page cleanup, dictionary index validation, page-bounded allocations, and synchronous termination paths; I found no additional lifecycle/hang/OOM blocker.

@iamlinjunhong

Copy link
Copy Markdown
Contributor Author

Request changes on the latest head. Two newly supported cross-type paths still have reproducible correctness failures:

  1. [High] TIMESTAMP(isAdjustedToUTC=true) -> DATETIME ignores the session timezone. The DATETIME mapper at parquet.go:1268-1323 directly converts the physical Unix value with types.Datetime(types.Unix*ToTimestamp(...)) for both plain and dictionary pages. It never inspects dtT.IsAdjustedToUTC and never uses the session location. This is inconsistent with the new TIMESTAMP -> DATE/TIME paths, which correctly call parquetTimestampValueToDatetime; that helper converts an adjusted UTC instant through ts.ToDatetime(loc). I reproduced this on the current head: loading 2024-01-01 00:00:00 UTC into DATETIME with Asia/Shanghai produces 00:00:00, while the correct session-local DATETIME is 08:00:00. Please route all timestamp units and both encodings through the common adjusted/unadjusted helper and add non-UTC tests for adjusted=true and adjusted=false.
  2. [High] DOUBLE/DECIMAL -> FLOAT32 silently writes infinity instead of rejecting overflow. The scalar FLOAT32 mapper at parquet.go:966-970 narrows with float32(val) without checking abs(val) > math.MaxFloat32; the new LIST -> VECF32 mapper at parquet.go:428-432 has the same issue. A finite Parquet DOUBLE value 1e100 maps successfully with nil error and becomes +Inf. MatrixOne cast handling and the STRING -> FLOAT32 path reject out-of-range values, so LOAD DATA must not silently manufacture infinity. Please centralize a checked FLOAT32 conversion and use it for scalar decimal/double sources and list-double elements. Add positive/negative overflow tests, including dictionary/plain scalar pages and LIST.

The current CI and the complete pkg/sql/colexec/external test suite pass, but the BVT fixes time_zone to UTC and uses only small float values, so neither failure is covered. I also checked NULL/error page cleanup, dictionary index validation, page-bounded allocations, and synchronous termination paths; I found no additional lifecycle/hang/OOM blocker.

fixed

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

最新 head 看下来,我上次卡的两个 correctness blocker 都已经补齐了,可以 approve。

  • TIMESTAMP -> DATETIME/TIME/DATE 现在统一走 parquetTimestampValueToDatetime(...) / 本地时区 helper,isAdjustedToUTC=true/false 的路径都收口了;America/New_York 的 DST 回归测试也补进来了。
  • DOUBLE/DECIMAL -> FLOAT32 以及 LIST<DOUBLE> -> VECF32 现在都走带范围检查的 parquetFloat64ToFloat32(...),不再静默写 Inf;正负溢出 UT 也补了。

按当前 scope,我这里没有剩余 blocker 了。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XL Denotes a PR that changes [1000, 1999] lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants