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

Minor: Split physical_plan/parquet/mod.rs into smaller modules #10727

Merged
merged 4 commits into from
Jun 1, 2024

Conversation

alamb
Copy link
Contributor

@alamb alamb commented May 30, 2024

Which issue does this PR close?

Part of #9929

Rationale for this change

While working on #9929 I am finding the code in the mod.rs file to be a bit hard to follow. I think it would be easier to understand if it was broken up into smaller files

What changes are included in this PR?

  1. Split physical_plan/parquet/mod.rs into smaller modules

Are these changes tested?

Existing CI

Are there any user-facing changes?

No, all structs are re-exported as ndded

@github-actions github-actions bot added the core Core DataFusion crate label May 30, 2024
@@ -698,175 +687,6 @@ impl ExecutionPlan for ParquetExec {
}
}

/// Implements [`FileOpener`] for a parquet file
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this code was all moved into smaller modules.

@alamb
Copy link
Contributor Author

alamb commented May 31, 2024

@Ted-Jiang or @waynexia I wonder if you would have time for what is hopefully a quick review?

Copy link
Member

@waynexia waynexia left a comment

Choose a reason for hiding this comment

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

I've checked the content side by side and there is no logic change. Thanks @alamb! This refactor makes it easier to read. I left some nit-picking comments:

Comment on lines +51 to +57
mod opener;
mod page_filter;
mod reader;
mod row_filter;
mod row_groups;
mod statistics;
mod writer;
Copy link
Member

Choose a reason for hiding this comment

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

Good division 👍

@alamb
Copy link
Contributor Author

alamb commented Jun 1, 2024

Thank you very much for the careful review @waynexia

@alamb alamb merged commit acd7106 into apache:main Jun 1, 2024
23 checks passed
@alamb alamb deleted the alamb/split_parquet branch June 1, 2024 10:57
findepi pushed a commit to findepi/datafusion that referenced this pull request Jul 16, 2024
…e#10727)

* Minor: Split physical_plan/parquet/mod.rs into smaller modules

* doc tweaks

* Add object store docs

* Apply suggestions from code review

Co-authored-by: Ruihang Xia <[email protected]>

---------

Co-authored-by: Ruihang Xia <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core DataFusion crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants