You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "zeroshade (via GitHub)" <gi...@apache.org> on 2023/06/21 20:04:55 UTC
[GitHub] [arrow] zeroshade opened a new pull request, #36215: GH-35760: [C++] `ArrowArrayStreamRelease` et all should confirm the struct is released
zeroshade opened a new pull request, #36215:
URL: https://github.com/apache/arrow/pull/36215
<!--
Thanks for opening a pull request!
If this is your first pull request you can find detailed information on how
to contribute here:
* [New Contributor's Guide](https://arrow.apache.org/docs/dev/developers/guide/step_by_step/pr_lifecycle.html#reviews-and-merge-of-the-pull-request)
* [Contributing Overview](https://arrow.apache.org/docs/dev/developers/overview.html)
If this is not a [minor PR](https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes). Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose
Opening GitHub issues 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 the pull request title in the following format?
GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
or
MINOR: [${COMPONENT}] ${SUMMARY}
In the case of PARQUET issues on JIRA the title also supports:
PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
-->
### What changes are included in this PR?
Replaced `assert`s in the `Arrow*Release` methods with a macro which outputs to `stderr` and calls `std::abort` directly with a message. This way the check still happens in release mode. The `ARROW_CHECK*` macros require C++ and this is intended to be a C file which others can vendor.
### Are there any user-facing changes?
Instead of only failing in debug mode, now these release methods will fail if the release callback fails to set the `release` member to `null` even in release mode. Leading to potential calls to `std::abort` that weren't previously occurring.
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please uncomment the line below and explain which changes are breaking.
-->
<!-- **This PR includes breaking changes to public APIs.** -->
<!--
Please uncomment the line below (and provide explanation) if the changes fix either (a) a security vulnerability, (b) a bug that caused incorrect or invalid data to be produced, or (c) a bug that causes a crash (even when the API contract is upheld). We use this to highlight fixes to issues that may affect users without their knowledge. For this reason, fixing bugs that cause errors don't count, since those are usually obvious.
-->
<!-- **This PR contains a "Critical Fix".** -->
--
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] pitrou merged pull request #36215: GH-35760: [C++] C Data Interface helpers should also run checks in non-debug mode
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou merged PR #36215:
URL: https://github.com/apache/arrow/pull/36215
--
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] pitrou commented on a diff in pull request #36215: GH-35760: [C++] `ArrowArrayStreamRelease` et all should confirm the struct is released
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #36215:
URL: https://github.com/apache/arrow/pull/36215#discussion_r1238300660
##########
cpp/src/arrow/c/helpers.h:
##########
@@ -18,10 +18,20 @@
#pragma once
#include <assert.h>
+#include <stdio.h>
+#include <stdlib.h>
#include <string.h>
#include "arrow/c/abi.h"
+#define CHECK_OR_DIE(condition, msg) \
+ do { \
+ if (!(condition)) { \
+ fprintf(stderr, "%s:%d:: %s", __FILE__, __LINE__, msg); \
Review Comment:
Let's allow `msg` to be complex.
```suggestion
fprintf(stderr, "%s:%d:: %s", __FILE__, __LINE__, (msg)); \
```
--
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] pitrou commented on pull request #36215: GH-35760: [C++] C Data Interface helpers should also run checks in non-debug mode
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #36215:
URL: https://github.com/apache/arrow/pull/36215#issuecomment-1609841666
@zeroshade `go/arrow/cdata/arrow/helpers.h` also needs to be updated but it's not a trivial copy of the C++ source anymore.
--
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] conbench-apache-arrow[bot] commented on pull request #36215: GH-35760: [C++] C Data Interface helpers should also run checks in non-debug mode
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #36215:
URL: https://github.com/apache/arrow/pull/36215#issuecomment-1615209531
Conbench analyzed the 6 benchmark runs on commit `de4936d8`.
There was 1 benchmark result indicating a performance regression:
- Commit Run on `arm64-m6g-linux-compute` at [2023-06-27 20:39:44Z](http://conbench.ursa.dev/compare/runs/792e3c7b4b6e49ea94098c07e6d85c3e...f188e67b07af44c78bd0f12d33cd321a/)
- [params=num_cols:64/is_partial:1/real_time, source=cpp-micro, suite=arrow-ipc-read-write-benchmark](http://conbench.ursa.dev/compare/benchmarks/0649b368d4c173538000191e9f5549ca...0649b49567ae7d438000598e0956ae5f)
The [full Conbench report](https://github.com/apache/arrow/runs/14698752461) has more details.
--
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 #36215: GH-35760: [C++] `ArrowArrayStreamRelease` et all should confirm the struct is released
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36215:
URL: https://github.com/apache/arrow/pull/36215#issuecomment-1601598866
:warning: GitHub issue #35760 **has been automatically assigned in GitHub** to PR creator.
--
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] pitrou commented on pull request #36215: GH-35760: [C++] C Data Interface helpers should also run checks in non-debug mode
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #36215:
URL: https://github.com/apache/arrow/pull/36215#issuecomment-1609991512
@zeroshade Can you open an issue for the Go followup?
--
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] pitrou commented on a diff in pull request #36215: GH-35760: [C++] `ArrowArrayStreamRelease` et all should confirm the struct is released
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #36215:
URL: https://github.com/apache/arrow/pull/36215#discussion_r1238299076
##########
cpp/src/arrow/c/helpers.h:
##########
@@ -18,10 +18,20 @@
#pragma once
#include <assert.h>
+#include <stdio.h>
+#include <stdlib.h>
#include <string.h>
#include "arrow/c/abi.h"
+#define CHECK_OR_DIE(condition, msg) \
+ do { \
+ if (!(condition)) { \
+ fprintf(stderr, "%s:%d:: %s", __FILE__, __LINE__, msg); \
+ std::abort(); \
Review Comment:
This needs to be C code, not C++.
--
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] pitrou commented on a diff in pull request #36215: GH-35760: [C++] `ArrowArrayStreamRelease` et all should confirm the struct is released
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #36215:
URL: https://github.com/apache/arrow/pull/36215#discussion_r1238300029
##########
cpp/src/arrow/c/helpers.h:
##########
@@ -18,10 +18,20 @@
#pragma once
#include <assert.h>
+#include <stdio.h>
+#include <stdlib.h>
#include <string.h>
#include "arrow/c/abi.h"
+#define CHECK_OR_DIE(condition, msg) \
Review Comment:
We need to namespace-prefix this macro to avoid clashes with third-party libs.
Perhaps name it `ARROW_C_ASSERT`?
--
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] pitrou commented on a diff in pull request #36215: GH-35760: [C++] `ArrowArrayStreamRelease` et all should confirm the struct is released
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #36215:
URL: https://github.com/apache/arrow/pull/36215#discussion_r1238301219
##########
cpp/src/arrow/c/helpers.h:
##########
@@ -18,10 +18,20 @@
#pragma once
#include <assert.h>
Review Comment:
Can perhaps remove this inclusion now?
--
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] zeroshade commented on a diff in pull request #36215: GH-35760: [C++] `ArrowArrayStreamRelease` et all should confirm the struct is released
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #36215:
URL: https://github.com/apache/arrow/pull/36215#discussion_r1238755622
##########
cpp/src/arrow/c/helpers.h:
##########
@@ -18,10 +18,20 @@
#pragma once
#include <assert.h>
+#include <stdio.h>
+#include <stdlib.h>
#include <string.h>
#include "arrow/c/abi.h"
+#define CHECK_OR_DIE(condition, msg) \
+ do { \
+ if (!(condition)) { \
+ fprintf(stderr, "%s:%d:: %s", __FILE__, __LINE__, msg); \
+ std::abort(); \
Review Comment:
good point, my mistake there at using `std::` it should be just `abort`.
--
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