Use generic operator #583
Merged
Use generic operator #583
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@kaiyingshan this failure is in GCylon. Could you please check that as well? |
nirandaperera
requested changes
May 18, 2022
cpp/src/cylon/table.cpp
Outdated
Comment on lines
1477
to
1491
| std::vector<int64_t> num_row_v = {num_row}, rows_per_partition; | ||
|
|
||
| std::shared_ptr<cylon::Column> num_row_col; | ||
| RETURN_CYLON_STATUS_IF_FAILED(Column::FromVector(num_row_v, num_row_col)); | ||
|
|
||
| std::vector<std::shared_ptr<cylon::Column>> output; | ||
|
|
||
| RETURN_CYLON_STATUS_IF_FAILED( | ||
| a->GetContext()->GetCommunicator()->Allgather(num_row_col, &output)); | ||
|
|
||
| for (auto col : output) { | ||
| auto temp = std::static_pointer_cast<arrow::Int64Scalar>( | ||
| col->data()->GetScalar(0).ValueOrDie()); | ||
| rows_per_partition.push_back(temp->value); | ||
| } |
There was a problem hiding this comment.
I think you should've used the Scalar all-gather here.
Suggested change
| std::vector<int64_t> num_row_v = {num_row}, rows_per_partition; | |
| std::shared_ptr<cylon::Column> num_row_col; | |
| RETURN_CYLON_STATUS_IF_FAILED(Column::FromVector(num_row_v, num_row_col)); | |
| std::vector<std::shared_ptr<cylon::Column>> output; | |
| RETURN_CYLON_STATUS_IF_FAILED( | |
| a->GetContext()->GetCommunicator()->Allgather(num_row_col, &output)); | |
| for (auto col : output) { | |
| auto temp = std::static_pointer_cast<arrow::Int64Scalar>( | |
| col->data()->GetScalar(0).ValueOrDie()); | |
| rows_per_partition.push_back(temp->value); | |
| } | |
| const auto& ctx = a->GetContext(); | |
| auto num_row_v = Scalar::Make(arrow::MakeScalar(num_row)); | |
| std::shared_ptr<Column> num_row_col; | |
| RETURN_CYLON_STATUS_IF_FAILED(ctx->GetCommunicator()->Allgather(num_row_v, &num_row_col)); | |
| int64_t* vals = std::static_pointer_cast<arrow::Int64Array>(num_row_col->data())->raw_values(); | |
| // TODO: it would have been nice, if you could use int64_t* vals, and int64_t num_row_col->length() instead of a vector | |
| // creating a vector here, copies data again | |
| std::vector<int64_t> rows_per_partition(vals, vals + num_row_col->length()); |
cpp/src/cylon/table.cpp
Outdated
Comment on lines
1523
to
1524
| auto sub_result_scalar = | ||
| std::make_shared<Scalar>(std::move(arrow::MakeScalar(int8_t(subResult)))); |
There was a problem hiding this comment.
when you are passing a shared_ptr to a constructor (Rvalue), moving is unnecessary. you would only have to move Lvalues.
Suggested change
| auto sub_result_scalar = | |
| std::make_shared<Scalar>(std::move(arrow::MakeScalar(int8_t(subResult)))); | |
| auto sub_result_scalar = std::make_shared<Scalar>(arrow::MakeScalar(int8_t(subResult))); |
cpp/src/cylon/table.cpp
Outdated
Comment on lines
1552
to
1567
| std::vector<int64_t> num_row_v = {num_row}, sizes; | ||
|
|
||
| std::shared_ptr<cylon::Column> num_row_col; | ||
| RETURN_CYLON_STATUS_IF_FAILED(Column::FromVector(num_row_v, num_row_col)); | ||
|
|
||
| std::vector<std::shared_ptr<cylon::Column>> sizes_cols; | ||
|
|
||
| RETURN_CYLON_STATUS_IF_FAILED( | ||
| table->GetContext()->GetCommunicator()->Allgather(num_row_col, | ||
| &sizes_cols)); | ||
|
|
||
| for (auto col : sizes_cols) { | ||
| auto temp = std::static_pointer_cast<arrow::Int64Scalar>( | ||
| col->data()->GetScalar(0).ValueOrDie()); | ||
| sizes.push_back(temp->value); | ||
| } |
There was a problem hiding this comment.
same comment as before!
cpp/src/cylon/table.cpp
Outdated
|
|
||
| auto result = DivideRowsEvenly(sizes); | ||
| auto size = | ||
| std::make_shared<Scalar>(std::move(arrow::MakeScalar(table->Rows()))); |
There was a problem hiding this comment.
Suggested change
| std::make_shared<Scalar>(std::move(arrow::MakeScalar(table->Rows()))); | |
| std::make_shared<Scalar>(arrow::MakeScalar(table->Rows())); |
nirandaperera
requested changes
May 23, 2022
cpp/src/cylon/table.cpp
Outdated
Comment on lines
1553
to
1566
| std::shared_ptr<cylon::Column> sizes_cols; | ||
| RETURN_CYLON_STATUS_IF_FAILED(Column::FromVector(sizes, sizes_cols)); | ||
|
|
||
| auto num_row_scalar = std::make_shared<Scalar>(arrow::MakeScalar(num_row)); | ||
|
|
||
| RETURN_CYLON_STATUS_IF_FAILED( | ||
| table->GetContext()->GetCommunicator()->Allgather(num_row_scalar, | ||
| &sizes_cols)); | ||
|
|
||
| for (int64_t i = 0; i < sizes_cols->length(); i++) { | ||
| auto temp = std::static_pointer_cast<arrow::Int64Scalar>( | ||
| sizes_cols->data()->GetScalar(i).ValueOrDie()); | ||
| sizes.push_back(temp->value); | ||
| } |
cpp/test/CMakeLists.txt
Outdated
| @@ -65,141 +65,7 @@ endfunction(cylon_run_test) | |||
| #Add tests as follows ... | |||
| # param 1 -- name of the test, param 2 -- number of processes | |||
There was a problem hiding this comment.
IMPORTANT: Please re-enable these tests!!!
Co-authored-by: niranda perera <[email protected]>
…/cylon into use-generic-operator
|
@kaiyingshan is this ready? Can you run a code reformatter on the changed files? |
nirandaperera
requested changes
May 26, 2022
cpp/test/partition_test.cpp
Outdated
| @@ -101,7 +101,7 @@ TEST_CASE("Partition testing", "[join]") { | |||
| } | |||
|
|
|||
| SECTION("dist sort test") { | |||
| status = cylon::DistributedSort(table, 0, output, true, {(uint32_t) WORLD_SZ, (uint64_t) table->Rows()}); | |||
| status = cylon::DistributedSort(table, 0, output, true, {(uint32_t) WORLD_SZ, (uint64_t) table->Rows(), SortOptions::REGULAR_SAMPLE}); | |||
There was a problem hiding this comment.
nit: I feel like this is more than 100 chars! 🙂
nirandaperera
approved these changes
May 26, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #586 among other things