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/06/08 17:05:19 UTC
[GitHub] [arrow] vibhatha opened a new pull request, #13344: [DO NOT MERGE] ARROW-16686: [C++] Use unique_ptr with FunctionOptions [WIP]
vibhatha opened a new pull request, #13344:
URL: https://github.com/apache/arrow/pull/13344
WIP: do not review.
--
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] kou commented on pull request #13344: ARROW-16686: [C++] Use shared_ptr with FunctionOptions
Posted by GitBox <gi...@apache.org>.
kou commented on PR #13344:
URL: https://github.com/apache/arrow/pull/13344#issuecomment-1153170673
Done.
--
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] vibhatha commented on pull request #13344: ARROW-16686: [C++] Use shared_ptr with FunctionOptions
Posted by GitBox <gi...@apache.org>.
vibhatha commented on PR #13344:
URL: https://github.com/apache/arrow/pull/13344#issuecomment-1157472191
> I think that you got conflicts when you run `git rebase apache/master`. I think that you run `git rebase --skip` for these conflicts. It dropped my commits.
I ran `git rebase --continue` after adding the modified files. Sorry about that.
--
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] lidavidm commented on a diff in pull request #13344: ARROW-16686: [C++] Use shared_ptr with FunctionOptions
Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #13344:
URL: https://github.com/apache/arrow/pull/13344#discussion_r899044135
##########
cpp/src/arrow/compute/api_aggregate.h:
##########
@@ -401,7 +401,7 @@ struct ARROW_EXPORT Aggregate {
std::string function;
/// options for the aggregation function
- const FunctionOptions* options;
+ std::shared_ptr<FunctionOptions> options;
Review Comment:
Ah, I didn't see that.
Hmm, I suppose it's not much of a difference here.
--
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] github-actions[bot] commented on pull request #13344: ARROW-16686: [C++] Use unique_ptr with FunctionOptions
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #13344:
URL: https://github.com/apache/arrow/pull/13344#issuecomment-1152420398
https://issues.apache.org/jira/browse/ARROW-16686
--
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] github-actions[bot] commented on pull request #13344: [DO NOT MERGE] ARROW-16686: [C++] Use unique_ptr with FunctionOptions
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #13344:
URL: https://github.com/apache/arrow/pull/13344#issuecomment-1150174306
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
Thanks for opening a pull request!
If this is not a [minor PR](https://github.com/apache/arrow/blob/master/CONTRIBUTING.md#Minor-Fixes). Could you open an issue for this pull request on JIRA? https://issues.apache.org/jira/browse/ARROW
Opening JIRAs ahead of time contributes to the [Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project.
Then could you also rename pull request title in the following format?
ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}
or
MINOR: [${COMPONENT}] ${SUMMARY}
See also:
* [Other pull requests](https://github.com/apache/arrow/pulls/)
* [Contribution Guidelines - How to contribute patches](https://arrow.apache.org/docs/developers/contributing.html#how-to-contribute-patches)
--
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] vibhatha commented on a diff in pull request #13344: ARROW-16686: [C++] Use shared_ptr with FunctionOptions
Posted by GitBox <gi...@apache.org>.
vibhatha commented on code in PR #13344:
URL: https://github.com/apache/arrow/pull/13344#discussion_r899656477
##########
cpp/src/arrow/compute/exec/plan_test.cc:
##########
@@ -390,7 +391,8 @@ TEST(ExecPlan, ToString) {
}}},
{"aggregate",
AggregateNodeOptions{
- /*aggregates=*/{{"hash_sum", nullptr}, {"hash_count", &options}},
+ /*aggregates=*/{{"hash_sum", nullptr},
+ {"hash_count", std::move(options)}},
Review Comment:
Good catch.
--
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 #13344: ARROW-16686: [C++] Use shared_ptr with FunctionOptions
Posted by GitBox <gi...@apache.org>.
westonpace commented on code in PR #13344:
URL: https://github.com/apache/arrow/pull/13344#discussion_r899640401
##########
cpp/src/arrow/compute/exec/plan_test.cc:
##########
@@ -390,7 +391,8 @@ TEST(ExecPlan, ToString) {
}}},
{"aggregate",
AggregateNodeOptions{
- /*aggregates=*/{{"hash_sum", nullptr}, {"hash_count", &options}},
+ /*aggregates=*/{{"hash_sum", nullptr},
+ {"hash_count", std::move(options)}},
Review Comment:
You use `options` again on line 437 so I'm not sure it is safe to move it here.
##########
cpp/src/arrow/compute/exec/aggregate.cc:
##########
@@ -61,7 +61,11 @@ Result<std::vector<std::unique_ptr<KernelState>>> InitKernels(
// use known default options for the named function if possible
auto maybe_function = ctx->func_registry()->GetFunction(aggregates[i].function);
if (maybe_function.ok()) {
- options = maybe_function.ValueOrDie()->default_options();
+ const FunctionOptions* default_opts =
Review Comment:
I think if you change line 58 to...
```
const FunctionOptions* options = aggregates[i].options.get();
```
...then this change isn't needed. We are just grabbing a read-only view of the options which we can safely discard after the call to `init`
##########
cpp/examples/arrow/execution_plan_documentation_examples.cc:
##########
@@ -539,9 +539,9 @@ arrow::Status SourceGroupAggregateSinkExample(cp::ExecContext& exec_context) {
ARROW_ASSIGN_OR_RAISE(cp::ExecNode * source,
cp::MakeExecNode("source", plan.get(), {}, source_node_options));
- cp::CountOptions options(cp::CountOptions::ONLY_VALID);
+ auto options = std::make_shared<cp::CountOptions>(cp::CountOptions::ONLY_VALID);
auto aggregate_options =
- cp::AggregateNodeOptions{/*aggregates=*/{{"hash_count", &options}},
+ cp::AggregateNodeOptions{/*aggregates=*/{{"hash_count", options}},
Review Comment:
```suggestion
cp::AggregateNodeOptions{/*aggregates=*/{{"hash_count", std::move(options)}},
```
Minor nit
--
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] vibhatha commented on a diff in pull request #13344: ARROW-16686: [C++] Use shared_ptr with FunctionOptions
Posted by GitBox <gi...@apache.org>.
vibhatha commented on code in PR #13344:
URL: https://github.com/apache/arrow/pull/13344#discussion_r899043400
##########
cpp/src/arrow/compute/exec/aggregate.cc:
##########
@@ -61,7 +61,11 @@ Result<std::vector<std::unique_ptr<KernelState>>> InitKernels(
// use known default options for the named function if possible
auto maybe_function = ctx->func_registry()->GetFunction(aggregates[i].function);
if (maybe_function.ok()) {
- options = maybe_function.ValueOrDie()->default_options();
+ auto default_opts = maybe_function.ValueOrDie()->default_options();
+ // only update the options if the default options are set
+ if (default_opts != nullptr) {
+ options = default_opts->Copy();
+ }
Review Comment:
Good one, I forgot the recent discussion on auto.
--
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] vibhatha commented on pull request #13344: ARROW-16686: [C++] Use shared_ptr with FunctionOptions
Posted by GitBox <gi...@apache.org>.
vibhatha commented on PR #13344:
URL: https://github.com/apache/arrow/pull/13344#issuecomment-1153054825
cc @nealrichardson made some changes to R library, appreciate your feedback.
--
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] kou commented on pull request #13344: ARROW-16686: [C++] Use shared_ptr with FunctionOptions
Posted by GitBox <gi...@apache.org>.
kou commented on PR #13344:
URL: https://github.com/apache/arrow/pull/13344#issuecomment-1157284182
I"ve reapplied my patches.
--
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] vibhatha commented on a diff in pull request #13344: ARROW-16686: [C++] Use shared_ptr with FunctionOptions
Posted by GitBox <gi...@apache.org>.
vibhatha commented on code in PR #13344:
URL: https://github.com/apache/arrow/pull/13344#discussion_r899044178
##########
python/pyarrow/_compute.pyx:
##########
@@ -2067,16 +2067,18 @@ def _group_by(args, keys, aggregations):
vector[CAggregate] c_aggregations
CDatum result
CAggregate c_aggr
+ shared_ptr[CFunctionOptions] null_opts
_pack_compute_args(args, &c_args)
_pack_compute_args(keys, &c_keys)
for aggr_func_name, aggr_opts in aggregations:
c_aggr.function = tobytes(aggr_func_name)
if aggr_opts is not None:
- c_aggr.options = (<FunctionOptions?> aggr_opts).get_options()
+ # (<FunctionOptions?> aggr_opts).get_options()
Review Comment:
Ah that's typo. I will remove this.
--
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] lidavidm commented on a diff in pull request #13344: ARROW-16686: [C++] Use shared_ptr with FunctionOptions
Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #13344:
URL: https://github.com/apache/arrow/pull/13344#discussion_r899011680
##########
python/pyarrow/_compute.pyx:
##########
@@ -2067,16 +2067,18 @@ def _group_by(args, keys, aggregations):
vector[CAggregate] c_aggregations
CDatum result
CAggregate c_aggr
+ shared_ptr[CFunctionOptions] null_opts
_pack_compute_args(args, &c_args)
_pack_compute_args(keys, &c_keys)
for aggr_func_name, aggr_opts in aggregations:
c_aggr.function = tobytes(aggr_func_name)
if aggr_opts is not None:
- c_aggr.options = (<FunctionOptions?> aggr_opts).get_options()
+ # (<FunctionOptions?> aggr_opts).get_options()
Review Comment:
what is the comment for?
##########
cpp/src/arrow/compute/api_aggregate.h:
##########
@@ -401,7 +401,7 @@ struct ARROW_EXPORT Aggregate {
std::string function;
/// options for the aggregation function
- const FunctionOptions* options;
+ std::shared_ptr<FunctionOptions> options;
Review Comment:
is `unique_ptr` possible?
##########
cpp/src/arrow/compute/exec/aggregate.cc:
##########
@@ -61,7 +61,11 @@ Result<std::vector<std::unique_ptr<KernelState>>> InitKernels(
// use known default options for the named function if possible
auto maybe_function = ctx->func_registry()->GetFunction(aggregates[i].function);
if (maybe_function.ok()) {
- options = maybe_function.ValueOrDie()->default_options();
+ auto default_opts = maybe_function.ValueOrDie()->default_options();
+ // only update the options if the default options are set
+ if (default_opts != nullptr) {
+ options = default_opts->Copy();
+ }
Review Comment:
```suggestion
FunctionOptions* default_opts = maybe_function.ValueOrDie()->default_options();
if (default_opts != nullptr) {
options = default_opts->Copy();
}
```
##########
cpp/src/arrow/compute/exec/aggregate_node.cc:
##########
@@ -116,11 +111,8 @@ class ScalarAggregateNode : public ExecNode {
kernels[i] = static_cast<const ScalarAggregateKernel*>(kernel);
if (aggregates[i].options == nullptr) {
- aggregates[i].options = function->default_options();
- }
- if (aggregates[i].options) {
- owned_options[i] = aggregates[i].options->Copy();
- aggregates[i].options = owned_options[i].get();
+ auto opts = function->default_options()->Copy();
+ aggregates[i].options = std::move(opts);
Review Comment:
```suggestion
aggregates[i].options = function->default_options()->Copy();
```
##########
python/pyarrow/_compute.pyx:
##########
@@ -2067,16 +2067,18 @@ def _group_by(args, keys, aggregations):
vector[CAggregate] c_aggregations
CDatum result
CAggregate c_aggr
+ shared_ptr[CFunctionOptions] null_opts
_pack_compute_args(args, &c_args)
_pack_compute_args(keys, &c_keys)
for aggr_func_name, aggr_opts in aggregations:
c_aggr.function = tobytes(aggr_func_name)
if aggr_opts is not None:
- c_aggr.options = (<FunctionOptions?> aggr_opts).get_options()
+ # (<FunctionOptions?> aggr_opts).get_options()
+ c_aggr.options = (<FunctionOptions?>aggr_opts).wrapped
else:
- c_aggr.options = NULL
+ c_aggr.options = null_opts
Review Comment:
`<shared_ptr[CFunctionOptions]>nullptr` should work
--
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] vibhatha commented on a diff in pull request #13344: ARROW-16686: [C++] Use shared_ptr with FunctionOptions
Posted by GitBox <gi...@apache.org>.
vibhatha commented on code in PR #13344:
URL: https://github.com/apache/arrow/pull/13344#discussion_r899656573
##########
cpp/src/arrow/compute/exec/aggregate.cc:
##########
@@ -61,7 +61,11 @@ Result<std::vector<std::unique_ptr<KernelState>>> InitKernels(
// use known default options for the named function if possible
auto maybe_function = ctx->func_registry()->GetFunction(aggregates[i].function);
if (maybe_function.ok()) {
- options = maybe_function.ValueOrDie()->default_options();
+ const FunctionOptions* default_opts =
Review Comment:
Good catch. Thank you.
--
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] vibhatha commented on pull request #13344: ARROW-16686: [C++] Use shared_ptr with FunctionOptions
Posted by GitBox <gi...@apache.org>.
vibhatha commented on PR #13344:
URL: https://github.com/apache/arrow/pull/13344#issuecomment-1158377125
@westonpace updated the PR.
--
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 #13344: ARROW-16686: [C++] Use shared_ptr with FunctionOptions
Posted by GitBox <gi...@apache.org>.
westonpace commented on code in PR #13344:
URL: https://github.com/apache/arrow/pull/13344#discussion_r900281049
##########
python/pyarrow/_compute.pyx:
##########
@@ -2067,16 +2067,17 @@ def _group_by(args, keys, aggregations):
vector[CAggregate] c_aggregations
CDatum result
CAggregate c_aggr
+ shared_ptr[CFunctionOptions] null_opts
Review Comment:
```suggestion
```
I don't think this is needed any longer.
--
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] vibhatha commented on pull request #13344: ARROW-16686: [C++] Use shared_ptr with FunctionOptions
Posted by GitBox <gi...@apache.org>.
vibhatha commented on PR #13344:
URL: https://github.com/apache/arrow/pull/13344#issuecomment-1157473542
cc @lidavidm @westonpace could you please take a look.
--
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] vibhatha commented on pull request #13344: ARROW-16686: [C++] Use shared_ptr with FunctionOptions
Posted by GitBox <gi...@apache.org>.
vibhatha commented on PR #13344:
URL: https://github.com/apache/arrow/pull/13344#issuecomment-1153044022
@kou need help with the C GLib changes.
--
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] kou commented on pull request #13344: ARROW-16686: [C++] Use shared_ptr with FunctionOptions
Posted by GitBox <gi...@apache.org>.
kou commented on PR #13344:
URL: https://github.com/apache/arrow/pull/13344#issuecomment-1157293604
I think that you got conflicts when you run `git rebase apache/master`. I think that you run `git rebase --skip` for these conflicts. It dropped my commits.
--
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] lidavidm merged pull request #13344: ARROW-16686: [C++] Use shared_ptr with FunctionOptions
Posted by GitBox <gi...@apache.org>.
lidavidm merged PR #13344:
URL: https://github.com/apache/arrow/pull/13344
--
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] vibhatha commented on pull request #13344: ARROW-16686: [C++] Use unique_ptr with FunctionOptions
Posted by GitBox <gi...@apache.org>.
vibhatha commented on PR #13344:
URL: https://github.com/apache/arrow/pull/13344#issuecomment-1152760438
cc @westonpace should the owned smart pointers be removed?
--
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] vibhatha commented on pull request #13344: ARROW-16686: [C++] Use shared_ptr with FunctionOptions
Posted by GitBox <gi...@apache.org>.
vibhatha commented on PR #13344:
URL: https://github.com/apache/arrow/pull/13344#issuecomment-1153191171
> Done.
Thank you!
--
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] vibhatha commented on pull request #13344: ARROW-16686: [C++] Use shared_ptr with FunctionOptions
Posted by GitBox <gi...@apache.org>.
vibhatha commented on PR #13344:
URL: https://github.com/apache/arrow/pull/13344#issuecomment-1157286593
> I've reapplied my patches.
did my rebase caused any issue?
--
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] vibhatha commented on a diff in pull request #13344: ARROW-16686: [C++] Use shared_ptr with FunctionOptions
Posted by GitBox <gi...@apache.org>.
vibhatha commented on code in PR #13344:
URL: https://github.com/apache/arrow/pull/13344#discussion_r899042495
##########
cpp/src/arrow/compute/api_aggregate.h:
##########
@@ -401,7 +401,7 @@ struct ARROW_EXPORT Aggregate {
std::string function;
/// options for the aggregation function
- const FunctionOptions* options;
+ std::shared_ptr<FunctionOptions> options;
Review Comment:
Good point, initially the idea was to put a `unique_ptr`, but there was a discussion in the JIRA and finally settled for the `shared_ptr`.
--
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] nealrichardson commented on pull request #13344: ARROW-16686: [C++] Use shared_ptr with FunctionOptions
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on PR #13344:
URL: https://github.com/apache/arrow/pull/13344#issuecomment-1154915526
> cc @nealrichardson made some changes to R library, appreciate your feedback.
Tests pass so LGTM
--
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] vibhatha commented on pull request #13344: ARROW-16686: [C++] Use shared_ptr with FunctionOptions
Posted by GitBox <gi...@apache.org>.
vibhatha commented on PR #13344:
URL: https://github.com/apache/arrow/pull/13344#issuecomment-1157472606
> I think that you got conflicts when you run `git rebase apache/master`. I think that you run `git rebase --skip` for these conflicts. It dropped my commits.
I ran `git rebase --continue` after adding the modified files. Sorry about that.
--
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] vibhatha commented on a diff in pull request #13344: ARROW-16686: [C++] Use shared_ptr with FunctionOptions
Posted by GitBox <gi...@apache.org>.
vibhatha commented on code in PR #13344:
URL: https://github.com/apache/arrow/pull/13344#discussion_r899284906
##########
cpp/src/arrow/compute/exec/aggregate.cc:
##########
@@ -61,7 +61,10 @@ Result<std::vector<std::unique_ptr<KernelState>>> InitKernels(
// use known default options for the named function if possible
auto maybe_function = ctx->func_registry()->GetFunction(aggregates[i].function);
if (maybe_function.ok()) {
- options = maybe_function.ValueOrDie()->default_options();
+ FunctionOptions* default_opts = maybe_function.ValueOrDie()->default_options();
Review Comment:
how about `const FunctionOptions* default_opts = maybe_function.ValueOrDie()->default_options();`?
--
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] vibhatha commented on pull request #13344: ARROW-16686: [C++] Use shared_ptr with FunctionOptions
Posted by GitBox <gi...@apache.org>.
vibhatha commented on PR #13344:
URL: https://github.com/apache/arrow/pull/13344#issuecomment-1157240752
@kou After rebase the C Glib CI is broken. Could you please help?
--
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] lidavidm commented on a diff in pull request #13344: ARROW-16686: [C++] Use shared_ptr with FunctionOptions
Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #13344:
URL: https://github.com/apache/arrow/pull/13344#discussion_r899186417
##########
cpp/src/arrow/compute/exec/aggregate.cc:
##########
@@ -61,7 +61,10 @@ Result<std::vector<std::unique_ptr<KernelState>>> InitKernels(
// use known default options for the named function if possible
auto maybe_function = ctx->func_registry()->GetFunction(aggregates[i].function);
if (maybe_function.ok()) {
- options = maybe_function.ValueOrDie()->default_options();
+ FunctionOptions* default_opts = maybe_function.ValueOrDie()->default_options();
Review Comment:
Hmm, I guess this doesn't work. Maybe `std::move(maybe_function).MoveValueUnsafe()->default_options()` or something?
--
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