You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@amoro.apache.org by "deniskuzZ (via GitHub)" <gi...@apache.org> on 2024/05/03 09:22:29 UTC
[I] isMinorNecessary() doesn't account for posDeleteFiles [amoro]
deniskuzZ opened a new issue, #2807:
URL: https://github.com/apache/amoro/issues/2807
### Search before asking
- [X] I have searched in the [issues](https://github.com/apache/amoro/issues?q=is%3Aissue) and found no similar issues.
### What would you like to be improved?
smallFileCount in isMinorNecessary()
### How should we improve?
int smallFileCount = fragmentFileCount + equalityDeleteFileCount + posDeleteFileCount;
### Are you willing to submit PR?
- [X] Yes I am willing to submit a PR!
### Subtasks
_No response_
### Code of Conduct
- [X] I agree to follow this project's [Code of Conduct](https://www.apache.org/foundation/policies/conduct)
--
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.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] isMinorNecessary() doesn't account for posDeleteFiles [amoro]
Posted by "zhongqishang (via GitHub)" <gi...@apache.org>.
zhongqishang commented on issue #2807:
URL: https://github.com/apache/amoro/issues/2807#issuecomment-2097235948
> still think that it would be useful for minor compaction to have a threshold based on pos delete files count (why eq deletes should be any different?).
`minor` mainly does two things,
1. Merge files smaller than 16m
2. Convert eq delete file to position delete file
So when a 128m data file is associated with an eq delete file, the eq delete file will be convert into a position delete through `minor`.
If rewrite position in `minor`, the file will be read and written very frequently, which is also very expensive.
In short, there is rewrite position file, which is in the `major` stage. In order to reduce the cost of reading and writing, a threshold is added (`self-optimizing.major.trigger.duplicate-ratio`).
--
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: [I] isMinorNecessary() doesn't account for posDeleteFiles [amoro]
Posted by "ayushtkn (via GitHub)" <gi...@apache.org>.
ayushtkn commented on issue #2807:
URL: https://github.com/apache/amoro/issues/2807#issuecomment-2108558006
@zhongqishang Have been exploring Amoro to work with Hive-4.0, & what we see when reading iceberg tables via Hive, reading Positional Delete files is quite costly and is one of the major bottleneck for performance while executing queries via Hive.
Do you think it would be worth having it, but behind a config by default turned false, so no existing folks get impacted, but if someone has a usecase like that, they can leverage it by explicitly asking for it by turning the config on.
WDYT?
--
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: [I] isMinorNecessary() doesn't account for posDeleteFiles [amoro]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ closed issue #2807: isMinorNecessary() doesn't account for posDeleteFiles
URL: https://github.com/apache/amoro/issues/2807
--
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: [I] isMinorNecessary() doesn't account for posDeleteFiles [amoro]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on issue #2807:
URL: https://github.com/apache/amoro/issues/2807#issuecomment-2095631174
@zhongqishang, thanks for checking!
Positional deletes are very expensive for read operations, so many customers with read-heavy use cases migrate to COW mode. Without proper caching on the executor side situation is even worse. In Spark Iceberg there is `RewritePositionDeleteFilesSparkAction` specifically created for that.
It's common for transactional workflows to generate multiple small delete files that cause the perf penalties unless compacted.
--
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: [I] isMinorNecessary() doesn't account for posDeleteFiles [amoro]
Posted by "zhongqishang (via GitHub)" <gi...@apache.org>.
zhongqishang commented on issue #2807:
URL: https://github.com/apache/amoro/issues/2807#issuecomment-2109456088
@ayushtkn
Great to hear that you have been exploring Amoro to work with Hive-4.0.
If I understand correctly, after optimizer compaction is executed, you expected is to keep only the data files (no pos delete files), right?
I think this is a reasonable demand scenario and best read performance can also be achieved.
But if the iceberg table with flink stream writer, you need to keep rewriting the data file, which will cause large IO consumption.
BTW, only add the count for posDeleteFiles trigger will not be rewrite data file and eq delete file.
In the current implementation, only `self-optimizing.major.trigger.duplicate-ratio` can generate Major task(rewrite pos delete file).
If possible, could you try setting the `self-optimizing.major.trigger.duplicate-ratio` property of the table to 0?
--
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: [I] isMinorNecessary() doesn't account for posDeleteFiles [amoro]
Posted by "zhongqishang (via GitHub)" <gi...@apache.org>.
zhongqishang commented on issue #2807:
URL: https://github.com/apache/amoro/issues/2807#issuecomment-2095071500
@deniskuzZ Thank you for propose the issue.
The `posDeleteFileCount` not included in `isMinorNecessary` method is expected.
(Assume target size is 128m)
`Minor` mainly include the follow two:
- Merge data files smaller than 16m
- Convert eq delete associated with files larger than 16m
`Major` mainly include the follow two:
- Merge data files in 16～112m
- Merge data files larger than 112m and pos files to generate new data files.
--
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: [I] isMinorNecessary() doesn't account for posDeleteFiles [amoro]
Posted by "zhongqishang (via GitHub)" <gi...@apache.org>.
zhongqishang commented on issue #2807:
URL: https://github.com/apache/amoro/issues/2807#issuecomment-2095934152
@deniskuzZ Yes, I agree the point.
Amoro provides the ability to continuously merge data files and delete files, including position delete file.
In Amoro, rewrite position delete file needs to trigger rules. Rewirte data file and position delete file will be performed under the following two conditions.
- The number of data files smaller than 16m + eq delete file count exceeds `self-optimizing.minor.trigger.file-count`[1]
- The position delete record count associated with a data file exceeding 16m is greater than 10% of the data file record count (self-optimizing.major.trigger.duplicate-ratio)[2]
The triggering rule is to reduce the rewrite amplification problem caused by continuous data writing. Maybe a periodic rewrite all file (`self-optimizing.full.rewrite-all-files`)might help you[3].
[1] : https://github.com/apache/amoro/blob/master/amoro-ams/amoro-ams-server/src/main/java/org/apache/amoro/server/optimizing/plan/CommonPartitionEvaluator.java#L321
[2] : https://github.com/apache/amoro/blob/master/amoro-ams/amoro-ams-server/src/main/java/org/apache/amoro/server/optimizing/plan/CommonPartitionEvaluator.java#L198-L214
[3] : https://amoro.apache.org/docs/0.6.1/configurations/#data-cleaning-configurations
--
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: [I] isMinorNecessary() doesn't account for posDeleteFiles [amoro]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on issue #2807:
URL: https://github.com/apache/amoro/issues/2807#issuecomment-2096526792
thanks @zhongqishang!
still think that it would be useful for minor compaction to have a threshold based on pos delete files count (why eq deletes should be any different?).
`self-optimizing.full.rewrite-all-files` helps, however, it's more expensive.
I guess I'll close the issue then if you see no benefit in 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