Skip to content

fix: refine doc for write support#999

Merged
liurenjie1024 merged 1 commit into
apache:mainfrom
ZENOTME:write_example
Mar 11, 2025
Merged

fix: refine doc for write support#999
liurenjie1024 merged 1 commit into
apache:mainfrom
ZENOTME:write_example

Conversation

@ZENOTME

@ZENOTME ZENOTME commented Feb 23, 2025

Copy link
Copy Markdown
Contributor

Part of #986. This PR adds more examples to refine the doc of writer.

@ZENOTME ZENOTME force-pushed the write_example branch 3 times, most recently from 67c1c5a to 4b1713d Compare February 24, 2025 08:15
@ZENOTME

ZENOTME commented Feb 24, 2025

Copy link
Copy Markdown
Contributor Author

Feel free for any suggestion to make example better. cc @Xuanwo @liurenjie1024 @Fokko @kevinjqliu @xxchan @jonathanc-n

Comment thread crates/iceberg/src/lib.rs Outdated
//!
//! ## Fast append data to table
//!
//! ```rust, no_run

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test only use memory file io and memory catalog, it should be able to run?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should create table here to make it run.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally believe it's more valuable. Is the blocker here that we don't support creating tables yet?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I will add it later.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find that the memory catalog can't update now. So we should merge #1002 first.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I hope we can have more high-level api examples. I think this is an example of low-level api which may be useful for computing engines integrated with iceberg-rust.🤔

@liurenjie1024 liurenjie1024 Feb 26, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an example showing in crate front page, which I don't think is appropriate. If we actually want to tell compute engine how to integrate it, a better place would be to place it in website, which appears as an advanced tutorial.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an example showing in crate front page, which I don't think is appropriate.

Looks reasonable to me. What about also place it at https://github.com/apache/iceberg-rust/blob/main/crates/iceberg/src/writer/mod.rs

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean the whole example? I'm not a big fan of that way. For writer doc, it should only focus on how to use a writer. The whole story of using different picecs in a compute engine is an advanced topic, and deserves a single chapter for that. It involves not only code, but also explaination the design in it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean the whole example? I'm not a big fan of that way. For writer doc, it should only focus on how to use a writer. The whole story of using different picecs in a compute engine is an advanced topic, and deserves a single chapter for that. It involves not only code, but also explaination the design in it.

Sounds reasonable for me.

Comment thread crates/iceberg/src/lib.rs Outdated
//! .try_into()
//! .unwrap(),
//! );
//! let location_generator = DefaultLocationGenerator::new(table.metadata().clone()).unwrap();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are returning Result<()> here, how about using ? here for clean code?

Comment thread crates/iceberg/src/lib.rs Outdated

@liurenjie1024 liurenjie1024 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ZENOTME for this pr, LGTM!

@liurenjie1024 liurenjie1024 merged commit 6c6ef74 into apache:main Mar 11, 2025
@ZENOTME ZENOTME deleted the write_example branch March 11, 2025 09:15
@liurenjie1024 liurenjie1024 mentioned this pull request Mar 11, 2025
ZENOTME added a commit to risingwavelabs/iceberg-rust that referenced this pull request Mar 12, 2025
Part of apache#986. This PR adds more examples to refine the doc of writer.

Co-authored-by: ZENOTME <st810918843@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants