feat(datafusion): Add TaskWriter for DataFusion#1769
feat(datafusion): Add TaskWriter for DataFusion#1769liurenjie1024 merged 4 commits intoapache:mainfrom
Conversation
CTTY
force-pushed
the
ctty/task-writer
branch
from
e875e8e to
f4b72ef
Compare
liurenjie1024
left a comment
liurenjie1024
left a comment
There was a problem hiding this comment.
I think we are on the right track. I left some comments, and we need to split them into smaller prs.
| Fanout(FanoutWriter<B>), | ||
| /// Writer for partitioned tables with sorted data (maintains single active writer) | ||
| Clustered(ClusteredWriter<B>), |
There was a problem hiding this comment.
We could simplify this as
Partitioned {
splitter: RecordBatchSplitter,
partitioned_writer: Arc<dyn PartitionedWriter>
}…atchPartitionSplitter (#1781) ## Which issue does this PR close? - Closes #1786 - Covered some of changes from the previous draft: #1769 ## What changes are included in this PR? - Move PartitionValueCalculator to core/arrow so it can be reused by RecordBatchPartitionSplitter - Allow skipping partition value calculation in partition splitter for projected batches - Return <PartitionKey, RecordBatch> rather than <Struct, RecordBatch> pairs in RecordBatchPartitionSplitter::split ## Are these changes tested? Added uts
CTTY
force-pushed
the
ctty/task-writer
branch
from
c5061e2 to
d3d3127
Compare
CTTY
changed the title
liurenjie1024
left a comment
liurenjie1024
left a comment
There was a problem hiding this comment.
Thanks @CTTY for this pr, generally LGTM!
|
|
||
| Self { | ||
| writer, | ||
| partition_splitter: None, |
There was a problem hiding this comment.
Why not init it here?
There was a problem hiding this comment.
This is because partition_splitter requires the schema of record batches, which may contain projected column and differ from the Iceberg schema. it would be safer to just use the schema of record batches directly to initialize partition_splitter
There was a problem hiding this comment.
I'm not convinced, in fact, this schema could be inferred from datafusion's ExecutionPlan
There was a problem hiding this comment.
Anyway, this is a small code style problem, we can fix it later.
There was a problem hiding this comment.
i'll revisit this when using task writer in the write node
liurenjie1024
left a comment
liurenjie1024
left a comment
There was a problem hiding this comment.
Thanks @CTTY for this pr!
|
|
||
| Self { | ||
| writer, | ||
| partition_splitter: None, |
There was a problem hiding this comment.
I'm not convinced, in fact, this schema could be inferred from datafusion's ExecutionPlan
2 participants
Which issue does this PR close?
What changes are included in this PR?
RecordBatchPartitionSplitterand projected partition valuesAre these changes tested?
Added unit tests