Repository navigation
[improvement](fe) Reuse Hudi table schema across native scan ranges - #70557
zhangstar333 wants to merge 1 commit into
Conversation
### What problem does this PR solve? Issue Number: None Related PR: apache#64304 Problem Summary: When Hudi schema evolution is disabled, the branch-4.2 planner constructs a TableSchemaResolver and resolves the same table schema for every native split. This repeats timeline and schema reads as the number of splits grows. Adapt the master optimization to the legacy planner by lazily resolving and reusing the InternalSchema within each HudiScanNode. Synchronize initialization for concurrent range creation. Keep JNI ranges lazy and retain per-file commit schema resolution when evolution is enabled. ### Release note Reduce FE scan range planning overhead for Hudi native scans without schema evolution by resolving the table schema once per scan node. ### Check List (For Author) - Test: Unit Test - HudiScanNodeTest: 18 tests passed via run-fe-ut.sh - Scoped Maven Checkstyle validation and git diff --check passed - End-to-end Hudi performance comparison was not run - Behavior changed: No (query results and schema evolution routing are unchanged) - Does this need documentation: No
|
/review |
There was a problem hiding this comment.
No new actionable findings were identified at head 5a24389d52dfee93bdb891c4dcda86d2e2465fb3. Static review covered both changed files, the range producers and schema consumers, catalog-generation guards, and the added tests. There are no existing inline findings to carry forward and no additional user-provided focus points.
The inspected flow is:
PhysicalHudiScan -> fresh HudiScanNode -> init(queryInstant, hudiClient)
eager finalization / concurrent batch producers -> splitToScanRange -> setScanParams
JNI -> JNI descriptor (no native schema resolution)
native + evolution enabled -> schema for each file commit
native + evolution disabled -> synchronized lazy schema for this scan
-> history schema + range schema ID -> BE native reader
Critical-checkpoint conclusions:
- Goal and evidence: the resolver construction, table-schema read, and conversion now occur once per successfully initialized native scan when schema evolution is disabled. Added tests assert reuse across files, independent scan nodes, JNI laziness, and per-commit evolution behavior.
- Scope: the production change is confined to one node-local field and one getter; the resolver expression and exception behavior are preserved.
- Concurrency and locks: eager range conversion is sequential; batch producers can convert concurrently. All cached-field accesses use the synchronized getter, safely publishing the completed schema. Its consumers read it into fresh Thrift objects. No new lock-order cycle was found; repeated remote reads become one initialization under the node monitor.
- Lifecycle: translation creates a fresh node for each physical scan. The cache lives with that node, introduces no resource handle or reference cycle, and leaves initialization failures uncached for retry. Catalog-generation checks remain before descriptor/history publication. No static initialization changes.
- Configuration: none added or changed.
- Compatibility: no function, storage-format, or FE/BE wire-contract change; schema IDs and history serialization follow the existing paths.
- Thrift/protobuf definitions: none changed; branch ID/enum comparison is inapplicable.
- Parallel paths: native Parquet/ORC and log-free MOR fallback reach the same native branch. Genuine JNI ranges remain lazy; evolution-enabled ranges retain file-commit resolution. Incremental and snapshot split selection are unchanged.
- Conditions: the existing native/JNI and evolution guards still delimit caching, and the new field comment explains its scope.
- Tests: the two added JUnit 5 tests and helpers were inspected for construction mocking, expected resolver counts, range schema IDs, and history entries. Direct concurrent-initialization, failure-retry, and ORC cases are not added. No builds or tests were run, as required by this review contract; runtime correctness and performance are not claimed as measured.
- Test results: no golden or generated result files changed; inspected assertions match the intended behavior.
- Observability: existing contextual exception propagation is retained. This local reuse introduces no new distributed operation requiring additional metrics or logs.
- Persistence and failover: no transaction, EditLog, or persisted metadata path changed.
- Writes and atomicity: this is read planning; no data-write or transaction-atomicity change. No new resource-leak or deadlock path was identified.
- FE/BE variables: no new transmitted variable; existing history-schema and range-ID fields are reused.
- Performance: expensive resolver/read/conversion work is removed from repeated ranges. Each native range still performs a short synchronized lookup, and each scan retains one schema. No benchmark was executed.
- Other invariants: no changed committed-data visibility, version boundary, MoW/delete-bitmap, BE allocation, or nullable-column handling. Schema-resolution exceptions still fail planning, rather than silently publishing a partial result.
The complete and risk-focused reviewers reported NO_NEW_VALUABLE_FINDINGS after their missed-case rechecks. All review candidates and risk focuses are disposed; no code-review coverage questions remain. Review converged in one round. Validation was static only, with the test limitations above.
Review converged after 1 round(s).
What problem does this PR solve?
Problem Summary:
When Hudi schema evolution is disabled, the planner constructs a TableSchemaResolver and resolves the same table schema for every native split.
This repeats timeline and schema reads as the number of splits grows.
Adapt the master optimization to the legacy planner by lazily resolving and reusing the InternalSchema within each HudiScanNode. Synchronize initialization for concurrent range creation.
Keep JNI ranges lazy and retain per-file commit schema resolution when evolution is enabled.