JSON tree algorithms refactor I: CSR data structure for column tree #15979

shrshi · 2024-06-11T20:15:30Z

Description

Part of #15903.

Introduces the Compressed Sparse Row (CSR) format to store the adjacency information of the column tree.
Analogous to reduce_to_column_tree, reduce_to_column_tree_csr reduces node tree representation to column tree stored in CSR format.

TODO:

Correctness test

Checklist

I am familiar with the Contributing Guidelines.
New or existing tests cover these changes.
The documentation is up to date with these changes.

ttnghia · 2024-06-25T17:48:18Z

Question: Is this ready for review? Or still WIP?

shrshi · 2024-06-25T18:12:34Z

Question: Is this ready for review? Or still WIP?

Sorry, forgot to update the title. It's ready for review.

…-tree-refactor

copy-pr-bot · 2024-06-28T17:21:02Z

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

shrshi · 2024-06-28T17:31:47Z

/ok to test

shrshi · 2024-09-06T23:28:11Z

/ok to test

revans2

This looks fine. I didn't see any test or performance regressions or improvements with this patch.

vuule · 2024-09-10T05:47:58Z

This looks fine. I didn't see any test or performance regressions or improvements with this patch.

AFAIK the new path is not (yet) use by default.

vuule

Last batch, nothing major :)

vuule · 2024-09-10T05:52:26Z

cpp/src/io/json/nested_json.hpp

+};
+
+/*
+ * @brief Unvalidated column tree stored in Compressed Sparse Row (CSR) format. The device json


Suggested change

* @brief Unvalidated column tree stored in Compressed Sparse Row (CSR) format. The device json

* @brief Invalidated column tree stored in Compressed Sparse Row (CSR) format. The device json

vuule · 2024-09-10T05:58:40Z

cpp/src/io/json/nested_json.hpp

+  // position of nnzs
+  csr adjacency;


I think the comment got separated from the data members it documents.
Also, I would not mind the full name of whatever nnz is :) (I assume something like non-zero)

Suggested change

// position of nnzs

csr adjacency;

csr adjacency;

// positions of nnzs

vuule · 2024-09-10T06:07:40Z

cpp/src/io/json/nested_json.hpp

+  rmm::device_uvector<NodeIndexT> rowidx;
+  rmm::device_uvector<NodeIndexT> colidx;


looks like these were left by accident, as they're unused.

Suggested change

rmm::device_uvector<NodeIndexT> rowidx;

rmm::device_uvector<NodeIndexT> colidx;

vuule · 2024-09-10T18:56:17Z

cpp/src/io/json/column_tree_construction.cu

+          typename OutputIterator1,
+          typename OutputIterator2>
+void max_row_offsets_col_categories(InputIterator1 keys_first,
+                                    InputIterator1 keys_last,


is this actually last? I expect it to be one past last, so we should name these begin/end.

vuule · 2024-09-10T19:22:20Z

cpp/src/io/json/column_tree_construction.cu

+                          } else
+                            ctg = NC_ERR;


Suggested change

} else

ctg = NC_ERR;

} else {

ctg = NC_ERR;

}

vuule · 2024-09-10T19:30:49Z

cpp/src/io/json/column_tree_construction.cu

+                          // *+*=*, v+v=v
+                          if (type_a == type_b) {
+                            ctg = type_a;
+                          } else if (is_a_leaf) {


the function is seemingly not commutative w.r.t. is_a_leaf/is_b_leaf. We check is_a_leaf first, so I'm not sure if we're getting the right result when both is_a_leaf and is_b_leaf are true.

vuule · 2024-09-10T22:32:01Z

cpp/src/io/json/column_tree_construction.cu

+  auto const num_columns = thrust::unique_count(
+    rmm::exec_policy_nosync(stream), level_ordered_col_ids.begin(), level_ordered_col_ids.end());
+  rmm::device_uvector<NodeIndexT> level_ordered_unique_node_ids(num_columns, stream);
+  rmm::device_uvector<NodeIndexT> mapped_col_ids(num_columns, stream);


there are several vectors in this function that live way past their last use (e.g. mapped_col_ids_copy, level_ordered_unique_node_ids, level_ordered_unique_node_ids). We can reset them, but it's probably cleaner to break the function into smaller ones and reduce the lifetime "organically".

vuule · 2024-09-10T23:45:02Z

cpp/src/io/json/column_tree_construction.cu

+    thrust::make_discard_iterator(),
+    thrust::make_zip_iterator(max_row_offsets.begin(), column_categories.begin()),
+    stream);
+  // 4. construct parent_col_ids using permutation iterator


Suggested change

// 4. construct parent_col_ids using permutation iterator

// 4. construct parent_col_ids using permutation iterator

vuule · 2024-09-10T23:47:35Z

cpp/src/io/json/column_tree_construction.cu

+                      parent_col_ids = parent_col_ids.begin()] __device__(auto i) {
+                       auto parent_col_id = parent_col_ids[i];
+                       if (parent_col_id == 0)
+                         map[i - 1]--;


Suggested change

map[i - 1]--;

--map[i - 1];

ttnghia · 2024-09-16T21:46:51Z

cpp/src/io/json/nested_json.hpp

+/*
+ * @brief Sparse graph adjacency matrix stored in Compressed Sparse Row (CSR) format.
+ */
+struct csr {


IMO this abbreviation is very confusing and cryptic. It is better if we call by full name like:

Suggested change

struct csr {

struct compress_sparse_row {

ttnghia · 2024-09-16T21:47:12Z

cpp/src/io/json/nested_json.hpp

+  rmm::device_uvector<NodeIndexT> rowidx;
+  rmm::device_uvector<NodeIndexT> colidx;


Suggested change

rmm::device_uvector<NodeIndexT> rowidx;

rmm::device_uvector<NodeIndexT> colidx;

rmm::device_uvector<NodeIndexT> row_idx;

rmm::device_uvector<NodeIndexT> col_idx;

ttnghia · 2024-09-16T21:47:33Z

cpp/src/io/json/nested_json.hpp

+ * annotated with information required to construct cuDF columns.
+ */
+struct column_tree {
+  // position of nnzs


What is nnzs?

ttnghia · 2024-09-16T21:49:30Z

cpp/src/io/json/nested_json.hpp

+  bool is_array_of_arrays,
+  NodeIndexT const row_array_parent_col_id,


Please use const consistently.

Suggested change

bool is_array_of_arrays,

NodeIndexT const row_array_parent_col_id,

bool const is_array_of_arrays,

NodeIndexT const row_array_parent_col_id,

I would recommend not using const for parameter types unless they are pass by reference. The const provides no help to the caller since the pass-by-copy ensures the parameter value is not changed from the caller's perspective. It also limits the implementation which may now require a separate variable (register) to manage a value that otherwise would not be const.

Thanks for sharing the insight @davidwendt . +1 on the above comment.
I do have one doubt: if the pass by value argument is const, will it help compiler to do optimizations better? (or make compiler work easier(faster) because it already knows it's const).

The const hint to the compiler is a bit outdated. The compiler usually knows when a variable value is not changed and automatically provides optimizations regardless of the const decorator. So I'd rather make the interface clear without hampering the implementation details.

There's a middle ground I've heard of: avoid const by-value in the definition (visible to user), but use const in the definition to prevent the parameter value changing somewhere in the code.
I personally don't use const here, but can understand the (mild) usefulness in the definition.

It also limits the implementation which may now require a separate variable (register) to manage a value that otherwise would not be const.

@davidwendt could you explain this, I'm not following?

Just that the caller does not need to know your implementation details regarding the parameter. If you want to use the parameter as non-const in your implementation that should be up to you.
Using const in the definition and not in the declaration seems like a hack (to me) though I know it is supported by the C++ spec.

ttnghia · 2024-09-16T21:49:58Z

cpp/src/io/json/nested_json.hpp

+                      bool is_array_of_arrays,
+                      NodeIndexT const row_array_parent_col_id,


Similarly, use const consistently.

Suggested change

bool is_array_of_arrays,

NodeIndexT const row_array_parent_col_id,

bool const is_array_of_arrays,

NodeIndexT const row_array_parent_col_id,

ttnghia · 2024-09-16T21:52:33Z

cpp/src/io/json/column_tree_construction.cu

+template <typename T>
+void print(device_span<T const> d_vec, std::string name, rmm::cuda_stream_view stream)


Function for debugging should not be compiled by default. Typically, they are wrapped in a macro such as:

#ifdef JSON_DEBUG template <typename T> void print(device_span<T const> d_vec, std::string name, rmm::cuda_stream_view stream) {...} #endif // JSON_DEBUG

ttnghia · 2024-09-16T21:53:30Z

cpp/src/io/json/column_tree_construction.cu

+  device_span<TreeDepthT> node_levels;
+  device_span<NodeIndexT const> col_ids;
+  device_span<NodeIndexT const> parent_node_ids;


Again, use const consistenly.

Suggested change

device_span<TreeDepthT> node_levels;

device_span<NodeIndexT const> col_ids;

device_span<NodeIndexT const> parent_node_ids;

device_span<TreeDepthT const> node_levels;

device_span<NodeIndexT const> col_ids;

device_span<NodeIndexT const> parent_node_ids;

ttnghia · 2024-09-16T21:53:58Z

cpp/src/io/json/column_tree_construction.cu

+
+struct parent_nodeids_to_colids {
+  device_span<NodeIndexT const> col_ids;
+  device_span<NodeIndexT> rev_mapped_col_ids;


Suggested change

device_span<NodeIndexT> rev_mapped_col_ids;

device_span<NodeIndexT const> rev_mapped_col_ids;

ttnghia · 2024-09-16T21:54:53Z

cpp/src/io/json/column_tree_construction.cu

+  bool is_array_of_arrays,
+  NodeIndexT const row_array_parent_col_id,


Suggested change

bool is_array_of_arrays,

NodeIndexT const row_array_parent_col_id,

bool const is_array_of_arrays,

NodeIndexT const row_array_parent_col_id,

ttnghia · 2024-09-16T21:55:19Z

cpp/src/io/json/column_tree_construction.cu

+  /*
+  print<NodeIndexT>(level_ordered_node_ids, "h_level_ordered_node_ids", stream);
+  print<NodeIndexT>(col_ids, "h_col_ids", stream);
+  print<NodeIndexT>(level_ordered_col_ids, "h_level_ordered_col_ids", stream);
+  */


Suggested change

/*

print<NodeIndexT>(level_ordered_node_ids, "h_level_ordered_node_ids", stream);

print<NodeIndexT>(col_ids, "h_col_ids", stream);

print<NodeIndexT>(level_ordered_col_ids, "h_level_ordered_col_ids", stream);

*/

#ifdef JSON_DEBUG

print<NodeIndexT>(level_ordered_node_ids, "h_level_ordered_node_ids", stream);

print<NodeIndexT>(col_ids, "h_col_ids", stream);

print<NodeIndexT>(level_ordered_col_ids, "h_level_ordered_col_ids", stream);

#ifdef // JSON_DEBUG

ttnghia · 2024-09-16T21:56:28Z

cpp/src/io/json/column_tree_construction.cu

+  print<NodeIndexT>(mapped_col_ids, "h_mapped_col_ids", stream);
+  print<NodeIndexT>(level_ordered_unique_node_ids, "h_level_ordered_unique_node_ids", stream);
+  print<NodeIndexT>(rev_mapped_col_ids, "h_rev_mapped_col_ids", stream);


Suggested change

print<NodeIndexT>(mapped_col_ids, "h_mapped_col_ids", stream);

print<NodeIndexT>(level_ordered_unique_node_ids, "h_level_ordered_unique_node_ids", stream);

print<NodeIndexT>(rev_mapped_col_ids, "h_rev_mapped_col_ids", stream);

#ifdef JSON_DEBUG

print<NodeIndexT>(mapped_col_ids, "h_mapped_col_ids", stream);

print<NodeIndexT>(level_ordered_unique_node_ids, "h_level_ordered_unique_node_ids", stream);

print<NodeIndexT>(rev_mapped_col_ids, "h_rev_mapped_col_ids", stream);

#endif // JSON_DEBUG

ttnghia · 2024-09-16T21:57:27Z

cpp/src/io/json/column_tree_construction.cu

+  // Note that the first element of csr_parent_col_ids is -1 (parent_node_sentinel)
+  // children adjacency
+
+  print<NodeIndexT>(parent_col_ids, "h_parent_col_ids", stream);


Suggested change

print<NodeIndexT>(parent_col_ids, "h_parent_col_ids", stream);

#ifdef JSON_DEBUG

print<NodeIndexT>(parent_col_ids, "h_parent_col_ids", stream);

#endif // JSON_DEBUG

ttnghia · 2024-09-16T21:57:55Z

cpp/src/io/json/column_tree_construction.cu

+                  non_leaf_nodes.begin(),
+                  rowidx.begin() + 1);
+
+  print<NodeIndexT>(rowidx, "h_rowidx", stream);


Wrap in JSON_DEBUG.

ttnghia · 2024-09-16T21:58:11Z

cpp/src/io/json/column_tree_construction.cu

+    }),
+    thrust::plus<NodeIndexT>{});
+
+  print<NodeIndexT>(rowidx, "h_rowidx", stream);


Wrap in JSON_DEBUG.

ttnghia · 2024-09-16T21:58:23Z

cpp/src/io/json/column_tree_construction.cu

+                  map.begin(),
+                  colidx.begin());
+
+  print<size_type>(max_row_offsets, "h_max_row_offsets", stream);


Wrap in JSON_DEBUG.

ttnghia · 2024-09-16T22:00:48Z

cpp/src/io/json/column_tree_construction.cu

+  {
+    auto max_row_offsets_it =


I like the idea of wrapping code in multiple local blocks so the temp arrays can be freed as soon as possible. If possible, please consider applying this throughout this PR.

shrshi · 2024-09-17T18:50:14Z

/ok to test

shrshi added 2 commits June 11, 2024 19:40

added csr data struct

1ec9617

formatting

022d7ce

github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Jun 11, 2024

shrshi added cuIO cuIO issue improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jun 11, 2024

shrshi added 2 commits June 25, 2024 00:55

added test

382633f

formatting

1823854

github-actions bot added the CMake CMake build issue label Jun 25, 2024

shrshi marked this pull request as ready for review June 25, 2024 01:00

shrshi requested a review from a team as a code owner June 25, 2024 01:00

shrshi requested review from zpuller, davidwendt, karthikeyann and vuule June 25, 2024 01:00

GregoryKimball requested a review from ttnghia June 25, 2024 17:34

shrshi changed the title ~~[WIP] JSON tree algorithms refactor I: CSR data structure for column tree~~ JSON tree algorithms refactor I: CSR data structure for column tree Jun 25, 2024

shrshi added 7 commits June 25, 2024 22:32

Merge branch 'branch-24.08' into json-tree-refactor-ii

4a7e2a5

Merge branch 'branch-24.08' into json-tree-refactor

8d5ddfb

fixing csr construction

84a7749

moving the csr algorithms

810c389

formatting

6a1a415

Merge branch 'branch-24.08' into json-tree-refactor

85c197d

Merge branch 'json-tree-refactor' of github.com:shrshi/cudf into json…

996c6dd

…-tree-refactor

shrshi requested a review from a team as a code owner June 28, 2024 17:21

shrshi mentioned this pull request Jul 6, 2024

[WIP] JSON tree algorithms refactor II: Constructing device JSON column #16205

Draft

8 tasks

shrshi requested a review from vuule September 6, 2024 23:23

revans2 approved these changes Sep 9, 2024

View reviewed changes

vuule reviewed Sep 10, 2024

View reviewed changes

shrshi added 3 commits September 11, 2024 17:13

Merge branch 'branch-24.10' into json-tree-refactor

63eec8a

adding more tests; debugging on the way

5f4aca6

formatting

e6a9941

ttnghia reviewed Sep 16, 2024

View reviewed changes

shrshi added 2 commits September 17, 2024 18:46

added more tests; fixed bugs

82c9ebe

formatting

8dd6877

shrshi added 3 commits September 18, 2024 22:08

finally tests passing

0c63f22

fixed all bugs hopefully

2d4861e

formatting

7759a91

	* @brief Unvalidated column tree stored in Compressed Sparse Row (CSR) format. The device json
	* @brief Invalidated column tree stored in Compressed Sparse Row (CSR) format. The device json

		rmm::device_uvector<NodeIndexT> rowidx;
		rmm::device_uvector<NodeIndexT> colidx;

	// 4. construct parent_col_ids using permutation iterator

	// 4. construct parent_col_ids using permutation iterator

		bool is_array_of_arrays,
		NodeIndexT const row_array_parent_col_id,

		template <typename T>
		void print(device_span<T const> d_vec, std::string name, rmm::cuda_stream_view stream)

	device_span<NodeIndexT> rev_mapped_col_ids;
	device_span<NodeIndexT const> rev_mapped_col_ids;

JSON tree algorithms refactor I: CSR data structure for column tree #15979

Are you sure you want to change the base?

JSON tree algorithms refactor I: CSR data structure for column tree #15979

Conversation

shrshi commented Jun 11, 2024 • edited Loading

Description

Checklist

ttnghia commented Jun 25, 2024

shrshi commented Jun 25, 2024

copy-pr-bot bot commented Jun 28, 2024

shrshi commented Jun 28, 2024

shrshi commented Sep 6, 2024

revans2 left a comment

Choose a reason for hiding this comment

vuule commented Sep 10, 2024

vuule 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

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

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

ttnghia Sep 16, 2024 • 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

ttnghia Sep 16, 2024 • 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

shrshi commented Sep 17, 2024

shrshi commented Jun 11, 2024 •

edited

Loading

ttnghia Sep 16, 2024 •

edited

Loading

ttnghia Sep 16, 2024 •

edited

Loading