Conversation
|
Thanks for opening a pull request! Could you open an issue for this pull request on Github Issues? Then could you also rename pull request title and commit log in the following format? See also: |
jikunshang
left a comment
jikunshang
left a comment
There was a problem hiding this comment.
Overall LGTM. some minor comments left. please rename PR title and add some description.
|
|
||
| // ReadBatchSpaced will record a null bitmap. | ||
| std::cout << std::endl << "test ReadBatchSpaced API" << std::endl; | ||
| std::cout<<"malloc values_rbs"<<std::endl; |
| @@ -0,0 +1,170 @@ | |||
| #include <stdlib.h> | |||
|
|
||
| TEST(predicateFilterTest, minMaxTest) | ||
| { | ||
| arrow::fs::HdfsOptions options_; |
|
|
||
| namespace ape { | ||
|
|
||
| class PredicateExpression : public Expression { |
There was a problem hiding this comment.
rename to RowGroupFilterExpression would be better?
|
|
||
| namespace ape { | ||
|
|
||
| class PredicateExpression : public Expression { |
There was a problem hiding this comment.
Possible to reuse FilterExpression since they use same expression? (say add PredicateWithParam method in FilterExpression)
|
Thanks a lot for Kunshang’s review. I commit a new C++ file instead of adding function to the existing expression so that the basic version wouldn’t be polluted. Yes they can be merged and become more clear. And I’ll look for and the other issues you mentioned and repair these. |
|
CI failed at code style check stage. Please fix it. |
2 participants
What changes were proposed in this pull request?
(Please fill in changes proposed in this fix)
How was this patch tested?
(Please explain how this patch was tested. E.g. unit tests, integration tests, manual tests)
(If this patch involves UI changes, please attach a screenshot; otherwise, remove this)