Repository navigation
Add write support to Avro format - #24925
Conversation
github-actions
Bot
added
sqllogictest
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24925 +/- ##
==========================================
+ Coverage 81.45% 81.74% +0.28%
==========================================
Files 1120 1128 +8
Lines 401289 416706 +15417
Branches 401289 416706 +15417
==========================================
+ Hits 326874 340626 +13752
- Misses 55296 56013 +717
- Partials 19119 20067 +948 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
This MR does not yet support the capability to specify compression. Should that be added as well? |
There was a problem hiding this comment.
Thanks for working on this. The Avro write support looks good overall. I have one non-blocking suggestion around testing the buffered flush path.
This MR does not yet support the capability to specify compression. Should that be added as well?
This can be a follow up PR.
| .write(&batch) | ||
| .map_err(|e| internal_datafusion_err!("{e}"))?; | ||
| let mut buff_to_flush = shared_buffer.buffer.try_lock().unwrap(); | ||
| if buff_to_flush.len() > BUFFER_FLUSH_BYTES { |
There was a problem hiding this comment.
Could we add a round-trip test where the encoded Avro output exceeds BUFFER_FLUSH_BYTES, ideally across multiple input batches? The current tests don't appear to exercise the path where SharedBuffer is cleared and later Avro blocks are appended to the object-store writer. It would be good to cover this boundary to make sure larger, multi-block files aren't accidentally truncated or malformed.
There was a problem hiding this comment.
To be honest, I was kind of lazy here and pretty much copy/pasted from the datasource-arrow crate. I don't think that has a test covering exceeding the buffer limit either.
Good idea to make sure this code path is actually covered by tests. Just a test that writes the same batch in a loop enough times so that the threshold is crossed at least once?
There was a problem hiding this comment.
I've added an SLT that triggers the flush code path multiple times and reads back the result.
|
🚀 |
|
Amazing |
|
Kudos, @pepijnve ! |
|
This is so cool. We can make tpch data in avro format 🎉 andrewlamb@Andrews-MacBook-Pro-3:~/Downloads$ tpchgen-cli parquet --tables lineitem
lineitem [==================] (100%) andrewlamb@Andrews-MacBook-Pro-3:~/Downloads$ datafusion-cli
DataFusion CLI v55.1.0
> COPY (SELECT * FROM 'lineitem.parquet') to 'lineitem.avro';
+---------+
| count |
+---------+
| 6001215 |
+---------+
1 row(s) fetched.
Elapsed 0.956 seconds.
>
\q
andrewlamb@Andrews-MacBook-Pro-3:~/Downloads$ ls -ltr lineitem.*
-rw-r--r--@ 1 andrewlamb staff 221M Sep 16 17:25 lineitem.parquet
-rw-r--r--@ 1 andrewlamb staff 456M Sep 16 17:25 lineitem.avro
andrewlamb@Andrews-MacBook-Pro-3:~/Downloads$ |
|
Standing on the shoulders of giants. Credit goes to everyone who contributed to the Avro writer in arrow-rs. |
6 participants
Which issue does this PR close?
Rationale for this change
The avro integration was still missing the necessary glue code to support writing. This MR adds the missing bits.
What changes are included in this PR?
This MR was authored using Claude Code and subsequently reviewed by myself.
What is the testing strategy for this PR?
Are there any user-facing changes?
Yes,
COPY ... TO ... STORED AS AVROnow works.