Repository navigation
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Two P2 regressions remain in historical option restoration: obsolete startup options can make a valid snapshot read fail, and inconsistent fallback priority can make the selected rows depend on FE CPU count. Both inline findings include concrete triggers and fix directions. No existing inline threads or blocking comment IDs were supplied, and there was no additional user focus.
All nine changed files and their binding, planning, metadata, and backend interactions were reviewed statically. Checkpoint conclusions:
| Checkpoint | Conclusion |
|---|---|
| Goal and data correctness | Historical fields and sequence/aggregation options are restored together, and current schema-only renames survive JNI reconstruction. The inline findings identify uncovered startup-state and fallback-priority regressions. |
| Scope, clarity, and reuse | Focused Paimon changes; common restoration and SDK fallback translation are reused. |
| Concurrency and locks | Local maps and table copies; no new threads, locks, shared mutable state, or lock order. Existing statement caches and retained-schema checks remain in use. |
| Lifecycle and static initialization | Bound schemas survive catalog-loader removal; table and secondary system-source payloads were traced. No new owned resource or static-initialization dependency. |
| Configuration | No new setting; existing relation/catalog/physical reader-option precedence is retained through provenance markers. |
| Compatibility | No SPI, function-symbol, storage-format, serialized-class-shape, or protocol-definition change. FE and BE use the same pinned SDK version. |
| Thrift and protobuf IDs | Not applicable; no definitions changed. |
| Parallel paths | Examined metadata/schema/partition/statistics, current and historical selection, OPTIONS/incremental resets, native/JNI, system wrappers, and independent fallback branches. |
| Conditional checks and errors | Changed type/provenance checks correspond to SDK behavior; empty-timestamp fallback preserves SDK error handling. Historical scan-state restoration causes the reported validation failure; restored branch-policy options can disagree with the retained fallback dispatch flag. |
| Test coverage | Inspected all 18 new unit tests and 24 regression query checkpoints across both force_jni_scanner settings, plus related existing tests. Removed historical scan mode and primary-to-fallback policy evolution with overlapping partitions are missing cases. |
| Expected results | Added rows agree with writes and selectors, including sequence retention and aggregation totals 22/27; result ordering is deterministic. |
| Observability | Existing contextual metadata/scan errors are sufficient for this focused change; no new service or recurring operation needs metrics. |
| Transactions, persistence, and failover | No production transaction, EditLog, durability format, or failover protocol change. |
| Writes and atomicity | Production writes are unchanged; new work builds disposable read-side copies. |
| FE/BE value passing | No new wire variable; selected schemas/options remain in the existing serialized table and system-source payloads. |
| Performance | Additional bounded snapshot/schema reads occur at copy boundaries; no new per-row work or unbounded data enumeration was identified. |
| Other concerns and limits | No additional verified issue after independent rechecks. Static review only: no builds/tests were run, runtime reader routing was not proven, and generated-output provenance was not independently verified. |
A statistics authentication-boundary candidate was dismissed after tracing the existing authentication requirements for binding and scans; no separate otherwise-successful query regression was established.
Review converged after 3 round(s).
### What problem does this PR solve? Historical schema options could revive a removed startup mode or obsolete primary-branch policy. The former rejects valid snapshot reads; the latter changes selected rows when manifest parallelism normalization rebuilds a fallback reader. Preserve effective selectors and branch policy, including absent keys, while restoring historical field options. ### Release note Fix historical Paimon reads after removing startup options or changing primary/fallback branch priority. ### Check List (For Author) - Test: Full Paimon connector install and Checkstyle; real-table unit tests for removed scan mode and overlapping branches at capacities 64/128; cluster regression with both scanner settings. - Behavior changed: Yes. Historical schema restoration retains effective read state and branch policy. - Does this need documentation: No.
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Static review of all 9 changed files at 61e929f: one new P2 finding. Historical field-option restoration also rewinds schema-independent reader settings, so a corrected physical batch size can become invalid again on historical reads.
Existing-thread dispositions:
- Removed startup mode, thread 4359737386: the reported latest-full resurrection is fixed; absent startup keys and aliases are preserved.
- Branch priority, thread 4359737377: the original primary-to-fallback example is fixed. The same already-reported priority-reconstruction concern still applies when historical chain-table.enabled=true is restored over a current false primary pair: manifest-cap reconstruction can infer a different priority at capacities 64 and 128. This is covered by that existing thread and is not reposted.
- No existing P0/P1 finding was present to carry forward. No additional user-provided focus was supplied.
Critical checkpoint conclusions:
- Goal: renamed sequence/aggregation field options are restored alongside the selected columns, and statement-bound current schemas survive JNI loader removal; the runtime-reader regression prevents full correctness.
- Scope and reuse: changes use shared copy and system-source boundaries; restoring the entire historical option map is broader than the field-dependent repair requires.
- Concurrency and locks: no new thread, lock, lock order, or shared mutable ownership is introduced; maps are copied and existing statement pins remain in place.
- Lifecycle and initialization: copied tables retain their FileIO/catalog context; catalog loaders are removed only for backend transport, then the exact schema is restored. No new static-initialization or close-ownership issue found.
- Configuration: no new setting; catalog provenance and relation overrides are retained, but corrected physical reader tuning is lost in the reported case.
- Compatibility: no public SPI, wire carrier, or persistent format change identified.
- Thrift/protobuf: no gensrc changes.
- Parallel paths: inspected binding, partition/statistics metadata, snapshot/timestamp/tag/OPTIONS, incremental resets, fallback/chain handling, system and non-file sources, and native/JNI execution. No additional distinct omission verified.
- Conditions and errors: file/system/statement/chain guards have concrete roles; SDK selector-error fallback remains SDK-compatible and later scan validation owns errors. The new reader-setting failure is reported inline.
- Tests: inspected 21 real-table unit tests and 26 ordered regression query checkpoints for both force_jni_scanner values. Coverage includes renamed fields, warm/current schemas, historical selectors, independent branches, explicit catalog precedence, serialization, and removed startup mode; it omits repaired physical reader settings and the chain-history priority variant.
- Expected results: sequence winners and aggregation sums match fixture writes; no incorrect output block found. Static inspection cannot verify that output files were generated by a runner.
- Observability: existing option-validation errors and metadata exception logging cover these transformations; no new metrics requirement identified.
- Persistence/failover: no EditLog or transaction-persistence change.
- Data writes/atomicity: production changes affect reads and copies, not commit/write protocols.
- FE-BE propagation: no new transport variable; both scanners use the selected schema, and JNI receives the loader-free equivalent.
- Performance: additional selector/schema metadata reads and option-map copies are per table/branch; no demonstrated data-size-dependent or lock-held regression.
- Further recheck: reviewed null resets, aliases, catalog override equality, physical-option removal, REST rebinding order, fallback reconstruction, system-source transport, and statistics authentication. No other distinct substantiated finding remains.
Limits: static inspection only; no build, test, cluster, or Kerberos execution. The read-optimized system-table test may use an empty SDK baseline and therefore proves schema/copy parity more strongly than nonempty row behavior. Review convergence describes completion of the bounded review, not absence of bugs.
Review converged after 3 round(s).
| .add(CoreOptions.BRANCH.key()) | ||
| .add(CoreOptions.SCAN_PRIMARY_BRANCH.key()) | ||
| .add(CoreOptions.SCAN_FALLBACK_BRANCH.key()) | ||
| .build(); |
Contributor
There was a problem hiding this comment.
[P2] Preserve current reader tuning when restoring historical fields
This new preserve set omits schema-independent reader options. Consider a Paimon table that commits snapshot 1 with read.batch-size=131072 (valid in the SDK), then changes that physical option to 1024. With no catalog or relation batch override, FOR VERSION AS OF 1 calls restoreBoundSchema, which starts with the old 131072; current 1024 equals the current persisted value, so the delta loop does not copy it, and this set does not preserve it either. Final PaimonReaderOptions.validateEffectiveTable therefore rejects the historical query against Doris's maximum of 65536, while the previous SDK copy kept 1024 and succeeded. This is a reader-tuning omission in the new set, separate from the startup-selector conflict in existing thread 4359737386. Retain effective current reader tuning, including removed keys, while restoring historical field-dependent options, and add a regression for a repaired physical setting.
Contributor
TPC-H: Total hot run time: 28184 ms |
Contributor
TPC-DS: Total hot run time: 149565 ms |
Contributor
ClickBench: Total hot run time: 24.16 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 28247 ms |
Contributor
TPC-DS: Total hot run time: 150377 ms |
Contributor
ClickBench: Total hot run time: 24.19 s |
Contributor
FE UT Coverage ReportIncrement line coverage `` 🎉 |
Contributor
FE Regression Coverage ReportIncrement line coverage |
2 similar comments
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
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.
What problem does this PR solve?
After renaming a Paimon sequence or aggregation field, current and historical reads can fail with a missing-field error. Paimon's table copy selects the historical columns but overlays current table options, so
sequence.fieldandfields.<column>.aggregate-functioncan reference names that do not exist in the selected schema.JNI transport also rebuilds tables without their catalog loader. The SDK factory can reapply a snapshot selector during that rebuild, reverting a schema-only rename before the bound schema is restored. Build with selectors temporarily removed and then restore the complete bound schema.
Restore the selected schema and its physical options together before applying query options. Preserve the resolved table's read-state keys and branch policy, including absent keys, so historical options cannot resurrect a removed scan mode or change fallback priority during parallelism normalization. Preserve explicit catalog overrides and the SDK's fallback-branch snapshot translation. Apply the same logic to scan planning, metadata, statistics, and the source used to rebuild system-table wrappers, so binding and JNI serialization agree.
Add real-table tests for snapshot, timestamp, tag, and OPTIONS reads after renames, current reads after schema-only renames, catalog-loader removal and serialization, catalog override precedence, fallback branches, empty-table timestamps, and read-optimized system tables. Add an end-to-end regression covering sequence semantics and repeated aggregation-field renames with
force_jni_scanner=falseandtrue.Release note
Fix Paimon current and historical reads after renaming sequence or aggregation fields.
Check List (For Author)