Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix struct row comparator's exception on empty structs #10604

Merged
merged 4 commits into from
Apr 8, 2022
Merged

Fix struct row comparator's exception on empty structs #10604

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
10b7b51
fix the comparison of empty struct
sperlingxx Apr 6, 2022
3b02124
update
sperlingxx Apr 7, 2022
0ee82bc
Change decomposition logic to support empty structs
devavret Apr 7, 2022
c2176c2
fix typo
sperlingxx Apr 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cpp/benchmarks/sort/sort_structs.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,5 +80,5 @@ void nvbench_sort_struct(nvbench::state& state)
NVBENCH_BENCH(nvbench_sort_struct)
.set_name("sort_struct")
.add_int64_power_of_two_axis("NumRows", {10, 18, 26})
.add_int64_axis("Depth", {1, 8})
.add_int64_axis("Depth", {0, 1, 8})
.add_int64_axis("Nulls", {0, 1});
6 changes: 5 additions & 1 deletion cpp/include/cudf/table/experimental/row_operators.cuh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,11 @@ class device_row_comparator {
return cuda::std::make_pair(state, depth);
}

// Structs have been modified to only have 1 child when using this.
if (lcol.num_child_columns() == 0) {
return cuda::std::make_pair(weak_ordering::EQUIVALENT, depth);
}
Comment on lines +200 to +202
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@devavret I assume there is some check earlier on in hose code that verifies corresponding struct columns have the same number of children?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. If it reaches here, they're the same structure.

Copy link
Contributor Author

@sperlingxx sperlingxx Apr 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know there could be empty structs. Does this work if a struct contains 2 struct children, the first of which is empty?

It does work. I added a test case to cover this scenario. IIUC, following the rule of decompose_structs, the second child will be detached from the root struct.

   S0
   / \
  S1  S2

=>

S0
|
S1   S2

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation of decompose_structs itself was broken because it operated under the assumption that struct has at least 1 child. If S1 was empty then it would create

S0
|
S1
|
S2

I fixed the impl so this is good to go.


// Non-empty structs have been modified to only have 1 child when using this.
lcol = lcol.children()[0];
rcol = rcol.children()[0];
++depth;
Expand Down
54 changes: 54 additions & 0 deletions cpp/tests/sort/sort_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -804,6 +804,60 @@ TYPED_TEST(FixedPointTestAllReps, FixedPointSortedOrderGather)
CUDF_TEST_EXPECT_TABLES_EQUAL(sorted_table, sorted->view());
}

struct SortCornerTest : public BaseFixture {
};

TEST_F(SortCornerTest, WithEmptyStructColumn)
{
using int_col = fixed_width_column_wrapper<int32_t>;

// struct{}, int, int
int_col col_for_mask{{0, 0, 0, 0, 0, 0}, {1, 0, 1, 1, 1, 1}};
auto null_mask = cudf::copy_bitmask(col_for_mask.release()->view());
auto struct_col = cudf::make_structs_column(6, {}, UNKNOWN_NULL_COUNT, std::move(null_mask));

int_col col1{{1, 2, 3, 1, 2, 3}};
int_col col2{{1, 1, 1, 2, 2, 2}};
table_view input{{struct_col->view(), col1, col2}};

int_col expected{{1, 0, 3, 4, 2, 5}};
std::vector<order> column_order{order::ASCENDING, order::ASCENDING, order::ASCENDING};
auto got = sorted_order(input, column_order);
CUDF_TEST_EXPECT_COLUMNS_EQUAL(expected, got->view());

// struct{struct{}, int}
int_col col3{{0, 1, 2, 3, 4, 5}};
std::vector<std::unique_ptr<cudf::column>> child_columns;
child_columns.push_back(std::move(struct_col));
child_columns.push_back(col3.release());
auto struct_col2 =
cudf::make_structs_column(6, std::move(child_columns), 0, rmm::device_buffer{});
table_view input2{{struct_col2->view()}};

int_col expected2{{5, 4, 3, 2, 0, 1}};
auto got2 = sorted_order(input2, {order::DESCENDING});
CUDF_TEST_EXPECT_COLUMNS_EQUAL(expected2, got2->view());

// struct{struct{}, struct{int}}
int_col col_for_mask2{{0, 0, 0, 0, 0, 0}, {1, 0, 1, 1, 0, 1}};
auto null_mask2 = cudf::copy_bitmask(col_for_mask2.release()->view());
std::vector<std::unique_ptr<cudf::column>> child_columns2;
auto child_col_1 = cudf::make_structs_column(6, {}, UNKNOWN_NULL_COUNT, std::move(null_mask2));
child_columns2.push_back(std::move(child_col_1));
int_col col4{{0, 1, 2, 3, 4, 5}};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverse this and the answer will not be the one you expect. You'd expect 415320 but you'd get 145320.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed 0ee82bc

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the TL;DR on the change made in #0ee82bc?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than flattening the struct and then traversing backwards to create branches, we now create branches while traversing. In the earlier method we depended on finding a leaf column to know when to break off the branch.

std::vector<std::unique_ptr<cudf::column>> grand_child;
grand_child.push_back(std::move(col4.release()));
auto child_col_2 = cudf::make_structs_column(6, std::move(grand_child), 0, rmm::device_buffer{});
child_columns2.push_back(std::move(child_col_2));
auto struct_col3 =
cudf::make_structs_column(6, std::move(child_columns2), 0, rmm::device_buffer{});
table_view input3{{struct_col3->view()}};

int_col expected3{{1, 4, 0, 2, 3, 5}};
auto got3 = sorted_order(input3, {order::ASCENDING});
CUDF_TEST_EXPECT_COLUMNS_EQUAL(expected3, got3->view());
};

} // namespace test
} // namespace cudf

Expand Down