feat: support reading and writing`StringView` and `BinaryView` in parquet (part 2) by ariesdevil · Pull Request #5557 · apache/arrow-rs

ariesdevil · 2024-03-26T16:45:07Z

Which issue does this PR close?

Closes #5530

Rationale for this change

Use view_buffer instead of offset_buffer for view type parquet reader

What changes are included in this PR?

Use view_buffer instead of offset_buffer for view type parquet reader

Are there any user-facing changes?

Yes

ariesdevil · 2024-03-28T16:33:02Z

The parquet build for wasm32 check always failed, but it works on my machine...

ariesdevil · 2024-03-28T16:37:37Z

Hi @alamb @tustvold , this PR is not really ready for review, however, I may need some advice, and then I'll continue.

alamb · 2024-03-28T17:11:13Z

Thanks @ariesdevil -- I think @tustvold is away for a few days. I will try and give this PR a look over the next day or two. Very exciting!

cc @XiangpengHao

parquet/src/arrow/buffer/view_buffer.rs

alamb

Thank you @ariesdevil -- I think this is looking close to something we can merge 🙏

To merge this, I think we need:

Benchmarks for reading/writing StringView / BinaryViews
Cover a few more cases for round tripping

I think there is quite a bit more performance to be had as well by avoiding string copies. This PR will copy the data I think twice (once out of parquet buffer and once out of the offset buffer).

It would be nice to avoid at least one of those copies prior to merge, but I also think once we have the test coverage it would be ok to merge this PR and then do additional optimizations as follow on PRs

We can add benchmarks here:

reading

arrow-rs/parquet/benches/arrow_reader.rs

Lines 867 to 868 in 7e134f4

// string benchmarks

//==============================

writing:

arrow-rs/parquet/benches/arrow_writer.rs

Lines 382 to 392 in 7e134f4

    
           let batch = create_string_bench_batch(4096, 0.25, 0.75).unwrap(); 
        
           group.throughput(Throughput::Bytes( 
        
               batch 
        
                   .columns() 
        
                   .iter() 
        
                   .map(|f| f.get_array_memory_size() as u64) 
        
                   .sum(), 
        
           )); 
        
           group.bench_function("4096 values string", |b| { 
        
               b.iter(|| write_batch(&batch).unwrap()) 
        
           });

End to end round trip tests:

parquet/src/arrow/buffer/offset_buffer.rs

parquet/src/arrow/arrow_writer/mod.rs

alamb · 2024-04-01T18:58:19Z

This is really nice step forward -- thank you @ariesdevil

alamb

Thank you @ariesdevil -- I think this PR looks like a really nice incremental step.

As I think we have identified, it can be significantly improved performance wise but having the basic tests and benchmarks in place we are in a good space to optimize it.

I left some other smaller style / documentation comments that might be nice to cleanup, but I don't think they are strictly required

Let's wait another day for @tustvold to see if he wants to review (i think he is still not back yet) but otherwise we'll merge this PR and add additional features as a follow on PR

arrow-array/src/array/byte_view_array.rs

arrow/src/util/bench_util.rs

parquet/benches/arrow_writer.rs

parquet/src/arrow/buffer/offset_buffer.rs

alamb · 2024-04-04T15:25:21Z

I plan to give this another review today

alamb · 2024-04-05T20:44:10Z

The integration test https://github.com/apache/arrow-rs/actions/runs/8553472637/job/23436722309?pr=5557 seems to have failed for reasons unrelated to this PR. I have restarted it

################# FAILURES #################
FAILED TEST: union Rust producing,  Rust consuming
<class 'RuntimeError'>: Command failed: /build/rust/debug/flight-test-integration-client --host localhost --port=34799 --path /tmp/arrow-integration-92iwbbih/generated_union.json
With output:
--------------
Error: tonic::transport::Error(Transport, hyper::Error(Connect, ConnectError("tcp connect error", Os { code: 99, kind: AddrNotAvailable, message: "Cannot assign requested address" })))

alamb · 2024-04-05T20:49:03Z

I am sorry @ariesdevil -- I need more time and a fresh set of eyes to review the new implementation in this PR. I will find time over the next few days.

ariesdevil · 2024-04-06T03:44:15Z

Hi @alamb ,the new implementation has indeed changed a lot. It should have been implemented in another PR, but the original performance is really poor.

Thanks for your patience and have a nice weekend.

alamb · 2024-04-12T15:57:37Z

Validate utf8 data (I left comments). I think we need a test for that too -- I'll make a PR to make that easier to add.

I wrote new tests in #5639

alamb · 2024-04-16T15:31:43Z

I am on vacation this week but I hope to find some time later today or tomorrow to give this another look. Thanks @ariesdevil

ariesdevil · 2024-04-16T15:35:23Z

No need to rush, enjoy your vacation, just so I can have time to rethink how to continue optimization 😃

alamb · 2024-04-22T15:30:45Z

I see some new updates -- I plan to look at this PR later this week (probably won't be until Wednesday)

XiangpengHao · 2024-06-12T21:15:32Z

Hi @ariesdevil, thanks for the great work! I'm looking at this PR these days and trying to push it forward. However, I do feel the code change is too large for a single PR that it is quite difficult to understand every detail of the implementation.

I suggest we break the PR into even smaller pieces and incrementally add the features/optimizations so it poses less mental burdens on the reviewers, what do you think?

I'll try to consolidate a few low-hanging fruit changes and try to get them merged first; let me know your thoughts!

ariesdevil · 2024-06-13T03:09:21Z

Hi @XiangpengHao, thanks for your reply. I don't think this PR is very large, the previous reviewers are already very familiar with this PR, this PR has been submitted for almost three months, and the reviewer has had enough time to get familiar with it.

alamb · 2024-06-13T12:16:43Z

Hi @XiangpengHao, thanks for your reply. I don't think this PR is very large, the previous reviewers are already very familiar with this PR, this PR has been submitted for almost three months, and the reviewer has had enough time to get familiar with it.

Indeed -- I am sorry this PR is stalled @ariesdevil -- it has been very hard for me to find enough contiguous time to get familar enough with this PR and really understand it (and ensure the test coverage is adequate, etc).

@ariesdevil do you think we should attempt to merge this entire PR at once? I do think it would help speed up review if we could break it into some smaller PRs

ariesdevil · 2024-06-13T12:27:41Z

Hi @alamb, I understand how busy you must be maintaining multiple projects and writing an important paper simultaneously.

As we now have try_append_view implemented, we no need ViewBuffer any more, I'll rebase this PR after #5877 merged. After that, this PR should be smaller.

alamb · 2024-06-13T13:16:53Z

Hi @alamb, I understand how busy you must be maintaining multiple projects and writing an important paper simultaneously.

Thank you for understanding -- I feel bad whenever PRs get stuck.

As we now have try_append_view implemented, we no need ViewBuffer any more, I'll rebase this PR after #5877 merged. After that, this PR should be smaller.

That is awesome. THank you. I will make every effort to help get this over the line and now that @XiangpengHao is helping I am optimistic we can get it in shortly

alamb · 2024-06-13T13:22:23Z

#5877 is merged now!

This reverts commit 71f51aa10079cdadfe7a0f5026563eedd7f76bd9.

ariesdevil · 2024-06-15T02:49:38Z

closed by #5877

github-actions bot added parquet Changes to the parquet crate arrow Changes to the arrow crate labels Mar 26, 2024

ariesdevil force-pushed the parquet branch 5 times, most recently from 17695d3 to 1c7260e Compare March 28, 2024 16:23

ariesdevil marked this pull request as ready for review March 28, 2024 16:33

alamb mentioned this pull request Mar 28, 2024

DataFusion weekly project plan (Andrew Lamb) - March 25, 2024 apache/datafusion#9796

Closed

6 tasks

cgbur reviewed Mar 29, 2024

View reviewed changes

parquet/src/arrow/buffer/view_buffer.rs Outdated Show resolved Hide resolved

ariesdevil force-pushed the parquet branch 2 times, most recently from d2f1a30 to fdd2e48 Compare March 31, 2024 15:18

alamb changed the title ~~feat: support string and binary view read write parquet~~ feat: support reading and writingStringView and BinaryView in parquet Apr 1, 2024

alamb reviewed Apr 1, 2024

View reviewed changes

parquet/src/arrow/buffer/offset_buffer.rs Outdated Show resolved Hide resolved

parquet/src/arrow/arrow_writer/mod.rs Outdated Show resolved Hide resolved

parquet/src/arrow/arrow_writer/mod.rs Outdated Show resolved Hide resolved

alamb mentioned this pull request Apr 1, 2024

DataFusion weekly project plan (Andrew Lamb) - April 1, 2024 apache/datafusion#9899

Closed

7 tasks

ariesdevil force-pushed the parquet branch 2 times, most recently from f37c3dd to f65807f Compare April 2, 2024 15:17

alamb previously approved these changes Apr 2, 2024

View reviewed changes

arrow-array/src/array/byte_view_array.rs Outdated Show resolved Hide resolved

arrow/src/util/bench_util.rs Outdated Show resolved Hide resolved

parquet/benches/arrow_writer.rs Outdated Show resolved Hide resolved

parquet/src/arrow/buffer/offset_buffer.rs Outdated Show resolved Hide resolved

ariesdevil force-pushed the parquet branch 3 times, most recently from a22ee14 to d286521 Compare April 4, 2024 10:56

alamb mentioned this pull request Apr 8, 2024

DataFusion weekly project plan (Andrew Lamb) - April 8, 2024 apache/datafusion#10002

Closed

9 tasks

ariesdevil force-pushed the parquet branch 3 times, most recently from fef3c9f to cbf08f4 Compare April 15, 2024 13:13

ariesdevil force-pushed the parquet branch from cbf08f4 to a3213dd Compare April 22, 2024 11:53

XiangpengHao mentioned this pull request Jun 12, 2024

Avoid copy/allocation when read view types from parquet #5877

Merged

ariesdevil force-pushed the parquet branch from a3213dd to c17b217 Compare June 13, 2024 03:06

ariesdevil force-pushed the parquet branch from c17b217 to e7e219a Compare June 13, 2024 03:19

ariesdevil added 4 commits June 15, 2024 10:24

using view_buffer instead of offset_buffer for view type parquet reader

de5fddb

add check valid utf8

b0b22df

use different buffer for different encoding

54559b4

Revert "use different buffer for different encoding"

fd99ce2

This reverts commit 71f51aa10079cdadfe7a0f5026563eedd7f76bd9.

ariesdevil force-pushed the parquet branch 2 times, most recently from 5730aa0 to 7fe2847 Compare June 15, 2024 02:46

rebase master

4dd2e15

ariesdevil force-pushed the parquet branch from 7fe2847 to 4dd2e15 Compare June 15, 2024 02:48

ariesdevil closed this Jun 15, 2024

ariesdevil deleted the parquet branch June 15, 2024 02:49

alamb mentioned this pull request Jun 17, 2024

Potential performance improvements for reading Parquet to StringViewArray/BinaryViewArray #5904

Closed

XiangpengHao mentioned this pull request Jun 27, 2024

Add view buffer for parquet reader #5970

Merged

	let batch = create_string_bench_batch(4096, 0.25, 0.75).unwrap();
	group.throughput(Throughput::Bytes(
	batch
	.columns()
	.iter()
	.map(\|f\| f.get_array_memory_size() as u64)
	.sum(),
	));
	group.bench_function("4096 values string", \|b\| {
	b.iter(\|\| write_batch(&batch).unwrap())
	});

Comments

Conversation

ariesdevil commented Mar 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Uh oh!

ariesdevil commented Mar 28, 2024

Uh oh!

ariesdevil commented Mar 28, 2024

Uh oh!

alamb commented Mar 28, 2024

Uh oh!

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alamb commented Apr 1, 2024

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alamb commented Apr 4, 2024

Uh oh!

alamb commented Apr 5, 2024

Uh oh!

alamb commented Apr 5, 2024

Uh oh!

ariesdevil commented Apr 6, 2024

Uh oh!

alamb commented Apr 12, 2024

Uh oh!

alamb commented Apr 16, 2024

Uh oh!

ariesdevil commented Apr 16, 2024

Uh oh!

alamb commented Apr 22, 2024

Uh oh!

XiangpengHao commented Jun 12, 2024

Uh oh!

ariesdevil commented Jun 13, 2024

Uh oh!

alamb commented Jun 13, 2024

Uh oh!

ariesdevil commented Jun 13, 2024

Uh oh!

alamb commented Jun 13, 2024

Uh oh!

alamb commented Jun 13, 2024

Uh oh!

ariesdevil commented Jun 15, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ariesdevil commented Mar 26, 2024 •

edited

Loading