Repository navigation
[fix](catalog) Keep the TIME scale that connectors declare - #70351
Open
morningman wants to merge 1 commit into
Open
morningman wants to merge 1 commit into
morningman wants to merge 1 commit into
Conversation
### Background
Connector plugins (Hudi, and the coming Lance connector) describe their columns with the SPI's `ConnectorType`, and fe-core's `ConnectorColumnConverter` turns each into a Doris type. Connectors put a temporal column's fractional-second scale in the type's precision field: `("DATETIMEV2", 3, 0)` is `datetime(3)`, and Hudi's `HudiTypeMapping` sends `("TIMEV2", 3, 0)` for an Avro `time-millis` column and `("TIMEV2", 6, 0)` for `time-micros`.
### What problem does this PR solve?
Issue Number: None
Related PR: apache#62183 (introduced the converter without a TIMEV2 case), apache#64304 (moved Hudi onto it)
PR Type: Bugfix
Problem Summary:
Since Hudi became a connector plugin, its `time-millis` and `time-micros` columns show as `time` instead of `time(3)` and `time(6)`, which is what 4.x shows and what `HudiTypeMapping` asks for: the converter drops the scale and builds a scale-0 TIMEV2. The Lance connector, ported in a follow-up, maps Arrow `time32(ms)` and `time64` the same way and would lose its scale too. The converter now reads the TIMEV2 scale from the precision field, as it already does for DATETIMEV2.
### Details
- **Frequency:** Always, for every connector column whose `ConnectorType` is TIMEV2 with a scale; today that is every Hudi `time-millis` / `time-micros` column on master. Found while porting the Lance connector, by reading the converter, and confirmed by the unit test below.
- **Root cause:** `ConnectorColumnConverter.convertScalarType` reads the precision field for CHAR, VARCHAR, VARBINARY, the decimals, DATETIMEV2 and TIMESTAMPTZ, but has no TIMEV2 case, so TIMEV2 falls through to `ScalarType.createType("TIMEV2")`, which returns the scale-0 `ScalarType.TIMEV2`.
- **Why existing tests missed it:** Unit tests: `ConnectorColumnConverterTest` checks decimals, CHAR/VARCHAR lengths and the TIMESTAMPTZ extra info but has no TIMEV2 case, and `HudiTypeMappingTest` asserts the `("TIMEV2", 3, 0)` the plugin emits but stops before fe-core converts it. Regression: no suite under `regression-test/suites` reads a Hudi time column.
- **Fix:** The TIMEV2 case uses the precision field as the scale when one is given; without one the type stays the scale-0 TIMEV2 it has always been. The fix sits in the converter because every connector goes through it. The reverse direction, `toConnectorType`, is unchanged: only the read-only Hudi and Lance connectors produce TIMEV2, and the JDBC connector maps TIME to STRING.
- **Other paths and branches:** `toConnectorType` is not affected for reads (see Fix). branch-4.1 and branch-4.2 are not affected: they have no `ConnectorColumnConverter`, and their fe-core Hudi mapping (`HudiUtils`) builds `createTimeV2Type(3)` / `createTimeV2Type(6)` directly. The bug is on master since apache#64304.
### Testing
- `ConnectorColumnConverterTest#timeScaleRidesInThePrecisionSlot` (new): fails before the fix with `expected: <timev2(3)> but was: <timev2(0)>` and passes after; it also pins that a TIMEV2 without a precision stays scale 0. Run locally on master `f5c993f2c94` with apache#70338 applied, which touches neither file.
- `ConnectorColumnConverterTest` (35 tests) and `HudiTypeMappingTest` (17 tests): pass locally on the same tree.
### Release note
None
### Check List (For Author)
- Behavior changed: No
- Does this need documentation: No
- Pre-submission status: Ready
### AI Disclosure
- Tool: Claude Code (Claude Opus 5.5)
- Generated: code, tests, this description
- Human review: not stated
- Prompts: The author asked to port the branch-4.2 Lance FE read path to master as a connector plugin; this fix came out of that port, and the author's rule for such finds is to fix them in their own PR.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
TPC-H: Total hot run time: 28636 ms |
Contributor
TPC-DS: Total hot run time: 154547 ms |
Contributor
ClickBench: Total hot run time: 24.02 s |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Static review of PR #70351 at head 9a484e8 converged after 2 rounds. The TIMEV2 converter now reads the ConnectorType precision slot as the fractional scale, matching the current Hudi mappings for Avro time-millis/time-micros (3/6); omitted precision still produces the legacy scale-0 TIMEV2. All current FE conversion and schema-cache callers preserve the resulting ScalarType scale, and the current Lance mapping is a BE-side path that does not use this converter. No new P0/P1/P2/P3 finding was verified, and there are no existing blocking inline comment IDs applicable to this head. FE locking, exception/RPC-boundary, visible-version, and external-catalog initialization/reset checkpoints are not implicated by these two changed files. No additional user review focus was supplied. The pre-existing Hudi MOR/JNI rejection of Avro TIME logical types and the hypothetical future-plugin case of a TIMEV2 precision above Doris's scale-6 limit were inspected and dismissed as outside the changed reachable behavior; they are retained as coverage notes. Validation was static inspection only under the review contract; no builds or tests were executed.
Review converged after 2 round(s).
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28782 ms |
Contributor
TPC-DS: Total hot run time: 154121 ms |
Contributor
ClickBench: Total hot run time: 23.96 s |
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Static review of PR #70351 at head 9a484e8: no new actionable finding verified. Converged after 1 round with a complete reviewer and a separate risk reviewer, both returning NO_NEW_VALUABLE_FINDINGS, followed by independent main-agent verification and a final sweep of both changed files. No existing blocking inline findings apply; the supplied and refreshed live inline comment sets are empty. No additional user review focus was supplied.
Critical checkpoint conclusions:
- Goal and correctness: TIMEV2 now takes its fractional-second scale from ConnectorType.precision, matching Hudi's Avro time-millis/time-micros producers (3/6). Omitted precision preserves the previous scale-0 singleton. The added test checks 0, 3, 6, omitted precision and time(3) SQL rendering; assertions were inspected, not executed.
- Scope and reuse: the six-line production case uses the existing ScalarType factory and follows adjacent DATETIMEV2 handling; the other changed file adds one focused JUnit 5 test.
- Concurrency and locking: no new threads, shared mutations, locks, heavy work under locks or lock-order changes.
- Lifecycle and initialization: no new resource lifetime or static initialization dependency; explicit scale creates a local type and the fallback reuses the existing singleton.
- Configuration: no settings added or changed; dynamic-update propagation is inapplicable.
- Compatibility: no API symbols, connector SPI or storage layouts changed. The correction uses existing TIMEV2 scale representation and consumers; no new mixed-version incompatibility identified.
- Thrift/protobuf definitions: neither directory changed; field-id and enum-value comparisons are inapplicable.
- Parallel paths: ARRAY/MAP/STRUCT conversion recursively reaches this case; existing DATETIMEV2/TIMESTAMPTZ paths remain consistent. Shared reverse schema conversion serves DDL/write paths, while current Hudi scan handles come from original connector metadata. No newly failing read consumer found.
- Conditions and errors: precision >= 0 distinguishes explicit scale from the unset sentinel, with a comment matching the neighboring convention. Current TIMEV2 producers supply supported scales 3/6; no new swallowed error found.
- Test coverage: scalar scales/default and SQL rendering are covered, with existing Hudi producer assertions independently matching 3/6. No nested TIME-specific, invalid-scale or end-to-end regression case was added; recursive dispatch and actual producer bounds were inspected. Builds and tests were not run under the static-only contract.
- Expected test results: the added type and SQL expectations agree with ScalarType implementation; no golden output files changed.
- Observability: deterministic local type conversion adds no distributed operation requiring new logging or metrics.
- Transactions and persistence: no journal/replay, visible-version, delete-bitmap or failover logic changed.
- Writes and atomicity: no data-write operation or shared-state mutation introduced; transaction atomicity is unaffected.
- FE/BE propagation: no new transmitted variable; existing ScalarType thrift serialization and BE DataTypeTimeV2 construction preserve the supported scale.
- Performance and other issues: constant-time dispatch and a small per-type allocation match neighboring temporal cases; no material redundant work or further verified defect found. FE exception/RPC boundaries and external-catalog initialization/reset behavior are not changed.
Risk dispositions and limits: the unchanged reverse helper's TIMEV2 scale loss has no concrete new failure in current read paths. A separate predicate converter can emit catalog precision/scale, but residual predicates bind existing slots and remaining filters retain original BE conjuncts; they do not feed those types through this changed schema converter. Hudi partition-value types use the schema fallback. The pre-existing Hudi JNI rejection of Avro TIME logical types remains unchanged; this review and the new unit test establish metadata conversion behavior, not end-to-end Hudi time-column execution. Hypothetical future producers with scale above 6 were not treated as an introduced bug. No unresolved patch-specific candidate or unreviewed changed scope remains. Validation was static inspection only; no builds, tests or runtime validation were executed.
Review converged after 1 round(s).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Background
Connector plugins (Hudi, and the coming Lance connector) describe their columns with the SPI's
ConnectorType, and fe-core'sConnectorColumnConverterturns each into a Doris type. Connectors put a temporal column's fractional-second scale in the type's precision field:("DATETIMEV2", 3, 0)isdatetime(3), and Hudi'sHudiTypeMappingsends("TIMEV2", 3, 0)for an Avrotime-milliscolumn and("TIMEV2", 6, 0)fortime-micros.What problem does this PR solve?
Issue Number: None
Related PR: #62183 (introduced the converter without a TIMEV2 case), #64304 (moved Hudi onto it)
PR Type: Bugfix
Problem Summary:
Since Hudi became a connector plugin, its
time-millisandtime-microscolumns show astimeinstead oftime(3)andtime(6), which is what 4.x shows and whatHudiTypeMappingasks for: the converter drops the scale and builds a scale-0 TIMEV2. The Lance connector, ported in a follow-up, maps Arrowtime32(ms)andtime64the same way and would lose its scale too. The converter now reads the TIMEV2 scale from the precision field, as it already does for DATETIMEV2.Details
ConnectorTypeis TIMEV2 with a scale; today that is every Huditime-millis/time-microscolumn on master. Found while porting the Lance connector, by reading the converter, and confirmed by the unit test below.ConnectorColumnConverter.convertScalarTypereads the precision field for CHAR, VARCHAR, VARBINARY, the decimals, DATETIMEV2 and TIMESTAMPTZ, but has no TIMEV2 case, so TIMEV2 falls through toScalarType.createType("TIMEV2"), which returns the scale-0ScalarType.TIMEV2.ConnectorColumnConverterTestchecks decimals, CHAR/VARCHAR lengths and the TIMESTAMPTZ extra info but has no TIMEV2 case, andHudiTypeMappingTestasserts the("TIMEV2", 3, 0)the plugin emits but stops before fe-core converts it. Regression: no suite underregression-test/suitesreads a Hudi time column.toConnectorType, is unchanged: only the read-only Hudi and Lance connectors produce TIMEV2, and the JDBC connector maps TIME to STRING.toConnectorTypeis not affected for reads (see Fix). branch-4.1 and branch-4.2 are not affected: they have noConnectorColumnConverter, and their fe-core Hudi mapping (HudiUtils) buildscreateTimeV2Type(3)/createTimeV2Type(6)directly. The bug is on master since [refactor](catalog) decouple external catalogs from FE core into loadable connector plugins #64304.Testing
ConnectorColumnConverterTest#timeScaleRidesInThePrecisionSlot(new): fails before the fix withexpected: <timev2(3)> but was: <timev2(0)>and passes after; it also pins that a TIMEV2 without a precision stays scale 0. Run locally on masterf5c993f2c94with [fix](catalog) Mask connector catalog secrets and the ALTER CATALOG audit text #70338 applied, which touches neither file.ConnectorColumnConverterTest(35 tests) andHudiTypeMappingTest(17 tests): pass locally on the same tree.Release note
None
Check List (For Author)
AI Disclosure