Repository navigation
feat: Add DictionaryBatch write support to IPC encoder and writer - #926
Conversation
c2c74ae to
8dfc378
Compare
8dfc378 to
b1a66b6
Compare
|
Hi @niekverw I took where you started and iterated on it a bit. Thanks for the taking the first crack at this dictionary write support. |
|
Sorry that I didn't notice this PR! I was on vacation when you added it and my notifications only caught Rusty's PR. If you're still keen to pursue this I'm back from vacation now and happy to review. Rusty's also taken on delta dictionary support which I think is mostly independent of this. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #926 +/- ##
==========================================
- Coverage 79.96% 79.29% -0.68%
==========================================
Files 105 106 +1
Lines 15461 16316 +855
Branches 1738 1925 +187
==========================================
+ Hits 12364 12937 +573
- Misses 1998 2163 +165
- Partials 1099 1216 +117 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
paleolimbot
left a comment
paleolimbot
left a comment
There was a problem hiding this comment.
This is great work!
The CI failure is a small test issue...other than that just a question to make sure I'm understanding the nested dictionary situation.
@rustyconover I think we can merge this after fixing the test leak an integrate your additional work on top. What do you think?
| // Emitting a full (non-delta) DictionaryBatch for each dictionary before every | ||
| // RecordBatch keeps each batch's indices valid against the dictionary that precedes | ||
| // it, which is required because each array in the stream carries its own dictionary. |
There was a problem hiding this comment.
I think this is a good scope for this PR, but we will want to ensure we have some way to avoid emitting duplicate dictionaries as tracked follow-up.
| if (array_view->dictionary != NULL) { | ||
| NANOARROW_RETURN_NOT_OK(ArrowIpcWriterWriteDictionariesForArrayView( | ||
| writer, array_view->dictionary, next_id, error)); | ||
| } |
There was a problem hiding this comment.
The encoder rejects dictionary values that have a dictionary. Should this error instead? (Or if it works, can we add a test?)
|
I think before we merge this we should also more clearly make sure three things that aren't quite supported error effectively:
|
|
I think so too. I thought this PR was a great start thats why I did a bunch of work on top of it! I'd like to get dictionary write support landed, because then I'll roll it out across a few of my Query Farm's projects and really start to use nanoarrow a lot more! |
|
Hey all, no problem, just go with Rusty’s PR - I just wanted to share this as I was using it and seemed to work for me - no need to merge this specific PR if rusty has built a better solution. |
|
Thanks @niekverw |
|
I'll push a small commit when I'm back at the keyboard Tuesday to get CI passing and catalog the required follow-up work, and we'll use Rusty's PR to clean up. This is great work and there's no reason your name shouldn't be on the commit! |
paleolimbot
changed the title
…support) into ipc-write-compression.
Hi, I had implemented dictionary batch writing for a project that I was working on, I noticed a thread (#622) about it, so I just wanted to share what I had in case it is useful. I needed this for a project that emits dictionary-encoded columns over IPC. I use ArrowIpcWriterWriteDictionaryBatch directly, for the PR I wired it into WriteArrayStream so dictionaries get emitted automatically on the high-level path and added a write→read round-trip test. Happy to also adjust.