You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@amoro.apache.org by "xuzifu666 (via GitHub)" <gi...@apache.org> on 2025/08/19 02:36:52 UTC
[PR] [AMORO-3734][Improvement]: Add operation/schemaId to Snapshot [amoro]
xuzifu666 opened a new pull request, #3735:
URL: https://github.com/apache/amoro/pull/3735
<!--
Thanks for sending a pull request!
Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://amoro.apache.org/how-to-contribute/
2. If the PR is related to an issue in https://github.com/apache/amoro/issues, add '[AMORO-XXXX]' in your PR title, e.g., '[AMORO-XXXX] Your PR title ...'.
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][AMORO-XXXX] Your PR title ...'.
-->
## Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you add a feature, you can talk about its use case.
2. If you fix a bug, you can clarify why it is a bug.
3. Use Fix/Resolve/Close #{ISSUE_NUMBER} to link this PR to its related issue
-->
Close https://github.com/apache/amoro/issues/3734.
## Brief change log
<!--
Clearly describe the changes made in modules, classes, methods, etc.
-->
Snapshot should provide users with relevant information such as operation and schema, which will bring useful improvements to visualization.
-
--
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] [AMORO-3734][Improvement]: Add operation/schemaId to Snapshot [amoro]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #3735:
URL: https://github.com/apache/amoro/pull/3735#issuecomment-3199084918
## [Codecov](https://app.codecov.io/gh/apache/amoro/pull/3735?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 `0%` with `13 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 28.50%. Comparing base ([`f6bad83`](https://app.codecov.io/gh/apache/amoro/commit/f6bad83bab802849730f6e0035179a4c8b8fee88?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`c37fcda`](https://app.codecov.io/gh/apache/amoro/commit/c37fcda75000e36e917f4a06b0eeb5b5ca5a57ce?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 2 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/amoro/pull/3735?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [.../org/apache/amoro/formats/mixed/MixedSnapshot.java](https://app.codecov.io/gh/apache/amoro/pull/3735?src=pr&el=tree&filepath=amoro-format-iceberg%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Famoro%2Fformats%2Fmixed%2FMixedSnapshot.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-YW1vcm8tZm9ybWF0LWljZWJlcmcvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Ftb3JvL2Zvcm1hdHMvbWl4ZWQvTWl4ZWRTbmFwc2hvdC5qYXZh) | 0.00% | [9 Missing :warning: ](https://app.codecov.io/gh/apache/amoro/pull/3735?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [.../apache/amoro/formats/iceberg/IcebergSnapshot.java](https://app.codecov.io/gh/apache/amoro/pull/3735?src=pr&el=tree&filepath=amoro-format-iceberg%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Famoro%2Fformats%2Ficeberg%2FIcebergSnapshot.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-YW1vcm8tZm9ybWF0LWljZWJlcmcvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Ftb3JvL2Zvcm1hdHMvaWNlYmVyZy9JY2ViZXJnU25hcHNob3QuamF2YQ==) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/amoro/pull/3735?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...rg/apache/amoro/formats/paimon/PaimonSnapshot.java](https://app.codecov.io/gh/apache/amoro/pull/3735?src=pr&el=tree&filepath=amoro-format-paimon%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Famoro%2Fformats%2Fpaimon%2FPaimonSnapshot.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-YW1vcm8tZm9ybWF0LXBhaW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvYW1vcm8vZm9ybWF0cy9wYWltb24vUGFpbW9uU25hcHNob3QuamF2YQ==) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/amoro/pull/3735?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #3735 +/- ##
============================================
- Coverage 28.50% 28.50% -0.01%
- Complexity 3755 3756 +1
============================================
Files 617 617
Lines 49988 49966 -22
Branches 6438 6441 +3
============================================
- Hits 14248 14241 -7
+ Misses 34727 34711 -16
- Partials 1013 1014 +1
```
| [Flag](https://app.codecov.io/gh/apache/amoro/pull/3735/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/3735/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `28.50% <0.00%> (-0.01%)` | :arrow_down: |
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/3735?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
Re: [PR] [AMORO-3734][Improvement]: Add operation/schemaId to Snapshot [amoro]
Posted by "zhangwl9 (via GitHub)" <gi...@apache.org>.
zhangwl9 commented on PR #3735:
URL: https://github.com/apache/amoro/pull/3735#issuecomment-3200157023
> > Thanks for the contribution! Could you explain why we need to expose operation and schemaId? In which cases will they be used? It would be helpful if you could also provide some typical usage scenarios so we can better understand the design intent.
>
> Thanks for your attention @xxubai . As I understand it, for example, if a user task is write-only, they need to retrieve the operation from the system table or snapshot file to determine whether their compaction task has taken effect. This needs to be obtained from the operation. Furthermore, during schema evolution, users also need to determine whether their field changes have taken effect based on the schemaId. This should be useful in actual visual monitoring or troubleshooting.
Where will this operation and schemaId be displayed for the user to see, or does the user need to get it via restapi?
--
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] [AMORO-3734][Improvement]: Add operation/schemaId to Snapshot [amoro]
Posted by "xuzifu666 (via GitHub)" <gi...@apache.org>.
xuzifu666 commented on code in PR #3735:
URL: https://github.com/apache/amoro/pull/3735#discussion_r2284550633
##########
amoro-format-iceberg/src/main/java/org/apache/amoro/formats/mixed/MixedSnapshot.java:
##########
@@ -42,10 +42,15 @@ public long watermark() {
@Override
public long commitTime() {
- Long changCommit =
- Optional.ofNullable(changeSnapshot).map(Snapshot::timestampMillis).orElse(-1L);
- Long baseCommit = Optional.ofNullable(baseSnapshot).map(Snapshot::timestampMillis).orElse(-1L);
- return Longs.max(changCommit, baseCommit);
+ return Longs.max(getChangeCommit(), getBaseCommit());
+ }
+
+ public long getChangeCommit() {
+ return Optional.ofNullable(changeSnapshot).map(Snapshot::timestampMillis).orElse(-1L);
+ }
+
+ public long getBaseCommit() {
Review Comment:
ditto
##########
amoro-format-iceberg/src/main/java/org/apache/amoro/formats/mixed/MixedSnapshot.java:
##########
@@ -42,10 +42,15 @@ public long watermark() {
@Override
public long commitTime() {
- Long changCommit =
- Optional.ofNullable(changeSnapshot).map(Snapshot::timestampMillis).orElse(-1L);
- Long baseCommit = Optional.ofNullable(baseSnapshot).map(Snapshot::timestampMillis).orElse(-1L);
- return Longs.max(changCommit, baseCommit);
+ return Longs.max(getChangeCommit(), getBaseCommit());
+ }
+
+ public long getChangeCommit() {
Review Comment:
Thanks for reminder， changed it to private.
--
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] [AMORO-3734][Improvement]: Add operation/schemaId to Snapshot [amoro]
Posted by "zhoujinsong (via GitHub)" <gi...@apache.org>.
zhoujinsong commented on PR #3735:
URL: https://github.com/apache/amoro/pull/3735#issuecomment-3219520662
This change looks good to me. Maybe we can discuss how to display this information in the Web UI later.
--
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] [AMORO-3734][Improvement]: Add operation/schemaId to Snapshot [amoro]
Posted by "xuzifu666 (via GitHub)" <gi...@apache.org>.
xuzifu666 closed pull request #3735: [AMORO-3734][Improvement]: Add operation/schemaId to Snapshot
URL: https://github.com/apache/amoro/pull/3735
--
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] [AMORO-3734][Improvement]: Add operation/schemaId to Snapshot [amoro]
Posted by "xuzifu666 (via GitHub)" <gi...@apache.org>.
xuzifu666 commented on PR #3735:
URL: https://github.com/apache/amoro/pull/3735#issuecomment-3199786767
> Thanks for the contribution! Could you explain why we need to expose operation and schemaId? In which cases will they be used? It would be helpful if you could also provide some typical usage scenarios so we can better understand the design intent.
Thanks for your attention @xxubai . As I understand it, for example, if a user task is write-only, they need to retrieve the operation from the system table or snapshot file to determine whether their compaction task has taken effect. This needs to be obtained from the operation. Furthermore, during schema evolution, users also need to determine whether their field changes have taken effect based on the schemaId. This should be useful in actual visual monitoring or troubleshooting.
--
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] [AMORO-3734][Improvement]: Add operation/schemaId to Snapshot [amoro]
Posted by "xuzifu666 (via GitHub)" <gi...@apache.org>.
xuzifu666 commented on PR #3735:
URL: https://github.com/apache/amoro/pull/3735#issuecomment-3200187005
> > > Thanks for the contribution! Could you explain why we need to expose operation and schemaId? In which cases will they be used? It would be helpful if you could also provide some typical usage scenarios so we can better understand the design intent.
> >
> > Thanks for your attention @xxubai . As I understand it, for example, if a user task is write-only, they need to retrieve the operation from the system table or snapshot file to determine whether their compaction task has taken effect. This needs to be obtained from the operation. Furthermore, during schema evolution, users also need to determine whether their field changes have taken effect based on the schemaId. This should be useful in actual visual monitoring or troubleshooting.
>
> Where will this operation and schemaId be displayed for the user to see, or does the user need to get it via restapi?
Yes，when obtaining snapshot related information through the HTTP interface, it should be possible to obtain these newly added information.
--
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] [AMORO-3734][Improvement]: Add operation/schemaId to Snapshot [amoro]
Posted by "xxubai (via GitHub)" <gi...@apache.org>.
xxubai commented on code in PR #3735:
URL: https://github.com/apache/amoro/pull/3735#discussion_r2284488599
##########
amoro-format-iceberg/src/main/java/org/apache/amoro/formats/mixed/MixedSnapshot.java:
##########
@@ -42,10 +42,15 @@ public long watermark() {
@Override
public long commitTime() {
- Long changCommit =
- Optional.ofNullable(changeSnapshot).map(Snapshot::timestampMillis).orElse(-1L);
- Long baseCommit = Optional.ofNullable(baseSnapshot).map(Snapshot::timestampMillis).orElse(-1L);
- return Longs.max(changCommit, baseCommit);
+ return Longs.max(getChangeCommit(), getBaseCommit());
+ }
+
+ public long getChangeCommit() {
Review Comment:
Why is this function declared as public?
If this method is only meant for internal usage, consider using private or protected to better encapsulate the implementation details.
##########
amoro-format-iceberg/src/main/java/org/apache/amoro/formats/mixed/MixedSnapshot.java:
##########
@@ -42,10 +42,15 @@ public long watermark() {
@Override
public long commitTime() {
- Long changCommit =
- Optional.ofNullable(changeSnapshot).map(Snapshot::timestampMillis).orElse(-1L);
- Long baseCommit = Optional.ofNullable(baseSnapshot).map(Snapshot::timestampMillis).orElse(-1L);
- return Longs.max(changCommit, baseCommit);
+ return Longs.max(getChangeCommit(), getBaseCommit());
+ }
+
+ public long getChangeCommit() {
+ return Optional.ofNullable(changeSnapshot).map(Snapshot::timestampMillis).orElse(-1L);
+ }
+
+ public long getBaseCommit() {
Review Comment:
Same above
--
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] [AMORO-3734][Improvement]: Add operation/schemaId to Snapshot [amoro]
Posted by "xuzifu666 (via GitHub)" <gi...@apache.org>.
xuzifu666 closed pull request #3735: [AMORO-3734][Improvement]: Add operation/schemaId to Snapshot
URL: https://github.com/apache/amoro/pull/3735
--
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] [AMORO-3734][Improvement]: Add operation/schemaId to Snapshot [amoro]
Posted by "xuzifu666 (via GitHub)" <gi...@apache.org>.
xuzifu666 commented on PR #3735:
URL: https://github.com/apache/amoro/pull/3735#issuecomment-3219467654
> This change exposes two interfaces, which indeed provide more detailed snapshot information. However, I’m not entirely sure how these interfaces will be used in the future. Do you have any concrete plans for follow-up implementations? Maybe we can discuss this first.
As I mentioned earlier, the specific production value lies in the fact that during visual monitoring, users can obtain information such as the snapshot operation type and schema ID changes caused by schema evolution, which greatly facilitates monitoring and alerting. Other information can also be added later, such as the number of data entries and the number of changes in the snapshot, which will improve operational capabilities such as table management. @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] [AMORO-3734][Improvement]: Add operation/schemaId to Snapshot [amoro]
Posted by "zhoujinsong (via GitHub)" <gi...@apache.org>.
zhoujinsong merged PR #3735:
URL: https://github.com/apache/amoro/pull/3735
--
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