You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@paimon.apache.org by "Aitozi (via GitHub)" <gi...@apache.org> on 2024/08/05 02:00:40 UTC
[PR] spec: Add missing property in Snapshot [paimon-rust]
Aitozi opened a new pull request, #41:
URL: https://github.com/apache/paimon-rust/pull/41
Some fields in Snapshot are lost, add them back
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] spec: Add missing property in Snapshot [paimon-rust]
Posted by "Aitozi (via GitHub)" <gi...@apache.org>.
Aitozi commented on PR #41:
URL: https://github.com/apache/paimon-rust/pull/41#issuecomment-2270272952
@QuakeWang Sure, thanks.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] spec: Add missing property in Snapshot [paimon-rust]
Posted by "Aitozi (via GitHub)" <gi...@apache.org>.
Aitozi commented on code in PR #41:
URL: https://github.com/apache/paimon-rust/pull/41#discussion_r1703461745
##########
crates/paimon/src/spec/snapshot.rs:
##########
@@ -134,4 +144,38 @@ impl Snapshot {
pub fn statistics(&self) -> Option<&str> {
self.statistics.as_deref()
}
+
+ /// Get the commit time of this snapshot.
+ #[inline]
+ pub fn time_millis(&self) -> u64 {
+ self.time_millis
+ }
+
+ /// Get the commit identifier of this snapshot.
+ #[inline]
+ pub fn commit_identifier(&self) -> i64 {
+ self.commit_identifier
+ }
+
+ /// Get the commit kind of this snapshot.
+ #[inline]
+ pub fn commit_kind(&self) -> CommitKind {
+ self.commit_kind
+ }
+}
+
+/// Type of changes in this snapshot.
+///
+/// Impl Reference: <https://github.com/apache/paimon/blob/release-0.8.2/paimon-core/src/main/java/org/apache/paimon/Snapshot.java#L506>.
+#[derive(Debug, Clone, Copy, PartialEq, Eq, Deserialize, Serialize)]
+#[serde(rename_all = "camelCase")]
+pub enum CommitKind {
Review Comment:
Hi @QuakeWang I think that just means `log_offsets` ?
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] spec: Add missing property in Snapshot [paimon-rust]
Posted by "Aitozi (via GitHub)" <gi...@apache.org>.
Aitozi commented on code in PR #41:
URL: https://github.com/apache/paimon-rust/pull/41#discussion_r1703460995
##########
crates/paimon/src/spec/snapshot.rs:
##########
@@ -39,6 +39,16 @@ pub struct Snapshot {
#[builder(default = None)]
index_manifest: Option<String>,
commit_user: String,
+ // Mainly for snapshot deduplication.
+ //
+ // If multiple snapshots have the same commitIdentifier, reading from any of these snapshots
Review Comment:
I have a question out of this PR, Currently, the overwrite (expire partition) share the same commit identifier with the current snapshots's commit identifier. So I think this comments may not right ? CC @JingsongLi
##########
crates/paimon/src/spec/snapshot.rs:
##########
@@ -39,6 +39,16 @@ pub struct Snapshot {
#[builder(default = None)]
index_manifest: Option<String>,
commit_user: String,
+ // Mainly for snapshot deduplication.
+ //
+ // If multiple snapshots have the same commitIdentifier, reading from any of these snapshots
Review Comment:
I have a question out of this PR. Currently, the overwrite (expire partition) share the same commit identifier with the current snapshots's commit identifier. So I think this comments may not right ? CC @JingsongLi
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] spec: Add missing property in Snapshot [paimon-rust]
Posted by "QuakeWang (via GitHub)" <gi...@apache.org>.
QuakeWang commented on code in PR #41:
URL: https://github.com/apache/paimon-rust/pull/41#discussion_r1703460558
##########
crates/paimon/src/spec/snapshot.rs:
##########
@@ -134,4 +144,38 @@ impl Snapshot {
pub fn statistics(&self) -> Option<&str> {
self.statistics.as_deref()
}
+
+ /// Get the commit time of this snapshot.
+ #[inline]
+ pub fn time_millis(&self) -> u64 {
+ self.time_millis
+ }
+
+ /// Get the commit identifier of this snapshot.
+ #[inline]
+ pub fn commit_identifier(&self) -> i64 {
+ self.commit_identifier
+ }
+
+ /// Get the commit kind of this snapshot.
+ #[inline]
+ pub fn commit_kind(&self) -> CommitKind {
+ self.commit_kind
+ }
+}
+
+/// Type of changes in this snapshot.
+///
+/// Impl Reference: <https://github.com/apache/paimon/blob/release-0.8.2/paimon-core/src/main/java/org/apache/paimon/Snapshot.java#L506>.
+#[derive(Debug, Clone, Copy, PartialEq, Eq, Deserialize, Serialize)]
+#[serde(rename_all = "camelCase")]
+pub enum CommitKind {
Review Comment:
@Aitozi We do not need this enum, you can refer #11.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] spec: Add missing property in Snapshot [paimon-rust]
Posted by "QuakeWang (via GitHub)" <gi...@apache.org>.
QuakeWang commented on PR #41:
URL: https://github.com/apache/paimon-rust/pull/41#issuecomment-2270198073
It's my bad, when add the snapshot lost these field. Maybe I can add these in #39 with some tests. What do you think of 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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] spec: Add missing property in Snapshot [paimon-rust]
Posted by "Aitozi (via GitHub)" <gi...@apache.org>.
Aitozi closed pull request #41: spec: Add missing property in Snapshot
URL: https://github.com/apache/paimon-rust/pull/41
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org