Adding a primitive and high-level writer interface #248
Conversation
rtyler
force-pushed
the
mixing-in-deltalake_ext
branch
from
b07d95f to
550a9ba
Compare
rtyler
force-pushed
the
mixing-in-deltalake_ext
branch
from
180774c to
5e4edde
Compare
rtyler
added
binding/rust
rust/src/writer.rs
Outdated
| /** | ||
| * Attempt to construct the BufferedJSONWriter, will fail if the table's metadata is not | ||
| * present | ||
| */ |
There was a problem hiding this comment.
nit, worth considering standardizing all doc string comments to /// and //!, should save you two top and bottom lines as well. this seems to be the norm in other rust projects.
There was a problem hiding this comment.
I think this is super tedious and whatever "other rust projects do" doesn't convince me much unless there is a lint to enforce it 😄
I'll change it anyways, but I'm registering my protest 😈
There was a problem hiding this comment.
haha, I would agree with the tedious claim if you didn't put a * at the beginning of every line ;)
rtyler
force-pushed
the
mixing-in-deltalake_ext
branch
from
a1d41a5 to
c43ac47
Compare
rust/src/delta.rs
Outdated
| * zeros and the trailing c000 string | ||
| * | ||
| */ | ||
| path_parts.push(format!("part-00000-{}-c000.snappy.parquet", Uuid::new_v4())); |
There was a problem hiding this comment.
typical hadoop parquet has a UUID, that is shared between all files in the batch. e.g. this useful in debugging.
instead of hardcoded 00000, i'd ask to do an incrementing counter.
and instead of hard-coding snappy, i'd advice to take it from the compression engine used in ParquetBuffer .. WriterProperties.
There was a problem hiding this comment.
@nfx to summarize, this is the scheme we should be using for the partition name right?
part-{COUNTER}-{BATCH_UUID}-c000.ext.
Do you know what is the c000 segment for?
and instead of hard-coding snappy, i'd advice to take it from the compression engine used in ParquetBuffer .. WriterProperties.
I also agree with this one 👍
There was a problem hiding this comment.
@houqp c000 might be the bucket number, but I don’t remember from top of my head. Will look once open a laptop next week.
i was writing custom parquet writer 4 years ago and researched it a bit then. It’s referenced from classes referenced in InsertIntoHadoopFsRelation in Apache Spark, for sure.. or at least not too far deep from it.
Main part: file naming won’t mean much for reading and writing (unless bucketed). It’s just for troubleshooting when something goes terribly wrong and you have to manually recover from fs. Probably won’t happen with delta 😜
There was a problem hiding this comment.
The counter stuff I'll file a ticket for later
rtyler
force-pushed
the
mixing-in-deltalake_ext
branch
4 times, most recently
from
93cab12 to
63d45cf
Compare
Originally authored by @xianwill
…he "add" action This API might receive some changes to accommodate users which need to provide `txn` actions along with the add_file
…actions This should support use-cases where the caller needs to put a txn or two into the log
…level writer interface I envision this writers module encapsulating a few different flavors of high-level writers. The exact syntax/API surface that should be common for all the high-level writers should be largely identical but I'm not ready to stabilize around what's here for that. I also am on the fence on how this should handle (or not handle) `txn` actions
…, and finish up BufferedJSONWriter There are some optimizations that come to mind for this work, but at this point I think it's ready for simple high-level JSON writer usage
This commit also addresses a number of other code review comments
rtyler
force-pushed
the
mixing-in-deltalake_ext
branch
from
63d45cf to
b6f3bcb
Compare
rtyler
force-pushed
the
mixing-in-deltalake_ext
branch
from
b6f3bcb to
e1d8d10
Compare
4 participants
Description
This pull request is a work in progress, pending more complete riffing on the deltalake_ext.rs.
The objective here is that the primitive API will just handle writing some bytes to storage and the subsequent
addtransaction, whereas the high-level writer API will manage buffering of rows for a parquet file.