Conversation
| /// | ||
| /// Impl Reference: <https://github.com/apache/paimon/blob/db8bcd7fdd9c2705435d2ab1d2341c52d1f67ee5/paimon-core/src/main/java/org/apache/paimon/Snapshot.java#L68>. | ||
| #[derive(Debug, Clone, PartialEq, Eq, Deserialize, Serialize, TypedBuilder)] | ||
| pub struct Snapshot { |
There was a problem hiding this comment.
Maybe we need serde(rename_all("camelCase"))?
crates/paimon/src/spec/snapshot.rs
Outdated
| delta_manifest_list: String, | ||
| /// a manifest list recording all changelog produced in this snapshot | ||
| #[builder(default = None)] | ||
| change_log_manifest_list: Option<String>, |
There was a problem hiding this comment.
Should be changelog_manifest_list
crates/paimon/src/spec/snapshot.rs
Outdated
| delta_record_count: Option<i64>, | ||
| /// record count of all changelog produced in this snapshot | ||
| #[builder(default = None)] | ||
| change_log_record_count: Option<i64>, |
crates/paimon/src/spec/snapshot.rs
Outdated
| } | ||
|
|
||
| #[inline] | ||
| pub fn change_log_manifest_list(&self) -> Option<&String> { |
There was a problem hiding this comment.
Better to return Option<&str> instead.
crates/paimon/src/spec/snapshot.rs
Outdated
| } | ||
|
|
||
| #[inline] | ||
| pub fn commit_kind(&self) -> &CommitKind { |
There was a problem hiding this comment.
CommitKind could be Copy, we can return CommitKind directly.
|
|
||
| impl Snapshot { | ||
| #[inline] | ||
| pub fn version(&self) -> i32 { |
There was a problem hiding this comment.
Better to provide APIs for our public API.
There was a problem hiding this comment.
Better to provide APIs for our public API.
Do you mean remove #[inline]?
There was a problem hiding this comment.
Oh sorry, I mean add API docs.
I have updated it, PTAL 👀
crates/paimon/src/spec/snapshot.rs
Outdated
| // specific language governing permissions and limitations | ||
| // under the License. | ||
|
|
||
| /*! |
crates/paimon/src/spec/snapshot.rs
Outdated
| commit_identifier: i64, | ||
| commit_kind: CommitKind, | ||
| time_millis: i64, | ||
| log_offsets: HashMap<i32, i64>, |
There was a problem hiding this comment.
I think we don't need to add this field.
We don't write it, and we don't have requirement to read it.
There was a problem hiding this comment.
I think we don't need to add this field. We don't write it, and we don't have requirement to read it.
OK, I will remove it later.
|
cc @JingsongLi and @SteNicholas for another look. |
3 participants
fix: #6
This PR will add
Snapshot. Some function includeManifestListis not implemented yet.