Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal to refactor some large individual files into modules #298

Open
sdd opened this issue Mar 24, 2024 · 5 comments
Open

Proposal to refactor some large individual files into modules #298

sdd opened this issue Mar 24, 2024 · 5 comments
Labels
enhancement New feature or request

Comments

@sdd
Copy link
Contributor

sdd commented Mar 24, 2024

A few of the source files within iceberg-rust are getting very large (especially manifest.rs, manifest_list.rs, schema.rs, and table_metadata.rs). Maybe it's just my taste but I find it painful to navigate when they get much over 1000 lines.

How would people feel about some PRs to turn these into module folders with nested files so that things like the serde code, builders, visitors etc can live in separate files in each subfolder, keeping the main mod.rs file focussed on the core type definitions & logic?

@marvinlanhenke
Copy link
Contributor

[...]with nested files so that things like the serde code, builders, visitors etc can live in separate files in each subfolder

I like the idea especially regarding the serde code. Other than that I think I can still navigate quite quickly with the structure we have atm.

Since a lot of features are still missing in terms of read support and a writer framework has not been implemented - I'd wait for those changes until I can reason better about a suitable structure / subfolders?

However, for the serde code I think it be appropriate to do it right now - since it's very limited in scope.

@liurenjie1024
Copy link
Contributor

In general I prefer smaller files, but I agree with @marvinlanhenke that for we can wait for more features to land before conducting further restructure. Maybe we can start with serde part and change them one by one so that we don't have too much conflicts?

cc @Xuanwo @ZENOTME @Fokko What do you think?

@Xuanwo
Copy link
Member

Xuanwo commented Mar 25, 2024

In general I prefer smaller files, but I agree with @marvinlanhenke that for we can wait for more features to land before conducting further restructure.

I feel the same way. It's too soon for us to undertake such refactors.

@liurenjie1024 liurenjie1024 added the enhancement New feature or request label Jun 15, 2024
@xxchan
Copy link
Member

xxchan commented Feb 23, 2025

Seems we can close this one in favor of concrete tasks #981 #980

@liurenjie1024
Copy link
Contributor

Seems we can close this one in favor of concrete tasks #981 #980

Or we could turn this into an umbrella issue and add #981 #980 as sub issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants