ARROW-6090: [Rust] [DataFusion] Physical plan for HashAggregate #5191
Conversation
|
@ursabot build |
andygrove
force-pushed
the
ARROW-6090
branch
2 times, most recently
from
94608b9 to
8464ff4
Compare
andygrove
changed the title
andygrove
changed the title
andygrove
force-pushed
the
ARROW-6090
branch
from
f545081 to
29f1197
Compare
|
@paddyhoran @nevi-me @sunchao This PR has now been rebased and is ready for review. Thanks! |
There was a problem hiding this comment.
Other than the comments above LGTM.
I would just check the examples before 0.15 release as you have re-arranged a few things lately. I created this issue for running the DataFusion examples in CI.
| @@ -1465,7 +1465,8 @@ mod tests { | |||
| } | |||
|
|
|||
| fn load_csv(filename: &str, schema: &Arc<Schema>) -> Rc<RefCell<dyn Relation>> { | |||
| let ds = CsvBatchIterator::new(filename, schema.clone(), true, &None, 10); | |||
| let ds = | |||
| CsvBatchIterator::try_new(filename, schema.clone(), true, &None, 10).unwrap(); | |||
There was a problem hiding this comment.
Shouldn't you propagate this error instead of using unwrap, this is the error from File::open so it's reasonable that load functions would have to deal with this, including load_csv.
If there is a strong reason why you don't want to maybe just switch to expect.
Also, why are you not exposing has_header, etc.? In particular, why is 10 used as the batch size?
There was a problem hiding this comment.
This is test code to support the unit tests and not part of the actual product ... but you are right, it would be better to have this method return a Result. I will fix that tonight.
There was a problem hiding this comment.
This is test code to support the unit tests and not part of the actual product
Ahh, ok sorry. It's hard to see the context while reviewing on github. No need to change if it's test code.
This PR implements the HashAggregate execution plan.