feat: suport read/write Manifest #79

ZENOTME · 2023-10-13T05:58:33Z

This PR prepare to support read/write Manifest.

related issue: #36

ZENOTME · 2023-10-13T06:04:37Z

For now, it still not been completed.

It only complete the basic design and I want to make sure whether the design well first.

If it looks well, I will complete it and add the test later.

cc @JanKaul @Fokko @Xuanwo @liurenjie1024

liurenjie1024

@ZENOTME Thanks for this! I would suggest to move the part for ser/de to another pr and resolve them first, otherwise it would be too large to review.

I'm ok with the overall design.

crates/iceberg/src/spec/partition.rs

crates/iceberg/src/spec/values.rs

crates/iceberg/src/spec/manifest.rs

crates/iceberg/src/spec/values.rs

ZENOTME · 2023-10-18T14:09:51Z

I will split the RawLiteral out into another PR.

ZENOTME · 2023-10-18T16:37:01Z

I have send a new PR #82 for value part. Let's complete it first and then continue on working this PR later.

ZENOTME · 2023-11-30T15:28:44Z

Sorry for being late. The #82 has closed. Let's move again. I have fixed the problem mentioned above and recommit.

ZENOTME · 2023-11-30T15:33:39Z

Seem the binary file will make typos fail. Maybe we should fix #70 first. Other parts is ready to review.

liurenjie1024

This PR looks great! We are close to manipulating metadata in rust! Left some small comments to improve.

crates/iceberg/src/spec/manifest_list.rs

liurenjie1024 · 2023-12-04T07:33:51Z

crates/iceberg/src/spec/transform.rs

@@ -126,17 +126,20 @@ pub enum Transform {
 impl Transform {
    /// Get the return type of transform given the input type.
    /// Returns `None` if it can't be transformed.
-    pub fn result_type(&self, input_type: &Type) -> Option<Type> {
+    pub fn result_type(&self, input_type: &Type) -> Result<Type> {


Why we need to change this signature? I think an Option is more appropriate here. I think we can have a method try_infer_return_type to convert Option to Result?

The None case is the case which is not support in iceberg.🤔 Why not treat it as a error.

They have different meaning. When return Option, it acts more like a check to say if this transform can be applied, and whether it's an error is up to user. But their difference seems negligible. cc @Xuanwo @JanKaul @Fokko What do you think?

I think there is a difference, let me explain! In Iceberg it is possible to have your custom transform, for example if you want to do something very specific, or you want to influence the distribution when it comes to partitioning. It can be that the transform is not available in the client, and then we cast it to an UnknownTransform: https://github.com/apache/iceberg-python/blob/8c8abb5c4c258e32941110a9ce0938e1328290b3/pyiceberg/transforms.py#L688

What this essentially means is that we don't know how it is transformed, and we cannot apply speedups. For example, bucket[16](34) = 3 then we know in which bucket the data is that we're looking for. But if there is some custom partitioning then we just have to look through all the buckets.

To this specific case of the type, in PyIceberg we return a StringType(), that's borrowed from Java where you just cast it to a string. I don't think that's used anywhere since we don't call this on the UnknownTransform. The Type cannot be None, so I think having Result is the right thing.

crates/iceberg/src/spec/manifest.rs

liurenjie1024 · 2023-12-12T08:09:09Z

I think we need to add configurations in typos.toml to skip the check:

https://github.com/crate-ci/typos/blob/master/docs/reference.md

Xuanwo · 2023-12-13T04:29:40Z

I think we need to add configurations in typos.toml to skip the check:

https://github.com/crate-ci/typos/blob/master/docs/reference.md

Yep, we should skip check on testdata.

ZENOTME · 2023-12-13T05:17:56Z

I think we need to add configurations in typos.toml to skip the check:
https://github.com/crate-ci/typos/blob/master/docs/reference.md

Yep, we should skip check on testdata.

I have added .typo.toml to skip.

crates/iceberg/src/spec/partition.rs

crates/iceberg/src/spec/manifest_list.rs

crates/iceberg/src/spec/manifest.rs

.typos.toml

liurenjie1024

LGTM, thanks for the effort!

crates/iceberg/src/spec/manifest.rs

Fokko · 2023-12-13T09:51:47Z

crates/iceberg/src/spec/manifest_list.rs

-                sequence_number: 1,
-                min_sequence_number: 1,
-                added_snapshot_id: 377075049360453639,
+                sequence_number: UNASSIGNED_SEQUENCE_NUMBER,


On the manifest list the snapshot should be assigned, maybe we can add a note here that it should be fixed once we get the snapshots generation in there.

I'm sorry. I'm not sure what you mean.🤔 In here the snapshot id is assigned. What's the snapshots generation?

Fokko · 2023-12-13T09:57:42Z

crates/iceberg/src/spec/manifest.rs

+        assert!(entry.sequence_number.is_none());
+        assert!(entry.file_sequence_number.is_none());


These should default to 0. You could implement this using the initial-value of Iceberg:

Suggested change

assert!(entry.sequence_number.is_none());

assert!(entry.file_sequence_number.is_none());

assert!(entry.sequence_number == Some(0));

assert!(entry.file_sequence_number == Some(0));

Fokko · 2023-12-13T10:07:57Z

crates/iceberg/src/spec/manifest.rs

+        assert!(entry.snapshot_id == Some(0));
+        assert!(entry.sequence_number == Some(1));
+        assert!(entry.file_sequence_number == Some(1));


This is some interesting test-data 🤔 The status is added, so this means a new file which typically doesn't set the snapshot-id, sequence-number, and file-sequence-number since they should be inherited. This way we don't have to rewrite the manifest after a failed conflict (because a new snapshot-id and sequence-number is assigned).

Fokko

LGTM! Thanks for putting so much effort into this @ZENOTME 🙌 Thanks for the review @JanKaul and @liurenjie1024. I think there are still some loose ends with the snapshot-id and sequence-number inheritance, but we can tackle that separately.

liurenjie1024 reviewed Oct 13, 2023

View reviewed changes

crates/iceberg/src/spec/partition.rs Outdated Show resolved Hide resolved

crates/iceberg/src/spec/values.rs Outdated Show resolved Hide resolved

JanKaul reviewed Oct 16, 2023

View reviewed changes

crates/iceberg/src/spec/manifest.rs Show resolved Hide resolved

crates/iceberg/src/spec/manifest.rs Outdated Show resolved Hide resolved

liurenjie1024 mentioned this pull request Oct 17, 2023

Implement basic file scan planning. #66

Closed

JanKaul reviewed Oct 17, 2023

View reviewed changes

crates/iceberg/src/spec/manifest.rs Outdated Show resolved Hide resolved

Fokko reviewed Oct 18, 2023

View reviewed changes

crates/iceberg/src/spec/manifest.rs Show resolved Hide resolved

Fokko reviewed Oct 18, 2023

View reviewed changes

crates/iceberg/src/spec/manifest.rs Outdated Show resolved Hide resolved

Fokko reviewed Oct 18, 2023

View reviewed changes

crates/iceberg/src/spec/manifest.rs Show resolved Hide resolved

Fokko reviewed Oct 18, 2023

View reviewed changes

crates/iceberg/src/spec/values.rs Outdated Show resolved Hide resolved

ZENOTME force-pushed the manifest branch from 79a4f64 to 7262a9f Compare November 30, 2023 15:26

liurenjie1024 reviewed Dec 4, 2023

View reviewed changes

ZENOTME added 2 commits December 12, 2023 14:51

support read/write Manifest

22e07d8

refine in memory struct

a72cecd

ZENOTME force-pushed the manifest branch from ff82174 to a72cecd Compare December 12, 2023 06:51

ZENOTME added 2 commits December 13, 2023 13:16

fix seq num assign

0070793

ignore typos testdata

fc80ffa

liurenjie1024 reviewed Dec 13, 2023

View reviewed changes

.typos.toml Outdated Show resolved Hide resolved

refine

251ad77

ZENOTME force-pushed the manifest branch from 8a4708c to 251ad77 Compare December 13, 2023 07:50

add license for typos.toml

70d3b31

liurenjie1024 approved these changes Dec 13, 2023

View reviewed changes

Fokko reviewed Dec 13, 2023

View reviewed changes

crates/iceberg/src/spec/manifest.rs Outdated Show resolved Hide resolved

remove distinct_counts

6781ba9

Fokko reviewed Dec 13, 2023

View reviewed changes

This was referenced Dec 13, 2023

Support initial-default when reading Avro #119

Closed

test: Replace binary avro file by generating it on the fly. #70

Closed

Fokko approved these changes Dec 13, 2023

View reviewed changes

Fokko merged commit e0e2b1b into apache:main Dec 13, 2023
6 checks passed

ZENOTME deleted the manifest branch December 14, 2023 03:26

liurenjie1024 mentioned this pull request Dec 14, 2023

Read ManifestList, Manifest #36

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: suport read/write Manifest #79

feat: suport read/write Manifest #79

ZENOTME commented Oct 13, 2023

ZENOTME commented Oct 13, 2023

liurenjie1024 left a comment •

edited

ZENOTME commented Oct 18, 2023

ZENOTME commented Oct 18, 2023

ZENOTME commented Nov 30, 2023

ZENOTME commented Nov 30, 2023

liurenjie1024 left a comment

liurenjie1024 Dec 4, 2023

ZENOTME Dec 8, 2023

liurenjie1024 Dec 8, 2023

Fokko Dec 12, 2023 •

edited

liurenjie1024 Dec 12, 2023

liurenjie1024 commented Dec 12, 2023

Xuanwo commented Dec 13, 2023

ZENOTME commented Dec 13, 2023

liurenjie1024 left a comment

Fokko Dec 13, 2023

ZENOTME Dec 14, 2023

Fokko Dec 13, 2023

Fokko Dec 13, 2023

Fokko left a comment

		assert!(entry.sequence_number.is_none());
		assert!(entry.file_sequence_number.is_none());

feat: suport read/write Manifest #79

feat: suport read/write Manifest #79

Conversation

ZENOTME commented Oct 13, 2023

ZENOTME commented Oct 13, 2023

liurenjie1024 left a comment • edited

Choose a reason for hiding this comment

ZENOTME commented Oct 18, 2023

ZENOTME commented Oct 18, 2023

ZENOTME commented Nov 30, 2023

ZENOTME commented Nov 30, 2023

liurenjie1024 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fokko Dec 12, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liurenjie1024 commented Dec 12, 2023

Xuanwo commented Dec 13, 2023

ZENOTME commented Dec 13, 2023

liurenjie1024 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fokko left a comment

Choose a reason for hiding this comment

liurenjie1024 left a comment •

edited

Fokko Dec 12, 2023 •

edited