GH-32763: [C++] Add FromProto for fetch & sort #34651

westonpace · 2023-03-21T05:07:14Z

This does not support the clustered sort direction, custom sort functions, or complex (non-reference) expressions as sort keys.

Closes: [C++] Adding Fetch Node FromProto #32763

github-actions · 2023-03-21T05:07:48Z

Closes: [C++] Adding Fetch Node FromProto #32763

github-actions · 2023-03-21T05:07:52Z

⚠️ GitHub issue #32763 has been automatically assigned in GitHub to PR creator.

westonpace · 2023-03-22T19:41:16Z

CC @vibhatha / @rtpsw mind taking a look?

amol- · 2023-04-05T10:01:00Z

@vibhatha @rtpsw are you able to take a look as suggest by @westonpace ?
If this one is mostly complete and only needs rebasing, it would be great to get it reviewed and ship before v12 is released.

westonpace · 2023-04-10T21:32:21Z

Rebased.

westonpace · 2023-04-10T21:32:54Z

I think @rtpsw is out this week but maybe @icexelloss can take a look

vibhatha · 2023-04-11T01:13:44Z

@amol- @westonpace I will take a look.

vibhatha · 2023-04-11T02:31:39Z

cpp/src/arrow/engine/substrait/relation_internal.cc

+compute::SortOrder SortOrderFromDirection(
+    const substrait::SortField::SortDirection& direction) {
+  if (direction < 3) {
+    return compute::SortOrder::Ascending;


Do we consider UNSPECIFIED as Ascending?

Here:

enum SortDirection { SORT_DIRECTION_UNSPECIFIED = 0; SORT_DIRECTION_ASC_NULLS_FIRST = 1; SORT_DIRECTION_ASC_NULLS_LAST = 2; SORT_DIRECTION_DESC_NULLS_FIRST = 3; SORT_DIRECTION_DESC_NULLS_LAST = 4; SORT_DIRECTION_CLUSTERED = 5; }

I assume since the 1, 2 are Ascending, <3 is to pick the first three as Ascending.

Hmm, technically unspecified would be an invalid plan I think. So it would probably be better to reject.

This is now an explicit rejection.

case substrait::SortField::SortDirection:: SortField_SortDirection_SORT_DIRECTION_UNSPECIFIED: return Status::Invalid("The substrait plan does not specify a sort direction");

vibhatha · 2023-04-11T02:36:28Z

cpp/src/arrow/engine/substrait/relation_internal.cc

+namespace {
+
+bool IsSortNullsFirst(const substrait::SortField::SortDirection& direction) {
+  return direction % 2 == 1;


This is efficient, but shall we leave a comment.

But CLUSTERED has no null preference though.

We now throw an error on CLUSTERED:

case substrait::SortField::SortDirection:: SortField_SortDirection_SORT_DIRECTION_CLUSTERED: default: return Status::NotImplemented( "Acero does not support the specified sort direction: ", dir);

cpp/src/arrow/engine/substrait/relation_internal.cc

vibhatha · 2023-04-11T02:45:01Z

cpp/src/arrow/engine/substrait/serde_test.cc

+  NamedTableProvider table_provider = [&](const std::vector<std::string>& names,
+                                          const Schema&) {
+    std::shared_ptr<acero::ExecNodeOptions> options =
+        std::make_shared<acero::TableSourceNodeOptions>(input_table);
+    return acero::Declaration("table_source", {}, options, "mock_source");
+  };


Instead could we use the following?

Suggested change

NamedTableProvider table_provider = [&](const std::vector<std::string>& names,

const Schema&) {

std::shared_ptr<acero::ExecNodeOptions> options =

std::make_shared<acero::TableSourceNodeOptions>(input_table);

return acero::Declaration("table_source", {}, options, "mock_source");

};

NamedTableProvider table_provider = AlwaysProvideSameTable(std::move(input_table));

Good cleanup, thanks. I've switched to this.

vibhatha

@westonpace the PR looks good to me. I have added a few suggestions.

cpp/src/arrow/engine/substrait/relation_internal.cc

icexelloss

Left a few questions. Mostly looks good.

westonpace · 2023-04-14T21:57:42Z

I'll merge this when green but I don't think there is any significant rush to get this into 12.0.0?

danepitkin · 2023-05-12T17:56:14Z

With Arrow v12 released (🎉 ) you should be all clear to merge at your leisure!

westonpace · 2023-05-15T19:05:13Z

I've rebased just in case and will let CI run one more time.

westonpace · 2023-05-15T19:06:55Z

Also, I see I missed some feedback from @vibhatha so I will try and get to that today.

pitrou

Just a few comments, but I'm not competent enough to comment on the core logic.

pitrou · 2023-05-16T14:42:00Z

cpp/src/arrow/engine/substrait/relation_internal.cc

+namespace {
+
+bool IsSortNullsFirst(const substrait::SortField::SortDirection& direction) {
+  return direction % 2 == 1;


Hmm, rather than arithmetic on enum values, I'd rather see a proper switch/case statement to make code more readable and maintainable. This is not so performance-sensitive that it must be optimized to the latest nanosecond.

I've switched to proper handling of these cases and I return NullPlacement and SortOrder. I agree the code is more readable now.

pitrou · 2023-05-16T14:42:11Z

cpp/src/arrow/engine/substrait/relation_internal.cc

+
+compute::SortOrder SortOrderFromDirection(
+    const substrait::SortField::SortDirection& direction) {
+  if (direction < 3) {


Same comment here.

cpp/src/arrow/engine/substrait/relation_internal.cc

pitrou · 2023-05-16T14:44:09Z

cpp/src/arrow/engine/substrait/relation_internal.cc

+          if ((null_placement == compute::NullPlacement::AtStart &&
+               !IsSortNullsFirst(sort.direction())) ||
+              (null_placement == compute::NullPlacement::AtEnd &&
+               IsSortNullsFirst(sort.direction()))) {


You can change IsSortNullsFirst to return a compute::NullPlacement directly and this will make these lines a bit simpler (just compare the old null placement with the new one).

I've cleaned this up.

… correctly recognizing mixed null placement.

pitrou

+1, thanks for the update @westonpace

ursabot · 2023-05-31T01:39:39Z

Benchmark runs are scheduled for baseline = 2a6848c and contender = fbe0d5f. fbe0d5f is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.18% ⬆️0.0%] test-mac-arm
[Finished ⬇️0.98% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.51% ⬆️0.09%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] fbe0d5f1 ec2-t3-xlarge-us-east-2
[Finished] fbe0d5f1 test-mac-arm
[Finished] fbe0d5f1 ursa-i9-9960x
[Finished] fbe0d5f1 ursa-thinkcentre-m75q
[Finished] 2a6848c8 ec2-t3-xlarge-us-east-2
[Finished] 2a6848c8 test-mac-arm
[Finished] 2a6848c8 ursa-i9-9960x
[Finished] 2a6848c8 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

westonpace marked this pull request as draft March 21, 2023 05:07

github-actions bot added Component: C++ awaiting committer review Awaiting committer review labels Mar 21, 2023

westonpace force-pushed the feature/GH-32763--substrait-fetch-order-by branch from d77339c to 167d4a3 Compare March 21, 2023 05:10

westonpace marked this pull request as ready for review March 21, 2023 05:10

westonpace mentioned this pull request Mar 23, 2023

[C++] Add ordering information to exec batches #32991

Open

westonpace force-pushed the feature/GH-32763--substrait-fetch-order-by branch from 167d4a3 to 87451a6 Compare April 10, 2023 21:32

vibhatha reviewed Apr 11, 2023

View reviewed changes

cpp/src/arrow/engine/substrait/relation_internal.cc Show resolved Hide resolved

vibhatha reviewed Apr 11, 2023

View reviewed changes

vibhatha approved these changes Apr 11, 2023

View reviewed changes

icexelloss reviewed Apr 11, 2023

View reviewed changes

cpp/src/arrow/engine/substrait/relation_internal.cc Outdated Show resolved Hide resolved

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Apr 11, 2023

icexelloss reviewed Apr 11, 2023

View reviewed changes

cpp/src/arrow/engine/substrait/relation_internal.cc Show resolved Hide resolved

icexelloss reviewed Apr 11, 2023

View reviewed changes

westonpace force-pushed the feature/GH-32763--substrait-fetch-order-by branch from 87451a6 to 1eb683a Compare April 14, 2023 21:52

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Apr 14, 2023

westonpace force-pushed the feature/GH-32763--substrait-fetch-order-by branch from 1eb683a to 4c457e8 Compare May 15, 2023 19:04

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels May 15, 2023

pitrou reviewed May 16, 2023

View reviewed changes

westonpace added 3 commits May 18, 2023 14:22

Add substrait support for sort & fetch

d9c6d06

Added test case for mixed null placement. Fixed bug where we were not…

815e0f3

… correctly recognizing mixed null placement.

Simplified the logic as per review comments

1e3b763

westonpace force-pushed the feature/GH-32763--substrait-fetch-order-by branch from 4c457e8 to 1e3b763 Compare May 19, 2023 13:16

github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels May 19, 2023

Remove unreachable break

0293dc9

github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels May 19, 2023

westonpace requested a review from pitrou May 19, 2023 13:39

pitrou approved these changes May 22, 2023

View reviewed changes

pitrou merged commit fbe0d5f into apache:main May 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-32763: [C++] Add FromProto for fetch & sort #34651

GH-32763: [C++] Add FromProto for fetch & sort #34651

westonpace commented Mar 21, 2023 •

edited by github-actions bot

Loading

github-actions bot commented Mar 21, 2023

github-actions bot commented Mar 21, 2023

westonpace commented Mar 22, 2023

amol- commented Apr 5, 2023

westonpace commented Apr 10, 2023

westonpace commented Apr 10, 2023

vibhatha commented Apr 11, 2023

vibhatha Apr 11, 2023

westonpace May 15, 2023

westonpace May 19, 2023 •

edited

Loading

vibhatha Apr 11, 2023

vibhatha Apr 11, 2023

westonpace May 19, 2023

vibhatha Apr 11, 2023

westonpace May 19, 2023

vibhatha left a comment

icexelloss left a comment

westonpace commented Apr 14, 2023

danepitkin commented May 12, 2023

westonpace commented May 15, 2023

westonpace commented May 15, 2023

pitrou left a comment

pitrou May 16, 2023

westonpace May 19, 2023

pitrou May 16, 2023

westonpace May 19, 2023

pitrou May 16, 2023

westonpace May 19, 2023

pitrou left a comment

ursabot commented May 31, 2023

GH-32763: [C++] Add FromProto for fetch & sort #34651

GH-32763: [C++] Add FromProto for fetch & sort #34651

Conversation

westonpace commented Mar 21, 2023 • edited by github-actions bot Loading

github-actions bot commented Mar 21, 2023

github-actions bot commented Mar 21, 2023

westonpace commented Mar 22, 2023

amol- commented Apr 5, 2023

westonpace commented Apr 10, 2023

westonpace commented Apr 10, 2023

vibhatha commented Apr 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonpace May 19, 2023 • edited Loading

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

vibhatha left a comment

Choose a reason for hiding this comment

icexelloss left a comment

Choose a reason for hiding this comment

westonpace commented Apr 14, 2023

danepitkin commented May 12, 2023

westonpace commented May 15, 2023

westonpace commented May 15, 2023

pitrou 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitrou left a comment

Choose a reason for hiding this comment

ursabot commented May 31, 2023

westonpace commented Mar 21, 2023 •

edited by github-actions bot

Loading

westonpace May 19, 2023 •

edited

Loading