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/09 16:52:01 UTC

[GitHub] [arrow] rtpsw opened a new pull request, #13355: ARROW-16796: [C++] Fix bad defaulting of ExecContext argument

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

   See https://issues.apache.org/jira/browse/ARROW-16796


-- 
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 pull request #13355: ARROW-16796: [C++] Fix bad defaulting of ExecContext argument

Posted by GitBox <gi...@apache.org>.
westonpace commented on PR #13355:
URL: https://github.com/apache/arrow/pull/13355#issuecomment-1151679039

   Good catch.  I wonder if we should remove the default argument to bind entirely (it would look something like https://github.com/westonpace/arrow/commit/c9ae1dd6a0857af69e48a95ec76480f4c466791e ).  Looks like there are only a few other non-test spots we call bind.
   
    * In the parquet reader we convert statistics into expressions and bind them to the schema.  These expressions will only use min/max and it's only really for simplification and not execution so we're probably ok.
    * The scanner has a number of methods that create exec plans (this is the "lightweight producer" half of the scanner).  We could arguably add an ExecContext to scan options but I think it would better to start phasing out this half of the scanner in favor of direct use of exec plans instead.
   
   CC @bkietz for second opinion


-- 
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 merged pull request #13355: ARROW-16796: [C++] Fix bad defaulting of ExecContext argument

Posted by GitBox <gi...@apache.org>.
westonpace merged PR #13355:
URL: https://github.com/apache/arrow/pull/13355


-- 
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] rtpsw commented on pull request #13355: ARROW-16796: [C++] Fix bad defaulting of ExecContext argument

Posted by GitBox <gi...@apache.org>.
rtpsw commented on PR #13355:
URL: https://github.com/apache/arrow/pull/13355#issuecomment-1151983476

   > Good catch. I wonder if we should remove the default argument to bind entirely (it would look something like [westonpace@c9ae1dd](https://github.com/westonpace/arrow/commit/c9ae1dd6a0857af69e48a95ec76480f4c466791e) ). Looks like there are only a few other non-test spots we call bind.
   
   This discussion has wider scope than this PR, so I moved it to the jira 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] github-actions[bot] commented on pull request #13355: ARROW-16796: [C++] Fix bad defaulting of ExecContext argument

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #13355:
URL: https://github.com/apache/arrow/pull/13355#issuecomment-1151379865

   https://issues.apache.org/jira/browse/ARROW-16796


-- 
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] rtpsw commented on pull request #13355: ARROW-16796: [C++] Fix bad defaulting of ExecContext argument

Posted by GitBox <gi...@apache.org>.
rtpsw commented on PR #13355:
URL: https://github.com/apache/arrow/pull/13355#issuecomment-1151600723

   @westonpace, could you review or refer to someone?


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