You are viewing a plain text version of this content. The canonical link for it is here.

Posted to github@arrow.apache.org by "felipecrv (via GitHub)" <gi...@apache.org> on 2023/04/14 13:05:15 UTC

[GitHub] [arrow] felipecrv opened a new pull request, #35129: GH-35059: [C++] Fix "hash_count" for run-end encoded inputs

felipecrv opened a new pull request, #35129:
URL: https://github.com/apache/arrow/pull/35129

   ### Rationale for this change
   
   Fixing a bug.
   
   ### What changes are included in this PR?
   
   Changes to the `"hash_count"` kernel implementation to handle REE and union arrays correctly.
   
   - [x] Generic (potentially slow) implementation
   - [ ] REE-specialized implementation
   
   ### Are these changes tested?
   
   Yes, by modifying the existing unit tests.
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] ursabot commented on pull request #35129: GH-35059: [C++] Fix "hash_count" for run-end encoded inputs

Posted by "ursabot (via GitHub)" <gi...@apache.org>.

ursabot commented on PR #35129:
URL: https://github.com/apache/arrow/pull/35129#issuecomment-1530811382

   ['Python', 'R'] benchmarks have high level of regressions.
   [test-mac-arm](https://conbench.ursa.dev/compare/runs/6331adfa453343f1ab3db9d2d59c4f96...e33671654caa403daaf24724a74617bd/)
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] westonpace commented on a diff in pull request #35129: GH-35059: [C++] Fix "hash_count" for run-end encoded inputs

Posted by "westonpace (via GitHub)" <gi...@apache.org>.

westonpace commented on code in PR #35129:
URL: https://github.com/apache/arrow/pull/35129#discussion_r1179605685


##########
cpp/src/arrow/testing/gtest_util.cc:
##########
@@ -427,6 +428,24 @@ std::shared_ptr<Table> TableFromJSON(const std::shared_ptr<Schema>& schema,
   return *Table::FromRecordBatches(schema, std::move(batches));
 }
 
+Result<std::shared_ptr<Table>> RunEndEncodeTableColumns(
+    const Table& table, const std::vector<int>& column_indices) {
+  const int num_columns = table.num_columns();
+  std::vector<std::shared_ptr<ChunkedArray>> encoded_columns;
+  encoded_columns.reserve(num_columns);
+  for (int i = 0; i < num_columns; i++) {
+    if (std::find(column_indices.begin(), column_indices.end(), i) !=
+        column_indices.end()) {
+      EXPECT_OK_AND_ASSIGN(auto run_end_encoded, compute::RunEndEncode(table.column(i)));
+      EXPECT_EQ(run_end_encoded.kind(), Datum::CHUNKED_ARRAY);

Review Comment:
   Given you're returning a result anyways you could just use ARROW_ASSIGN_OR_RAISE and ARROW_RETURN_NOT_OK here right?



##########
cpp/src/arrow/compute/kernels/hash_aggregate.cc:
##########
@@ -305,6 +306,56 @@ struct GroupedCountImpl : public GroupedAggregator {
     return Status::OK();
   }
 
+  template <bool count_valid>
+  struct RunEndEncodedCountImpl {
+    /// Count the number of valid or invalid values in a run-end-encoded array.
+    ///
+    /// \param[in] input the run-end-encoded array
+    /// \param[out] counts the counts being accumulated
+    /// \param[in] g the group ids of the values in the array
+    template <typename RunEndCType>
+    void DoCount(const ArraySpan& input, int64_t* counts, const uint32_t* g) {
+      ree_util::RunEndEncodedArraySpan<RunEndCType> ree_span(input);
+      const auto* physical_validity = ree_util::ValuesArray(input).GetValues<uint8_t>(0);
+      auto end = ree_span.end();
+      for (auto it = ree_span.begin(); it != end; ++it) {
+        const bool is_valid = bit_util::GetBit(physical_validity, it.index_into_array());
+        if constexpr (count_valid) {
+          if (is_valid) {
+            for (int64_t i = 0; i < it.run_length(); ++i, ++g) {
+              counts[*g] += 1;
+            }
+          } else {
+            g += it.run_length();
+          }
+        } else {
+          if (!is_valid) {
+            for (int64_t i = 0; i < it.run_length(); ++i, ++g) {
+              counts[*g] += 1;
+            }
+          } else {
+            g += it.run_length();
+          }
+        }

Review Comment:
   Can you collapse these two branches?
   
   ```
   if (is_valid == count_valid) {
     ...
   }
   ```
   
   Or does that not somehow generate the same benefit?  Seems like either way there is a single if comparison.  I expect the compiler would generate the same for `if (x == true)` that it does for `if (x)`.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org