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.

Sign up for GitHub

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

Jump to bottom

Remove 'make_row', expose a 'Row::new' method instead. #6763

Merged
merged 1 commit into from
Nov 20, 2024
Merged

Remove 'make_row', expose a 'Row::new' method instead. #6763

merged 1 commit into from
Nov 20, 2024

Conversation

jonded94
Copy link
Contributor

@jonded94 jonded94 commented Nov 20, 2024 •
edited by alamb
Loading

Which issue does this PR close?

Closes #6761
Closes #6762

Rationale for this change

parquet::record::make_row was the only method to create Row instances and was private. With a class method ::new, there is a new way of creating Row instances that is publicly exposed.

What changes are included in this PR?

  • Remove parquet::record::make_row
  • Add a parquet::record::Row::new method.

@github-actions github-actions bot added the parquet Changes to the parquet crate label Nov 20, 2024
@jonded94 jonded94 mentioned this pull request Nov 20, 2024
Closed
@jonded94
Remove 'make_row', expose a 'Row::new' method instead.
fd1b6ae 
Signed-off-by: Jonas Dedden <[email protected]>

Use `Row::new` also in internal tests.

Signed-off-by: Jonas Dedden <[email protected]>

cargo fmt
@jonded94 jonded94 force-pushed the introduce_row_new_method branch from 69e0a9d to fd1b6ae Compare November 20, 2024 20:43
alamb
alamb approved these changes Nov 20, 2024
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Makes sense to me -- thank you @jonded94 and @etseidl

parquet/src/record/api.rs
@@ -283,12 +288,6 @@ impl RowAccessor for Row {
row_complex_accessor!(get_map, MapInternal, Map);
}

/// Constructs a `Row` from the list of `fields` and returns it.
#[inline]
pub fn make_row(fields: Vec<(String, Field)>) -> Row {
Copy link
Contributor

Choose a reason for hiding this comment

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

I double checked and indeed this method is not public

https://docs.rs/parquet/latest/parquet/?search=make_row

@alamb
Copy link
Contributor

alamb commented Nov 20, 2024 •
edited
Loading

BTW using the Row API is likely to be quite slow to do processing with parquet. If you care about performance I would recommend looking at the other APIs that are available such as the arrow reader

@jonded94
Copy link
Contributor Author

If you care about performance I would recommend looking at the other APIs that are available such as the arrow reader

Yes, I implemented the data manipulation functionality that I need also already for whole RecordBatches & Arrays, but for a specific use case I also want to modify single rows. I'm aware that this is very much less efficient than it could be.

Thank you for your quick replies and collaboration!

@jonded94
Copy link
Contributor Author

That this CI action fails is not my fault, right?

@etseidl
Copy link
Contributor

etseidl commented Nov 20, 2024

That this CI action fails is not my fault, right?

Looks unrelated to me

etseidl
etseidl approved these changes Nov 20, 2024
View reviewed changes
Copy link
Contributor

@etseidl etseidl left a comment

Choose a reason for hiding this comment

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

I like this version much better! Thanks!

@tustvold tustvold merged commit 390f0ba into apache:master Nov 20, 2024
16 of 17 checks passed
@jonded94
Copy link
Contributor Author

Since you did a release only a few days ago, one can't expect a new release for another month, right?

@alamb
Copy link
Contributor

alamb commented Nov 21, 2024

Since you did a release only a few days ago, one can't expect a new release for another month, right?

That is correct. The next scheduled release is

@alamb alamb mentioned this pull request Dec 17, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@etseidl etseidl etseidl approved these changes

@alamb alamb alamb approved these changes

Assignees
No one assigned
Labels
parquet Changes to the parquet crate
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

parquet::record::make_row is not exposed to users, leaving no option to users to manually create Row objects
4 participants
@jonded94 @alamb @etseidl @tustvold