feat: Make FanoutWriter writer configurable
#1872
Conversation
|
cc @CTTY |
CTTY
left a comment
CTTY
left a comment
There was a problem hiding this comment.
Thanks for working on this! I've left some minor comments
|
|
||
| /// Whether to use `FanoutWriter` for partitioned tables (handles unsorted data). | ||
| /// If false, uses `ClusteredWriter` (requires sorted data, more memory efficient). | ||
| pub const PROPERTY_WRITE_FANOUT_ENABLED: &str = "write.fanout.enabled"; |
There was a problem hiding this comment.
I think write.datafusion.fanout.enabled will be a better property name since Java has
public static final String SPARK_WRITE_PARTITIONED_FANOUT_ENABLED = "write.spark.fanout.enabled";| props, | ||
| TableProperties::PROPERTY_WRITE_TARGET_FILE_SIZE_BYTES, | ||
| TableProperties::PROPERTY_WRITE_TARGET_FILE_SIZE_BYTES_DEFAULT, | ||
| )?, |
There was a problem hiding this comment.
Should we add the new property here as well?
| /// The default format for files. | ||
| pub write_format_default: String, | ||
| /// The target file size for files. | ||
| pub write_target_file_size_bytes: usize, |
There was a problem hiding this comment.
We need to add the property here as well
There was a problem hiding this comment.
I was thinking about this, I think whether or not to place it in the TableProperty struct is currently a bit inconsistent. write_format_default and write_target_file_size_bytes don't seem to be called by table_property.<property> and are manually parsed in write.rs. I'm not sure why they are parsed differently, but should we standardize one way to do it?
The different ways are currently calling try_from or parsing the way it is done in this pull request.
2 participants
Which issue does this PR close?
What changes are included in this PR?
Adds
PROPERTY_WRITE_FANOUT_ENABLEDtoTablePropertiesI made it
write.fanout.enabledinstead ofwrite.fanout-enabledin case there is anything we want configurable withFanoutWriterin the future.Are these changes tested?
Yes by existing tests.