Use enum to represent BufferdBatch state #297
Conversation
| } | ||
| } | ||
|
|
||
| /// The data stored in this BufferedBatch |
There was a problem hiding this comment.
Using an enum it becomes clear in the code that only one of these two states is possible
| #[derive(Debug)] | ||
| enum BufferedBatchData { | ||
| /// The batch is in memory | ||
| InMemory(RecordBatch), |
There was a problem hiding this comment.
it might make sense to put size_estimation in this enum too to ensure it is only used with InMemory
| .add(buffered_batch.size_estimation); | ||
| self.join_metrics.spilled_rows.add(buffered_batch.num_rows); | ||
| } | ||
| BufferedBatchData::Spilled(_) => { |
There was a problem hiding this comment.
It isn't clear to me how this is prevented FWIW, but at least it now isn't silently ignored
There was a problem hiding this comment.
I think this is the error that @viirya is suggesting: https://github.com/apache/datafusion/pull/11218/files#r1682222150
|
|
||
| Ok(buffered_cols) | ||
| } | ||
| // Invalid combination |
There was a problem hiding this comment.
The compiler now can validate that that this combination is invalid rather than relying on a runtime check
|
Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days. |
This targets apache#11218
The idea is to encode the valid states of
BufferedBatchusing anenumrather than twoOptions