[fix](paimon) infer manifest format from split file format in cpp reader#60795
Open
xylaaaaa wants to merge 5 commits intoapache:masterfrom
Open
[fix](paimon) infer manifest format from split file format in cpp reader#60795xylaaaaa wants to merge 5 commits intoapache:masterfrom
xylaaaaa wants to merge 5 commits intoapache:masterfrom
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
There was a problem hiding this comment.
Pull request overview
Adjusts Doris BE’s Paimon C++ reader option construction to avoid incorrect/default manifest format selection when FE scan ranges omit table options, by inferring formats from split metadata.
Changes:
- Infer
paimon::Options::FILE_FORMATfrom split-levelpaimon_params.file_formatwhen the option is missing/empty. - Infer
paimon::Options::MANIFEST_FORMATfrom split-levelpaimon_params.file_formatwhen the option is missing/empty.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
314
to
328
| // FE currently may not pass paimon table options in scan ranges. | ||
| // Avoid paimon-cpp defaulting manifest.format to avro when split file format is known. | ||
| if (_range.__isset.table_format_params && _range.table_format_params.__isset.paimon_params && | ||
| _range.table_format_params.paimon_params.__isset.file_format && | ||
| !_range.table_format_params.paimon_params.file_format.empty()) { | ||
| const auto& split_file_format = _range.table_format_params.paimon_params.file_format; | ||
| auto file_format_it = options.find(paimon::Options::FILE_FORMAT); | ||
| if (file_format_it == options.end() || file_format_it->second.empty()) { | ||
| options[paimon::Options::FILE_FORMAT] = split_file_format; | ||
| } | ||
| auto manifest_format_it = options.find(paimon::Options::MANIFEST_FORMAT); | ||
| if (manifest_format_it == options.end() || manifest_format_it->second.empty()) { | ||
| options[paimon::Options::MANIFEST_FORMAT] = split_file_format; | ||
| } | ||
| } |
There was a problem hiding this comment.
New option inference logic isn’t covered by existing PaimonCppReader unit tests. Consider adding a test that asserts when split-level file_format is set and options lacks/has empty paimon::Options::FILE_FORMAT / MANIFEST_FORMAT, _build_options() (or an observable init path) populates them, and does not override non-empty values.
Comment on lines
314
to
328
| // FE currently may not pass paimon table options in scan ranges. | ||
| // Avoid paimon-cpp defaulting manifest.format to avro when split file format is known. | ||
| if (_range.__isset.table_format_params && _range.table_format_params.__isset.paimon_params && | ||
| _range.table_format_params.paimon_params.__isset.file_format && | ||
| !_range.table_format_params.paimon_params.file_format.empty()) { | ||
| const auto& split_file_format = _range.table_format_params.paimon_params.file_format; | ||
| auto file_format_it = options.find(paimon::Options::FILE_FORMAT); | ||
| if (file_format_it == options.end() || file_format_it->second.empty()) { | ||
| options[paimon::Options::FILE_FORMAT] = split_file_format; | ||
| } | ||
| auto manifest_format_it = options.find(paimon::Options::MANIFEST_FORMAT); | ||
| if (manifest_format_it == options.end() || manifest_format_it->second.empty()) { | ||
| options[paimon::Options::MANIFEST_FORMAT] = split_file_format; | ||
| } | ||
| } |
There was a problem hiding this comment.
This behavior is slightly broader than the PR description (“only when table options are missing/empty”): it will also set FILE_FORMAT/MANIFEST_FORMAT when other table options are present but just these keys are absent/empty. If that’s intended, the PR description should be updated; if not, consider tightening the condition to only apply when the table-level paimon options map is missing/empty.
Contributor
Author
|
run buildall |
TPC-H: Total hot run time: 28640 ms |
TPC-DS: Total hot run time: 183793 ms |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
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.
Problem
Followup #60676
When FE does not pass full table options in scan ranges, paimon-cpp may default manifest.format to avro.
For non-avro environments, this can fail in PaimonCppReader initialization with:
Could not find a FileFormatFactory implementation class for format avro.
Solution
In PaimonCppReader::_build_options, if split-level file_format exists and table options are missing/empty:
This keeps paimon-cpp format resolution consistent with the actual split format and avoids unintended avro fallback.
Verification