#610: SampleArray util method replaced by using arrow::compute::Take … #612

mstaylor · 2022-08-26T17:32:37Z

…and pass corresponding indices to it. Note:

1)Commented code that was deprecated by the resolution of this issue
2) Note comments in sample_binary_array function related to result status.

…te::Take and pass corresponding indices to it. Note: 1)Commented code that was deprecated by the resolution of this issue 2) Note comments in sample_binary_array function related to result status.

…te::Take and pass corresponding indices to it - partial

nirandaperera · 2022-08-27T17:22:07Z

cpp/src/cylon/util/arrow_utils.cpp

+    auto arr_result = builder.Finish();
+
+    RETURN_ARROW_STATUS_IF_FAILED(arr_result.status());
+
+    auto indices = arr_result.ValueOrDie();


Suggested change

auto arr_result = builder.Finish();

RETURN_ARROW_STATUS_IF_FAILED(arr_result.status());

auto indices = arr_result.ValueOrDie();

ARROW_ASSIGN_OR_RAISE(auto indices, builder.Finish());

This is a convenient macro where arrow::Result is returned.

this should be done

nirandaperera · 2022-08-27T17:22:41Z

cpp/src/cylon/util/arrow_utils.cpp

+    auto sample_array = arrow::compute::Take(out, indices, take_options);
+
+    RETURN_ARROW_STATUS_IF_FAILED(sample_array.status());
+
+    out = sample_array.ValueOrDie().make_array();


same as above

this should be done

nirandaperera · 2022-08-27T17:26:20Z

cpp/src/cylon/util/arrow_utils.cpp

@@ -166,6 +166,7 @@ arrow::Status Duplicate(const std::shared_ptr<arrow::Table> &table, arrow::Memor
  return arrow::Status::OK();
 }

+/**


remove all commented out code

this should be done

…te::Take and pass corresponding indices to it - changes from PR review

…te::Take and pass corresponding indices to it - changes from build to using std:vector and std:sort. Vector data is moved to an arrow::Buffer::Wrap (wrapper) that can be used by the actual arrow::Array.

nirandaperera · 2022-08-31T01:29:14Z

@mstaylor Hey Bob, I added a vector wrapping util while fixing this issue #615. I think you can reuse it in this PR, if you take a pull from the main

mstaylor · 2022-08-31T15:04:22Z

@mstaylor Hey Bob, I added a vector wrapping util while fixing this issue #615. I think you can reuse it in this PR, if you take a pull from the main

OK - I will check on that.

…te::Take and pass corresponding indices to it - changes from build to using std:vector and std:sort. Vector data is moved to an arrow::Buffer::Wrap (wrapper) that can be used by the actual arrow::Array (continued - adds call to WrapNumericVector function added by cylondata#615-cylondata#616)

…nges to table.cpp

nirandaperera · 2022-09-08T16:43:11Z

cpp/src/cylon/util/arrow_utils.hpp

+cylon::Status SampleTableUniform(const std::shared_ptr<Table> &local_sorted,
+                                     int num_samples, std::vector<int32_t> sort_columns,
+                                     std::shared_ptr<Table> &sample_result,
+                                     const std::shared_ptr<CylonContext> &ctx);


Let's use all arrow objects in this header, and at the call site convert the arrow table to cylon table.

I don't think I understand why we would want to do this. Can you please elaborate on the reasoning.

nirandaperera · 2022-09-08T16:46:58Z

cpp/src/cylon/util/arrow_utils.cpp

+    std::vector<int64_t> vector_indices(num_samples);



You should do a reserve here. Otherwise vector would instantiate num_samples number of 0 in the vector.

this should be done

It's just syntactic sugar. All utils in this header are accepting/ returning arrow objects. 🙂

ok just pushed a change for this. Note my question on std::move. Since arrow::Table is an abstract class, std::move will not compile. Thoughts?

…ns and remove the incorrect initialization in the constructor.

mstaylor · 2022-09-12T20:45:17Z

@mstaylor Hey Bob, I added a vector wrapping util while fixing this issue #615. I think you can reuse it in this PR, if you take a pull from the main

OK - I will check on that.

This should be done

…ylon

nirandaperera

made some minor comments. Can you also run a code formatter with Google styling (100 chars)?

nirandaperera · 2022-09-18T20:15:31Z

cpp/src/cylon/table.cpp

+  auto status = util::SampleTableUniform(merged_table->get_table(), num_split_points, sort_columns,
+                                         const_cast<std::shared_ptr<arrow::Table> &>(split_points->get_table()), ctx);
+
+  return cylon::Status(static_cast<int>(status.code()), status.message());


@mstaylor this could be problematic. Hmm... This will actually create a NPE AFAIU.

Suggested change

auto status = util::SampleTableUniform(merged_table->get_table(), num_split_points, sort_columns,

const_cast<std::shared_ptr<arrow::Table> &>(split_points->get_table()), ctx);

return cylon::Status(static_cast<int>(status.code()), status.message());

std::shared_ptr<arrow::Table> atable;

RETURN_CYLON_STATUS_IF_ARROW_FAILS(util::SampleTableUniform(merged_table->get_table(),

num_split_points, sort_columns, atable, ctx);

return Table::FromArrowTable(ctx, std::move(atable), split_points);

nirandaperera · 2022-09-18T20:17:17Z

cpp/src/cylon/table.cpp

+
+  std::shared_ptr<Table> cylon_sample_result;
+
+  Table::FromArrowTable(ctx, sample_result, cylon_sample_result);


Suggested change

Table::FromArrowTable(ctx, sample_result, cylon_sample_result);

RETURN_CYLON_STATUS_IF_TAILED(Table::FromArrowTable(ctx, sample_result, cylon_sample_result));

nirandaperera · 2022-09-18T20:22:15Z

@mstaylor BTW please resolve the conflicts as well 🙂

# Conflicts: # cpp/src/cylon/table.cpp

mstaylor · 2022-09-27T16:13:28Z

made some minor comments. Can you also run a code formatter with Google styling (100 chars)?

Yes - the next build includes that requested change.

… applied google formatter per PR request. Validated tests complete successfully.

mstaylor · 2022-09-27T16:15:45Z

I've validated the build via the following command:

python build.py --cpp --test --python --pytest

mstaylor added 2 commits August 26, 2022 13:29

cylondata#610: SampleArray util method replaced by using arrow::compu…

5bede24

…te::Take and pass corresponding indices to it. Note: 1)Commented code that was deprecated by the resolution of this issue 2) Note comments in sample_binary_array function related to result status.

cylondata#610: SampleArray util method replaced by using arrow::compu…

54a1d65

…te::Take and pass corresponding indices to it - partial

nirandaperera requested changes Aug 27, 2022

View reviewed changes

mstaylor added 2 commits August 29, 2022 16:35

cylondata#610: SampleArray util method replaced by using arrow::compu…

74f846d

…te::Take and pass corresponding indices to it - changes from PR review

cylondata#610: SampleArray util method replaced by using arrow::compu…

94856aa

…te::Take and pass corresponding indices to it - changes from build to using std:vector and std:sort. Vector data is moved to an arrow::Buffer::Wrap (wrapper) that can be used by the actual arrow::Array.

mstaylor added 3 commits August 31, 2022 12:05

Merge branch 'main' into dev/issue/610

49fbb89

cylondata#610: move SampleTableUniform to arrow_utils and reflect cha…

796fd58

…nges to table.cpp

nirandaperera requested changes Sep 8, 2022

View reviewed changes

cylondata#610: correctly use reserve in vector to prevent reallocatio…

748afd1

…ns and remove the incorrect initialization in the constructor.

cylondata#610: changes SampleTableUniform to arrow datatypes versus c…

15d36aa

…ylon

nirandaperera requested changes Sep 18, 2022

View reviewed changes

Merge branch 'main' into dev/issue/610

aef19ea

# Conflicts: # cpp/src/cylon/table.cpp

cylondata#610: Implements requests change to address SIG Faults. Also…

44f6935

… applied google formatter per PR request. Validated tests complete successfully.

nirandaperera added 2 commits September 28, 2022 11:49

Update arrow_utils.cpp

cd633ee

Update arrow_utils.hpp

1bfeeb2

nirandaperera approved these changes Sep 28, 2022

View reviewed changes

nirandaperera merged commit ef4c904 into cylondata:main Sep 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#610: SampleArray util method replaced by using arrow::compute::Take … #612

#610: SampleArray util method replaced by using arrow::compute::Take … #612

mstaylor commented Aug 26, 2022

nirandaperera Aug 27, 2022

mstaylor Sep 12, 2022

nirandaperera Aug 27, 2022

mstaylor Sep 12, 2022

nirandaperera Aug 27, 2022

mstaylor Sep 12, 2022

nirandaperera commented Aug 31, 2022

mstaylor commented Aug 31, 2022

nirandaperera Sep 8, 2022

mstaylor Sep 12, 2022

nirandaperera Sep 8, 2022

mstaylor Sep 12, 2022

nirandaperera Sep 12, 2022

mstaylor Sep 15, 2022 •

edited

Loading

mstaylor commented Sep 12, 2022

nirandaperera left a comment

nirandaperera Sep 18, 2022

mstaylor Sep 27, 2022

nirandaperera Sep 18, 2022

mstaylor Sep 27, 2022

nirandaperera commented Sep 18, 2022

mstaylor commented Sep 27, 2022

mstaylor commented Sep 27, 2022

-  auto status = util::SampleTableUniform(merged_table->get_table(), num_split_points, sort_columns,
-                                         const_cast<std::shared_ptr<arrow::Table> &>(split_points->get_table()), ctx);
-  return cylon::Status(static_cast<int>(status.code()), status.message());
+  std::shared_ptr<arrow::Table> atable;
+  RETURN_CYLON_STATUS_IF_ARROW_FAILS(util::SampleTableUniform(merged_table->get_table(),
+  num_split_points, sort_columns, atable, ctx);
+  return Table::FromArrowTable(ctx, std::move(atable), split_points);


		std::shared_ptr<Table> cylon_sample_result;

		Table::FromArrowTable(ctx, sample_result, cylon_sample_result);

	Table::FromArrowTable(ctx, sample_result, cylon_sample_result);
	RETURN_CYLON_STATUS_IF_TAILED(Table::FromArrowTable(ctx, sample_result, cylon_sample_result));

#610: SampleArray util method replaced by using arrow::compute::Take … #612

#610: SampleArray util method replaced by using arrow::compute::Take … #612

Conversation

mstaylor commented Aug 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nirandaperera commented Aug 31, 2022

mstaylor commented Aug 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mstaylor Sep 15, 2022 • edited Loading

Choose a reason for hiding this comment

mstaylor commented Sep 12, 2022

nirandaperera left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nirandaperera commented Sep 18, 2022

mstaylor commented Sep 27, 2022

mstaylor commented Sep 27, 2022

mstaylor Sep 15, 2022 •

edited

Loading