You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/07/15 23:00:02 UTC

[GitHub] [arrow] drin commented on a diff in pull request #13518: ARROW-16700: [C++][R][Datasets] aggregates on partitioning columns

drin commented on code in PR #13518:
URL: https://github.com/apache/arrow/pull/13518#discussion_r922566935


##########
cpp/src/arrow/dataset/scanner_test.cc:
##########
@@ -1445,9 +1446,13 @@ DatasetAndBatches MakeBasicDataset() {
           equal(field_ref("c"), literal(23)),
           equal(field_ref("c"), literal(47)),
       },
-      [](compute::ExecBatch* batch, const RecordBatch&) {
+      [](compute::ExecBatch* batch, const RecordBatch&, size_t guarantee_index) {
         // a placeholder will be inserted for partition field "c"
-        batch->values.emplace_back(std::make_shared<Int32Scalar>());
+        if (guarantee_index == 0) {
+          batch->values.emplace_back(std::make_shared<Int32Scalar>(23));
+        } else {
+          batch->values.emplace_back(std::make_shared<Int32Scalar>(47));
+        }

Review Comment:
   I address this in .
   
   I essentially change the invocation of the lambda to:
   ```cpp
   if (not guarantees.empty()) {
     auto extract_result = ExtractKnownFieldValues(guarantees[frag_ndx]);
     ARROW_WARN_NOT_OK(extract_result.status(), "ExtractKnownFieldValues failed");
     for (const auto& known_field : extract_result->map) {
       batches.back().values.emplace_back(known_field.second);
     }
   }
   ```
   
   ooh, but I just realized I used `not` instead of `!`. I'm not sure if there's a preference



-- 
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