You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@amoro.apache.org by "wardlican (via GitHub)" <gi...@apache.org> on 2025/10/31 12:00:16 UTC
[PR] [Bug]: Amoro optimization can result in the input files and the merge… [amoro]
wardlican opened a new pull request, #3856:
URL: https://github.com/apache/amoro/pull/3856
## Why are the changes needed?
Amoro optimization can result in the input files and the merged output files having the same number of files, and this can cause the merge to fail and keep triggering the merge task.
Close #3855
## Brief change log
1. For undersizedSegmentFiles that result in only one file after bin-packing:
2. If rewriting pos delete is not required, simply add them to rewriteDataFiles.
3. During the second bin-packing, they can be merged with fragment files or other files.
-
## How was this patch tested?
- [ ] Add some test cases that check the changes thoroughly including negative and positive cases if possible
- [ ] Add screenshots for manual tests if appropriate
- [ ] Run test locally before making a pull request
## Documentation
- Does this pull request introduce a new feature? (yes / no)
- If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Bug]: Amoro optimization can result in the input files and the merge… [amoro]
Posted by "wardlican (via GitHub)" <gi...@apache.org>.
wardlican commented on PR #3856:
URL: https://github.com/apache/amoro/pull/3856#issuecomment-3472782255
Please help review whether this repair plan is feasible.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Bug]: Amoro optimization can result in the input files and the merge… [amoro]
Posted by "wardlican (via GitHub)" <gi...@apache.org>.
wardlican commented on PR #3856:
URL: https://github.com/apache/amoro/pull/3856#issuecomment-3713337642
> > The current implementation is based on the implementation of SizeBasedFileRewritePlanner; please help review it. @xxubai
>
> Can you test this implementation in your enviroment? It is really helpful for validating. Otherwise we should add a unit test to cover this case
Unit tests have been added for the targetSize() method, covering the following scenarios:
- **testTargetSizeWithSmallInputFiles** - Merging multiple small files
**Scenario**: 5 files of 10MB each (Total 50MB < 128MB).
**Expected**: Returns inputSize (50MB), merging them into a single file.
- **testTargetSizeWithSingleSmallFile** - Single small file
Scenario: 1 file of 10MB.
Expected: Returns targetSize (128MB).
- **testTargetSizeWithExactTargetSize** - Input size equals targetSize
**Scenario**: Input size is exactly 128MB.
**Expected**: Returns ≥targetSize.
- **testTargetSizeWithLargeRemainder** - Large remainder case
**Scenario**: 225MB input, remainder is 97MB (which is > 96MB minFileSize).
**Expected**: Rounds up to calculate an appropriate split size.
- **testTargetSizeWithSmallRemainderDistributed** - Distributable small remainder
**Scenario**: 256MB input (exactly 2x targetSize, no remainder).
**Expected**: Rounds down and distributes the size evenly.
- **testTargetSizeWithMultipleSmallFiles** - Multiple small files exceeding targetSize
**Scenario**: 10 files of 20MB each (Total 200MB > 128MB).
**Expected**: Calculates an appropriate split size.
- **testTargetSizeWithVeryLargeInput** - Very large input
**Scenario**: 500MB input.
**Expected**: Capped at maxFileSize (192MB, calculated as targetSize×1.5).
- **testTargetSizeWithCustomMinTargetSizeRatio** - Custom ratio
**Scenario**: Custom min-target-size-ratio set to 0.8.
**Expected**: Performs calculations based on the custom ratio.
- **testTargetSizeBoundaryConditions** - Boundary conditions
**Scenario**: Input size is near boundary values.
**Expected**: Correctly handles edge cases and boundary conditions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Bug]: Amoro optimization can result in the input files and the merge… [amoro]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #3856:
URL: https://github.com/apache/amoro/pull/3856#issuecomment-3679759369
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@amoro.apache.org list. Thank you for your contributions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Bug]: Amoro optimization can result in the input files and the merge… [amoro]
Posted by "wardlican (via GitHub)" <gi...@apache.org>.
wardlican commented on PR #3856:
URL: https://github.com/apache/amoro/pull/3856#issuecomment-3684771421
Referencing Spark/Iceberg's SizeBasedFileRewritePlanner, the number of output files is determined intelligently:
| Condition | Decision |
| ----------- | ----------- |
| Remainder > minFileSize (default 0.75 * targetSize) | Retain the remainder file (round up) |
| avgFileSizeWithoutRemainder < 1.1 * targetSize | Distribute the remainder to other files (rounding down) |
| other | Retain the remainder file (round up) |
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Bug]: Amoro optimization can result in the input files and the merge… [amoro]
Posted by "xxubai (via GitHub)" <gi...@apache.org>.
xxubai commented on PR #3856:
URL: https://github.com/apache/amoro/pull/3856#issuecomment-3709238575
> The current implementation is based on the implementation of SizeBasedFileRewritePlanner; please help review it. @xxubai
Can you test this implementation in your enviroment? It is really helpful for validating. Otherwise we should add a unit test to cover this case
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Bug]: Amoro optimization can result in the input files and the merge… [amoro]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #3856: [Bug]: Amoro optimization can result in the input files and the merge…
URL: https://github.com/apache/amoro/pull/3856
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Bug]: Amoro optimization can result in the input files and the merge… [amoro]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 commented on code in PR #3856:
URL: https://github.com/apache/amoro/pull/3856#discussion_r2510156492
##########
amoro-format-iceberg/src/main/java/org/apache/amoro/optimizing/IcebergRewriteExecutor.java:
##########
@@ -101,12 +101,25 @@ protected TaskWriter<Record> dataWriter() {
@Override
protected long targetSize() {
long targetSize = super.targetSize();
- long inputSize =
- Arrays.stream(input.rewrittenDataFiles()).mapToLong(DataFile::fileSizeInBytes).sum();
- // When the input files’ total size is below targetSize, remove the output file size limit to
+ DataFile[] rewrittenDataFiles = input.rewrittenDataFiles();
+ if (rewrittenDataFiles == null || rewrittenDataFiles.length == 0) {
+ return targetSize;
+ }
+ long inputSize = Arrays.stream(rewrittenDataFiles).mapToLong(DataFile::fileSizeInBytes).sum();
+ // When the input files' total size is below targetSize, remove the output file size limit to
// avoid outputting multiple files.
// For more details, please refer to: https://github.com/apache/amoro/issues/3645
- return inputSize < targetSize ? Long.MAX_VALUE : targetSize;
+ if (inputSize < targetSize) {
+ return Long.MAX_VALUE;
+ }
+ // Even if total size >= targetSize, if average file size is small (less than targetSize),
Review Comment:
Will this output a single file with too big file size?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Bug]: Amoro optimization can result in the input files and the merge… [amoro]
Posted by "wardlican (via GitHub)" <gi...@apache.org>.
wardlican commented on PR #3856:
URL: https://github.com/apache/amoro/pull/3856#issuecomment-3681617427
> @wardlican I have the same concern: could simply setting it to the maximum value result in a single file being excessively large? Could we perhaps refer to Spark's estimation logic？ Refer to https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/actions/SizeBasedFileRewritePlanner.java#L199-L206
Okay, I'll look into whether we can use the design from Iceberg for this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Bug]: Amoro optimization can result in the input files and the merge… [amoro]
Posted by "wardlican (via GitHub)" <gi...@apache.org>.
wardlican commented on PR #3856:
URL: https://github.com/apache/amoro/pull/3856#issuecomment-3696324246
> o whether we can use the design
> @wardlican I have the same concern: could simply setting it to the maximum value result in a single file being excessively large? Could we perhaps refer to Spark's estimation logic？ Refer to https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/actions/SizeBasedFileRewritePlanner.java#L199-L206
The current implementation is based on the implementation of SizeBasedFileRewritePlanner; please help review it. @xxubai
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Bug]: Amoro optimization can result in the input files and the merge… [amoro]
Posted by "wardlican (via GitHub)" <gi...@apache.org>.
wardlican commented on PR #3856:
URL: https://github.com/apache/amoro/pull/3856#issuecomment-3712768915
> > The current implementation is based on the implementation of SizeBasedFileRewritePlanner; please help review it. @xxubai
>
> Can you test this implementation in your enviroment? It is really helpful for validating. Otherwise we should add a unit test to cover this case
Okay, I will verify it in my local environment and add some unit tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Bug]: Amoro optimization can result in the input files and the merge… [amoro]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #3856:
URL: https://github.com/apache/amoro/pull/3856#issuecomment-3857066789
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@amoro.apache.org list. Thank you for your contributions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Bug]: Amoro optimization can result in the input files and the merge… [amoro]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #3856:
URL: https://github.com/apache/amoro/pull/3856#issuecomment-3894073226
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Bug]: Amoro optimization can result in the input files and the merge… [amoro]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #3856:
URL: https://github.com/apache/amoro/pull/3856#issuecomment-3684774527
## [Codecov](https://app.codecov.io/gh/apache/amoro/pull/3856?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `10.71429%` with `25 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 22.84%. Comparing base ([`12e7946`](https://app.codecov.io/gh/apache/amoro/commit/12e7946d6aed7d5df7619e09b48ea5cd475743d5?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`c8a56c5`](https://app.codecov.io/gh/apache/amoro/commit/c8a56c5c123f9051c5e82bebc47f5645cb23dad0?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 34 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/amoro/pull/3856?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...pache/amoro/optimizing/IcebergRewriteExecutor.java](https://app.codecov.io/gh/apache/amoro/pull/3856?src=pr&el=tree&filepath=amoro-format-iceberg%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Famoro%2Foptimizing%2FIcebergRewriteExecutor.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-YW1vcm8tZm9ybWF0LWljZWJlcmcvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Ftb3JvL29wdGltaXppbmcvSWNlYmVyZ1Jld3JpdGVFeGVjdXRvci5qYXZh) | 10.71% | [23 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/amoro/pull/3856?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (12e7946) and HEAD (c8a56c5). Click for more details.
>
> <details><summary>HEAD has 4 uploads less than BASE</summary>
>
>| Flag | BASE (12e7946) | HEAD (c8a56c5) |
>|------|------|------|
>|core|4|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #3856 +/- ##
============================================
- Coverage 29.03% 22.84% -6.20%
+ Complexity 3862 2531 -1331
============================================
Files 634 449 -185
Lines 50808 41074 -9734
Branches 6545 5788 -757
============================================
- Hits 14753 9382 -5371
+ Misses 34997 30882 -4115
+ Partials 1058 810 -248
```
| [Flag](https://app.codecov.io/gh/apache/amoro/pull/3856/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [core](https://app.codecov.io/gh/apache/amoro/pull/3856/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [trino](https://app.codecov.io/gh/apache/amoro/pull/3856/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `22.84% <10.71%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/amoro/pull/3856?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org