Enable parallel writing across row groups when writing encrypted parquet by rok · Pull Request #8162 · apache/arrow-rs

rok · 2025-08-16T20:40:57Z

Rationale for this change

#8029 introduced pub ArrowWriter.get_column_writers and pub ArrowWriter.append_row_group to enable multi-threaded parquet encrypted writing. However testing downstream showed the API is not feasible, see #8115.

What changes are included in this PR?

This introduces pub ArrowWriter.into_serialized_writer and deprecates pub ArrowWriter.get_column_writers and pub ArrowWriter.append_row_group. It also makes ArrowRowGroupWriterFactory public and adds a pub ArrowRowGroupWriterFactory.create_column_writers.

Are these changes tested?

This includes a DataFusion inspired test for concurrent writing across columns and row groups to make sure parallel writing is and remains possible with ArrowWriters API. Further we created a draft PR in DataFusion apache/datafusion#16738 to test for multithreaded writing support.

Are there any user-facing changes?

See description of changes.

rok · 2025-08-22T15:16:18Z

@alamb @adamreeve I think this will make for a better multithreading API.
Unfortunately this misses the release window and I assume we now have to deprecate a couple of ArrowWriter methods. Do we have a policy for this @alamb ?

parquet/tests/encryption/encryption_async.rs

adamreeve

Looks good to me thanks Rok, just some minor comments

parquet/tests/encryption/encryption_async.rs

rok · 2025-09-01T09:45:03Z

Thanks for the review @adamreeve ! I suppose it's time to ask @alamb to do a pass :)

albertlockett

LGTM

alamb

Thanks @rok and @adamreeve -- I went over the PR more carefully and it makes sense to me. The examples are quite cool.

Thanks again

alamb · 2025-09-15T19:27:50Z

I merged up from main and plan to merge this PR in once the tests pass

alamb · 2025-09-15T20:05:17Z

Hi @rok and @adamreeve -- there appears to be a clippy error now (due to a new API that was added on main). Can you possibly take a look?

Again, I apologize for the delay in review

rok · 2025-09-15T21:03:19Z

Thanks for the review @alamb!
It seems we missed a codepath. I'll fix it tomorrow.

rok · 2025-09-15T21:06:52Z

@alamb I've pushed a couple of ignores just to confirm the codepath. Please don't merge yet.

rok · 2025-09-17T10:35:12Z

@alamb the deprecation warning comes from pub get_column_writers and pub append_row_group being added to AsyncArrowWriter since last release. These (append_row_group, append_row_group) were added to ArrowWriter just prior the last release, but turned out to not be great API for downstream writing due to locking issues so we decided to deprecate them to as they were not needed and to reduce pub area as suggested here.
We would suggest removing pub get_column_writers and pub append_row_group from both ArrowWriter and AsyncArrowWriter before the next release and this PR reflects that.

Co-authored-by: Adam Reeve <[email protected]>

rok · 2025-09-17T10:45:11Z

I don't have the mixed sync/async-write test quite right yet, but I wonder if we really need it?

alamb · 2025-09-17T16:55:10Z

I am getting ready to create the 56.2.0 release candidate. Shall we try and get this one in, or can it wait for 57.0.0 in October?

Update version to 56.2.0, add changelog #8372

rok · 2025-09-17T17:03:14Z

@alamb it'd be great to get this one in. Let me take a look if I can fix up the test_async_arrow_group_writer test.

adamreeve · 2025-09-17T17:56:38Z

The motivation for #8262 seems to be making the AsyncArrowWriter API match ArrowWriter. Is the appropriate fix then to add into_serialized_writer to AsyncArrowWriter instead of get_column_writers and append_row_group?

cc @lilianm

alamb · 2025-09-18T11:29:21Z

The motivation for #8262 seems to be making the AsyncArrowWriter API match ArrowWriter. Is the appropriate fix then to add into_serialized_writer to AsyncArrowWriter instead of get_column_writers and append_row_group?

cc @lilianm

I am not sure -- what I would love to see is an example like this https://docs.rs/parquet/latest/parquet/arrow/arrow_writer/struct.ArrowColumnWriter.html#example-encoding-two-arrow-arrays-in-parallel

That shows how to use whatever APIs we have to write data in parallel

I am sorry I haven't found time to study this PR in detail. @adamreeve it sounds like you have some good ideas and have devoted time here. What would you recommend?

alamb

Ok, I went over this PR quite carefully this morning, and reviewed all the PRs / APIs and I think it makes sense. Thank you @rok and @adamreeve

Divergent Writing APIs

One thought I had (which we can do as a follow on PR) would be to unify the APIs for doing concurrent writing so they always use ArrowRowGroupWriterFactory, which I think would mean:

Deprecate get_column_writers
Make ArrowRowGroupWriterFactory::new public
Update the examples to use ArrowRowGroupWriterFactory

If this sounds reasonable, I can file a ticket.

I will also see if I can debug the test failures

alamb · 2025-09-18T11:35:53Z

parquet/src/arrow/async_writer/mod.rs

    }
-
-    /// Create a new row group writer and return its column writers.
-    pub async fn get_column_writers(&mut self) -> Result<Vec<ArrowColumnWriter>> {


I double checked and this code is not yet released, so this is not a public API change. It was added in

[Parquet] Write row group with async writer #8262

It is very fresh indeed. I hope we're not spoiling some plan here. We did find the ArrowRowGroupWriterFactory route better so I think it should be ok.

alamb · 2025-09-18T12:17:11Z

parquet/src/arrow/arrow_writer/mod.rs

    }

    /// Create a new row group writer and return its column writers.
+    #[deprecated(since = "56.2.0", note = "Use into_serialized_writer instead")]


Note to myself, these APIs were added in

Support multi-threaded writing of Parquet files with modular encryption #8029
So they are relatively new

alamb · 2025-09-18T12:39:33Z

parquet/src/arrow/async_writer/mod.rs


-        let mut writers = writer.get_column_writers().await.unwrap();
+        // Use low-level API to write an Arrow group
+        let arrow_writer = writer.sync_writer;


I think it would be good to use public APIs in the tests, rather than accessing the inner fields directly (which is not possible from other crates)

alamb · 2025-09-18T12:50:31Z

parquet/src/arrow/arrow_writer/mod.rs

+
+    /// Converts this writer into a lower-level [`SerializedFileWriter`] and [`ArrowRowGroupWriterFactory`].
+    /// This can be useful to provide more control over how files are written.
+    pub fn into_serialized_writer(


I spent quite some time trying to figure out why this API is needed -- specifically "why do we need an ArrowWriter at all, why not use SerializedFileWriter and get_column_writers directly, as shown in this example

After study I concluded the reason we need to expose ArrowRowGroupWriterFactory is that ArrowRowGroupWriterFactory::create_column_writers also has the appropriate encryption properties.

It is unfortunate that we'll now have two different sets of APIs for creating column writers -- via get_column_writers AND ArrowRowGroupWriterFactory

I filed a ticket with a suggestion for a unified API:

Unify API for writing column chunks / row groups in parallel #8389

alamb · 2025-09-18T12:56:11Z

parquet/src/arrow/arrow_writer/mod.rs

        Ok(())
    }
+
+    /// Converts this writer into a lower-level [`SerializedFileWriter`] and [`ArrowRowGroupWriterFactory`].


I spent quite a while trying to figure out why we can't just use get_column_writers as in the example to use ArrowRowGroupWriterFactory and I (finally) realized the reason is the encryption configuration isn't passed to get_column_writers. ArrowRowGroupWriterFactory does have the encryption details and thus can make the correct ArrowColumnWriters.

I think it is somewhat of a strange API to create an ArrowWriter only to immediately destructure it into a SerializedWriter / the underlying writer. It is also unfortunate we now have two different APIs for writing row groups in parallel, depending on encryption.

I have an idea to make the APIs better as a follow on.

Agreed it's odd. Motivation was to introduce as few new pubs as possible. Would be very curious about alternative API shapes.

#8162 (review) is my suggestion , basically TLDR is to make ArrowRowGroupWriterFactory constructor public and deprecate get_column_writers

I spent quite a while trying to figure out why we can't just use get_column_writers.

Sorry, I realise now that I didn't make this very clear in my original issue (#7359).

One other factor is that we couldn't just use the WriterProperties passed to get_column_writers to internally create a new FileEncryptor. When a FileEncryptor is created for the SerializedFileWriter, it generates random AAD (additional authentication data), and this AAD has to be the same for all encrypted modules in the file.

I filed a ticket with a suggestion for a unified API:

Unify API for writing column chunks / row groups in parallel #8389

alamb · 2025-09-18T12:57:22Z

parquet/src/arrow/arrow_writer/mod.rs

    }
+
+    /// Create column writers for a new row group.
+    pub fn create_column_writers(&self, row_group_index: usize) -> Result<Vec<ArrowColumnWriter>> {


I see now that this is the key API -- create column writers with the relevant encryption properties, if relevant

What do you thinks to set create_row_group_writer pub and remove this function and use function into_writers add in issue #8260

So I am not sure making ArrowRowGroupWriter public gets us much of anything, and it would not allow per-column parallel encoding

One benefit of getting the column writers individually, is that then the columns can be encoded in parallel. The ArrowRowGroupWriter can only write RowGroups in parallel.

I looked at ArrowRowGroupWriter a bit more, and the only substantial thing it does is call a loop with compute_leaves which is already public.

arrow-rs/parquet/src/arrow/arrow_writer/mod.rs

Lines 831 to 835 in bac3690

struct ArrowRowGroupWriter {

writers: Vec<ArrowColumnWriter>,

schema: SchemaRef,

buffered_rows: usize,

}

alamb

I removed the failing test (which was also added in #8262 which we removed in this PR too)

alamb · 2025-09-18T13:14:29Z

parquet/src/arrow/async_writer/mod.rs

        assert_eq!(to_write, read);
    }

-    #[tokio::test]


I removed this test because it was added in [Parquet] Write row group with async writer #8262 along with AsyncArrowWriter::get_column_writers and AsyncArrowWriter::append_row_group

Since we removed those APIs in this PR, we should also remove the test

rok · 2025-09-18T15:52:36Z

Sorry for the late response @alamb and thanks for the added changes. I think this is good as is and can be included in the release.

alamb · 2025-09-18T17:12:01Z

Sorry for the late response @alamb and thanks for the added changes. I think this is good as is and can be included in the release.

This was my fault for not reviewing this PR before in more detail.

I leave this open until tomorrow to give people a chance to respond, and if not I'll merge it in and make the RC

adamreeve · 2025-09-19T01:56:47Z

I am sorry I haven't found time to study this PR in detail. @adamreeve it sounds like you have some good ideas and have devoted time here. What would you recommend?

I've read through the latest comments and it sounds like you have a good understanding of the motivation for this change now, thanks for taking a close look Andrew.

I agree it would be good to consolidate on using ArrowRowGroupWriterFactory for concurrent writing and update the examples. And also agree that creating an ArrowWriter just to convert it straight away to an ArrowRowGroupWriterFactory is a bit awkward. It does conveniently also create a SerializedFileWriter using the Arrow schema, but that's not too complicated to do explicitly, and the existing concurrent writing example already does this.

If you'd rather not have the new into_serialized_writer method and require creating the ArrowRowGroupWriterFactory directly, maybe we should take some more time to implement your suggested approach and wait for the next release. But I think into_serialized_writer is convenient and unlikely to cause much extra maintenance burden.

alamb · 2025-09-19T10:44:32Z

If you'd rather not have the new into_serialized_writer method and require creating the ArrowRowGroupWriterFactory directly, maybe we should take some more time to implement your suggested approach and wait for the next release. But I think into_serialized_writer is convenient and unlikely to cause much extra maintenance burden.

Yeah I agree -- one method is not a big deal if we are going to go with the ArrowRowGroupWriterFactory

I'll merge this PR and file some follow on tickets shortly

alamb

I think this one is ready to go so I am going to merge it in

I filed a ticket to track simplifying / unifying the API here

#8389

alamb · 2025-09-19T12:24:14Z

Thank you @rok @lilianm and @adamreeve

# Which issue does this PR close? - related to #8162 # Rationale for this change - While reviewing #8162 I read a bunch more of the parquet code and I wanted to capture some of my understanding in comments. # What changes are included in this PR? Add more documentation to various parquet writing APIs # Are these changes tested? By CI # Are there any user-facing changes? Documentation only, no function changes --------- Co-authored-by: Ed Seidl <[email protected]>

github-actions bot added the parquet Changes to the parquet crate label Aug 16, 2025

rok force-pushed the multi-threaded_encrypted_writing_3 branch 4 times, most recently from bcab6f9 to b701695 Compare August 22, 2025 15:03

rok marked this pull request as ready for review August 22, 2025 15:12

rok requested review from adamreeve and alamb and removed request for alamb August 22, 2025 15:16

rok mentioned this pull request Aug 22, 2025

feat: support multi-threaded writing of Parquet files with modular encryption apache/datafusion#16738

Merged

adamreeve reviewed Aug 24, 2025

View reviewed changes

parquet/tests/encryption/encryption_async.rs Outdated Show resolved Hide resolved

adamreeve approved these changes Aug 31, 2025

View reviewed changes

parquet/tests/encryption/encryption_async.rs Outdated Show resolved Hide resolved

parquet/tests/encryption/encryption_async.rs Outdated Show resolved Hide resolved

albertlockett approved these changes Sep 9, 2025

View reviewed changes

alamb approved these changes Sep 15, 2025

View reviewed changes

mbrobbel added this to the 56.2.0 milestone Sep 16, 2025

rok and others added 7 commits September 17, 2025 12:39

initial commit

854f591

work

b3d9f61

work

ef8d66a

work

4523b48

work

520e32b

Add ArrowWriter::into_serialized_writer and update tests

49c9a0d

Tidy ups

52d4f61

rok and others added 7 commits September 17, 2025 12:39

Deprecation of original API

bac3690

allow deprecated tests

d5f8ed4

Review feedback

71527d0

Update parquet/tests/encryption/encryption_async.rs

2d8334b

Co-authored-by: Adam Reeve <[email protected]>

Review feedback

16c612f

ignoring deprecation

71eaaf1

Removing to-be-deprecated from async

c4e38db

rok force-pushed the multi-threaded_encrypted_writing_3 branch from 15ed2de to c4e38db Compare September 17, 2025 10:39

alamb mentioned this pull request Sep 17, 2025

Release arrow-rs / parquet Minor version 56.2.0 (September 2025) #7836

Closed

6 tasks

adamreeve mentioned this pull request Sep 17, 2025

[Parquet] expose ArrowRowGroupWriter #8260

Closed

alamb mentioned this pull request Sep 18, 2025

Docs: Add more comments to the Parquet writer code #8383

Merged

alamb approved these changes Sep 18, 2025

View reviewed changes

Remove outdated test

2bc4556

alamb reviewed Sep 18, 2025

View reviewed changes

clippy

dae1793

alamb mentioned this pull request Sep 18, 2025

Update version to 56.2.0, add changelog #8372

Merged

alamb mentioned this pull request Sep 19, 2025

Unify API for writing column chunks / row groups in parallel #8389

Closed

alamb approved these changes Sep 19, 2025

View reviewed changes

alamb merged commit 322745d into apache:main Sep 19, 2025
17 checks passed

	struct ArrowRowGroupWriter {
	writers: Vec<ArrowColumnWriter>,
	schema: SchemaRef,
	buffered_rows: usize,
	}

Conversation

rok commented Aug 16, 2025 • edited by alamb Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

rok commented Aug 22, 2025

Uh oh!

Uh oh!

adamreeve left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rok commented Sep 1, 2025

Uh oh!

albertlockett left a comment

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alamb commented Sep 15, 2025

Uh oh!

alamb commented Sep 15, 2025

Uh oh!

rok commented Sep 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rok commented Sep 15, 2025

Uh oh!

rok commented Sep 17, 2025

Uh oh!

rok commented Sep 17, 2025

Uh oh!

alamb commented Sep 17, 2025

Uh oh!

rok commented Sep 17, 2025

Uh oh!

adamreeve commented Sep 17, 2025

Uh oh!

alamb commented Sep 18, 2025

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Divergent Writing APIs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

rok commented Aug 16, 2025 •

edited by alamb

Loading

rok commented Sep 15, 2025 •

edited

Loading