You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "rtpsw (via GitHub)" <gi...@apache.org> on 2023/02/13 08:13:11 UTC
[GitHub] [arrow] rtpsw opened a new pull request, #34156: GH-34150: [C++] [Python] Fix improper initialization of ConversionOptions
rtpsw opened a new pull request, #34156:
URL: https://github.com/apache/arrow/pull/34156
This PR fixes a bug that caused a crash. See #34150 for 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] icexelloss commented on pull request #34156: GH-34150: [C++] [Python] Fix improper initialization of ConversionOptions
Posted by "icexelloss (via GitHub)" <gi...@apache.org>.
icexelloss commented on PR #34156:
URL: https://github.com/apache/arrow/pull/34156#issuecomment-1429873967
@rtpsw IIUC, there are two changes in this PR:
(1) Fix ConversionOptions initialization
(2) Add python bindings for ExtensionDetails, set_default_extension_provider, ExtensionProvider, ExtensionSet, etc
If that is correct, I think we don't necessarily need to mix the two - the first is a bug fix and second is additional functionality.
I am comfortable with approving (1) and but I am not sure about (2) so probably need Weston's input (I believe VD folks are in a conference so there might be delay)
--
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 #34156: GH-34150: [C++] [Python] Fix improper initialization of ConversionOptions
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on PR #34156:
URL: https://github.com/apache/arrow/pull/34156#issuecomment-1434926511
> Update: I confirmed in the same env that switching from inline member initialization to a constructor in ConversionOptions fixes the SIGSEGV.
That is odd. As long as it doesn't involve python changes I don't mind moving to using constructor initialization over member initialization (the Arrow codebase is not at all consistent 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] icexelloss commented on a diff in pull request #34156: GH-34150: [C++] [Python] Fix improper initialization of ConversionOptions
Posted by "icexelloss (via GitHub)" <gi...@apache.org>.
icexelloss commented on code in PR #34156:
URL: https://github.com/apache/arrow/pull/34156#discussion_r1107240204
##########
python/pyarrow/includes/libarrow_substrait.pxd:
##########
@@ -34,10 +51,27 @@ cdef extern from "arrow/engine/substrait/options.h" namespace "arrow::engine" no
BEST_EFFORT \
"arrow::engine::ConversionStrictness::BEST_EFFORT"
+ cdef cppclass CExtensionDetails \
+ "arrow::engine::ExtensionDetails"
+
+ cdef cppclass CExtensionProvider \
+ "arrow::engine::ExtensionProvider":
+ CResult[CRelationInfo] MakeRel(
+ const CConversionOptions conv_opts,
+ const vector[CDeclarationInfo]& inputs,
+ const CExtensionDetails& ext_details,
+ const CExtensionSet& ext_set)
+
+ cdef shared_ptr[CExtensionProvider] default_extension_provider()
+ cdef void set_default_extension_provider(
+ const shared_ptr[CExtensionProvider]& provider)
+
cdef cppclass CConversionOptions \
"arrow::engine::ConversionOptions":
+ CConversionOptions()
ConversionStrictness conversion_strictness
function[CNamedTableProvider] named_table_provider
Review Comment:
I see. I am indifferent about exposing named_tap_provider - but we feel we either (1) expose all or (2) expose minimal. (Although I am not sure which one Pyarrow generally follows)
Does fixing the bug require exposing extension_provider in 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] westonpace commented on pull request #34156: GH-34150: [C++] [Python] Fix improper initialization of ConversionOptions
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on PR #34156:
URL: https://github.com/apache/arrow/pull/34156#issuecomment-1433881825
Static locals should fix cython. Initialization is no longer a global thing and has nothing to do with header order. The fields will be initialized the first time `default_extension_provider` is called. So there is no way for cython to mess it up.
--
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 #34156: GH-34150: [C++] [Python] Fix improper initialization of ConversionOptions
Posted by "rtpsw (via GitHub)" <gi...@apache.org>.
rtpsw commented on PR #34156:
URL: https://github.com/apache/arrow/pull/34156#issuecomment-1432702572
It will take a bit of time for me to check, but I suspect static locals would only fix initialization order in C++ but not in Cython. Specifically, I suspect that Cython sees different header definitions for `ConversionOptions` (those generated from the `pxd` file) than C++ does, and doesn't see the inline initialization of its members in the C++ header, leading to an improperly initialized object. I don't mind using static locals to fix the C++ order of initialization, but I'd still suggest adding a constructor to `ConversionOptions`, where its members are initialized, and expose it to Cython. I added a reduced changeset here for just this exposure.
--
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 #34156: GH-34150: [C++] [Python] Fix improper initialization of ConversionOptions
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #34156:
URL: https://github.com/apache/arrow/pull/34156#issuecomment-1435648570
Benchmark runs are scheduled for baseline = daa5e267fc478ec016c950b7557bc10acbed3e5e and contender = 11ae62a1426a1594ed750be1f25334fe7198356d. 11ae62a1426a1594ed750be1f25334fe7198356d is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/5f5f01d695a345e68a51356b318f6e1b...2f0c931a3f5e476aa4956fdb88f98943/)
[Failed :arrow_down:0.31% :arrow_up:0.0%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/bb60907a188c4ccebf03a13461b2d8b0...d003e86fee9c432ebafdd1deddaba378/)
[Finished :arrow_down:1.53% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/1aa3df4e015e4ed08781b3aaaeec9bf0...5beff8e7b39a4e5899acaa96c3098468/)
[Finished :arrow_down:0.57% :arrow_up:0.06%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/7335b41dda3e4b1dafca772150196f6f...21a81fae44ea41e5862e4ac9cdd64973/)
Buildkite builds:
[Finished] [`11ae62a1` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2398)
[Failed] [`11ae62a1` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2428)
[Finished] [`11ae62a1` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2396)
[Finished] [`11ae62a1` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2420)
[Finished] [`daa5e267` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2397)
[Failed] [`daa5e267` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2427)
[Finished] [`daa5e267` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2395)
[Finished] [`daa5e267` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2419)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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 #34156: GH-34150: [C++] [Python] Fix improper initialization of ConversionOptions
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on PR #34156:
URL: https://github.com/apache/arrow/pull/34156#issuecomment-1432045111
Does #34209 work? I think static locals should be easier because:
* Works without modifying python (which means we don't have to repeat this in R)
* Minor benefit to load time because the singleton is only initialized if used
* Consistent with some of our newer handling of singletons (sorry for not suggesting this earlier)
--
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 #34156: GH-34150: [C++] [Python] Fix improper initialization of ConversionOptions
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34156:
URL: https://github.com/apache/arrow/pull/34156#issuecomment-1427517278
* Closes: #34150
--
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 a diff in pull request #34156: GH-34150: [C++] [Python] Fix improper initialization of ConversionOptions
Posted by "rtpsw (via GitHub)" <gi...@apache.org>.
rtpsw commented on code in PR #34156:
URL: https://github.com/apache/arrow/pull/34156#discussion_r1107481107
##########
python/pyarrow/includes/libarrow_substrait.pxd:
##########
@@ -34,10 +51,27 @@ cdef extern from "arrow/engine/substrait/options.h" namespace "arrow::engine" no
BEST_EFFORT \
"arrow::engine::ConversionStrictness::BEST_EFFORT"
+ cdef cppclass CExtensionDetails \
+ "arrow::engine::ExtensionDetails"
+
+ cdef cppclass CExtensionProvider \
+ "arrow::engine::ExtensionProvider":
+ CResult[CRelationInfo] MakeRel(
+ const CConversionOptions conv_opts,
+ const vector[CDeclarationInfo]& inputs,
+ const CExtensionDetails& ext_details,
+ const CExtensionSet& ext_set)
+
+ cdef shared_ptr[CExtensionProvider] default_extension_provider()
+ cdef void set_default_extension_provider(
+ const shared_ptr[CExtensionProvider]& provider)
+
cdef cppclass CConversionOptions \
"arrow::engine::ConversionOptions":
+ CConversionOptions()
ConversionStrictness conversion_strictness
function[CNamedTableProvider] named_table_provider
Review Comment:
> (Although I am not sure which one Pyarrow generally follows)
I don't know myself, but my guess is it is mixed. You'd probably find pretty much all public Arrow symbols in PyArrow, but not all in sub-components like Arrow Substrait.
> Does fixing the bug require exposing extension_provider in here?
The bug is due to `ConversionOptions` default constructor, so it has to be exposed. I'll check if the bug is fixed with a minimal change including it that I can get to compile.
--
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] icexelloss commented on a diff in pull request #34156: GH-34150: [C++] [Python] Fix improper initialization of ConversionOptions
Posted by "icexelloss (via GitHub)" <gi...@apache.org>.
icexelloss commented on code in PR #34156:
URL: https://github.com/apache/arrow/pull/34156#discussion_r1107200146
##########
python/pyarrow/includes/libarrow_substrait.pxd:
##########
@@ -34,10 +51,27 @@ cdef extern from "arrow/engine/substrait/options.h" namespace "arrow::engine" no
BEST_EFFORT \
"arrow::engine::ConversionStrictness::BEST_EFFORT"
+ cdef cppclass CExtensionDetails \
+ "arrow::engine::ExtensionDetails"
+
+ cdef cppclass CExtensionProvider \
+ "arrow::engine::ExtensionProvider":
+ CResult[CRelationInfo] MakeRel(
+ const CConversionOptions conv_opts,
+ const vector[CDeclarationInfo]& inputs,
+ const CExtensionDetails& ext_details,
+ const CExtensionSet& ext_set)
+
+ cdef shared_ptr[CExtensionProvider] default_extension_provider()
+ cdef void set_default_extension_provider(
+ const shared_ptr[CExtensionProvider]& provider)
+
cdef cppclass CConversionOptions \
"arrow::engine::ConversionOptions":
+ CConversionOptions()
ConversionStrictness conversion_strictness
function[CNamedTableProvider] named_table_provider
Review Comment:
Hold on - we are missing named_tap_provider here - does that not trigger any error?
--
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] icexelloss commented on pull request #34156: GH-34150: [C++] [Python] Fix improper initialization of ConversionOptions
Posted by "icexelloss (via GitHub)" <gi...@apache.org>.
icexelloss commented on PR #34156:
URL: https://github.com/apache/arrow/pull/34156#issuecomment-1431457595
Thanks - This makes sense. The PR looks good to me. But probably need a final approval from @westonpace
--
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 a diff in pull request #34156: GH-34150: [C++] [Python] Fix improper initialization of ConversionOptions
Posted by "rtpsw (via GitHub)" <gi...@apache.org>.
rtpsw commented on code in PR #34156:
URL: https://github.com/apache/arrow/pull/34156#discussion_r1107223003
##########
python/pyarrow/includes/libarrow_substrait.pxd:
##########
@@ -34,10 +51,27 @@ cdef extern from "arrow/engine/substrait/options.h" namespace "arrow::engine" no
BEST_EFFORT \
"arrow::engine::ConversionStrictness::BEST_EFFORT"
+ cdef cppclass CExtensionDetails \
+ "arrow::engine::ExtensionDetails"
+
+ cdef cppclass CExtensionProvider \
+ "arrow::engine::ExtensionProvider":
+ CResult[CRelationInfo] MakeRel(
+ const CConversionOptions conv_opts,
+ const vector[CDeclarationInfo]& inputs,
+ const CExtensionDetails& ext_details,
+ const CExtensionSet& ext_set)
+
+ cdef shared_ptr[CExtensionProvider] default_extension_provider()
+ cdef void set_default_extension_provider(
+ const shared_ptr[CExtensionProvider]& provider)
+
cdef cppclass CConversionOptions \
"arrow::engine::ConversionOptions":
+ CConversionOptions()
ConversionStrictness conversion_strictness
function[CNamedTableProvider] named_table_provider
Review Comment:
This doesn't trigger an error, and this is expected. Cython doesn't care if a symbol exists in C++ but is not exposed in a `pxd`. This allows symbols in a `pxd` to be included on a need-basis. If you'd like this symbol to be exposed, I could add (and touch) it.
--
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 #34156: GH-34150: [C++] [Python] Fix improper initialization of ConversionOptions
Posted by "rtpsw (via GitHub)" <gi...@apache.org>.
rtpsw commented on PR #34156:
URL: https://github.com/apache/arrow/pull/34156#issuecomment-1435941288
Thanks, @westonpace. I was able to use the latest Arrow nightly version to fix the bug.
Do you think #34209 should also be merged?
--
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 #34156: GH-34150: [C++] [Python] Fix improper initialization of ConversionOptions
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on code in PR #34156:
URL: https://github.com/apache/arrow/pull/34156#discussion_r1110085197
##########
python/pyarrow/substrait.py:
##########
@@ -18,4 +18,5 @@
from pyarrow._substrait import ( # noqa
get_supported_functions,
run_query,
+ _check_conversion_options,
Review Comment:
```suggestion
```
##########
python/pyarrow/tests/test_substrait.py:
##########
@@ -34,6 +34,10 @@
pytestmark = [pytest.mark.dataset, pytest.mark.substrait]
+def test_conversion_options():
+ assert substrait._check_conversion_options()
+
+
Review Comment:
```suggestion
```
##########
python/pyarrow/_substrait.pyx:
##########
@@ -26,6 +27,13 @@ from pyarrow.includes.libarrow cimport *
from pyarrow.includes.libarrow_substrait cimport *
+def _check_conversion_options():
+ cdef:
+ CConversionOptions conv_opts
+
+ return conv_opts.strictness == BEST_EFFORT
+
+
Review Comment:
```suggestion
```
Let's get rid of this. It'll just confuse future maintainers more than it helps I think.
--
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] icexelloss commented on a diff in pull request #34156: GH-34150: [C++] [Python] Fix improper initialization of ConversionOptions
Posted by "icexelloss (via GitHub)" <gi...@apache.org>.
icexelloss commented on code in PR #34156:
URL: https://github.com/apache/arrow/pull/34156#discussion_r1105916786
##########
python/pyarrow/includes/libarrow_substrait.pxd:
##########
@@ -34,10 +50,26 @@ cdef extern from "arrow/engine/substrait/options.h" namespace "arrow::engine" no
BEST_EFFORT \
"arrow::engine::ConversionStrictness::BEST_EFFORT"
+ cdef cppclass CExtensionDetails \
+ "arrow::ending::ExtensionDetails"
+
+ cdef cppclass CExtensionProvider \
+ "arrow::ending::ExtensionProvider":
Review Comment:
engine
--
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 #34156: GH-34150: [C++] [Python] Fix improper initialization of ConversionOptions
Posted by "rtpsw (via GitHub)" <gi...@apache.org>.
rtpsw commented on PR #34156:
URL: https://github.com/apache/arrow/pull/34156#issuecomment-1430924091
> Yes, that surprised me too. I'll try to figure out why.
My investigation suggests that Cython only parses `pxd` files, not compile them, but it does compile any symbol in a `pxd` file that is used in a `pyx` file that is compiles. In the [recent commit](https://github.com/apache/arrow/pull/34156/commits/eb8231834c75697b5b32e12874b506cdd7498fa3), I added a dummy function that "touches" the symbols added to the `pxd` here. I observed that typos were caught by Cython, and fixing them allowed the compilation to succeed.
--
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] icexelloss commented on pull request #34156: GH-34150: [C++] [Python] Fix improper initialization of ConversionOptions
Posted by "icexelloss (via GitHub)" <gi...@apache.org>.
icexelloss commented on PR #34156:
URL: https://github.com/apache/arrow/pull/34156#issuecomment-1430119943
I see - so (2) is required to fix (1) - ok
--
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 #34156: GH-34150: [C++] [Python] Fix improper initialization of ConversionOptions
Posted by "rtpsw (via GitHub)" <gi...@apache.org>.
rtpsw commented on PR #34156:
URL: https://github.com/apache/arrow/pull/34156#issuecomment-1430106854
> @rtpsw IIUC, there are two changes in this PR: (1) Fix ConversionOptions initialization (2) Add python bindings for ExtensionDetails, set_default_extension_provider, ExtensionProvider, ExtensionSet, etc
>
> If that is correct, I think we don't necessarily need to mix the two - the first is a bug fix and second is additional functionality.
>
> I am comfortable with approving (1) and but I am not sure about (2) so probably need Weston's input (I believe VD folks are in a conference so there might be delay)
The reason for (2) is because API signatures of `ConversionOptions` bring in other symbols. There might be some symbols we could reduce, but not all of them.
--
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 #34156: GH-34150: [C++] [Python] Fix improper initialization of ConversionOptions
Posted by "rtpsw (via GitHub)" <gi...@apache.org>.
rtpsw commented on PR #34156:
URL: https://github.com/apache/arrow/pull/34156#issuecomment-1434276173
> Static locals should fix cython. Initialization is no longer a global thing and has nothing to do with header order. The fields will be initialized the first time `default_extension_provider` is called. So there is no way for cython to mess it up.
I used your code but, unfortunately, I'm still seeing the same SIGSEGV due to an improperly initialized `extension_provider` member of `ConversionOptions`. How about combining both your and my 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] westonpace merged pull request #34156: GH-34150: [C++] [Python] Fix improper initialization of ConversionOptions
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace merged PR #34156:
URL: https://github.com/apache/arrow/pull/34156
--
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] icexelloss commented on pull request #34156: GH-34150: [C++] [Python] Fix improper initialization of ConversionOptions
Posted by "icexelloss (via GitHub)" <gi...@apache.org>.
icexelloss commented on PR #34156:
URL: https://github.com/apache/arrow/pull/34156#issuecomment-1430120955
@rtpsw This is tested internally and works? (I am surprised that nothing breaks because of the engine typo)
--
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] icexelloss commented on a diff in pull request #34156: GH-34150: [C++] [Python] Fix improper initialization of ConversionOptions
Posted by "icexelloss (via GitHub)" <gi...@apache.org>.
icexelloss commented on code in PR #34156:
URL: https://github.com/apache/arrow/pull/34156#discussion_r1107234617
##########
python/pyarrow/includes/libarrow_substrait.pxd:
##########
@@ -34,10 +51,27 @@ cdef extern from "arrow/engine/substrait/options.h" namespace "arrow::engine" no
BEST_EFFORT \
"arrow::engine::ConversionStrictness::BEST_EFFORT"
+ cdef cppclass CExtensionDetails \
+ "arrow::engine::ExtensionDetails"
+
+ cdef cppclass CExtensionProvider \
+ "arrow::engine::ExtensionProvider":
+ CResult[CRelationInfo] MakeRel(
+ const CConversionOptions conv_opts,
+ const vector[CDeclarationInfo]& inputs,
+ const CExtensionDetails& ext_details,
+ const CExtensionSet& ext_set)
+
+ cdef shared_ptr[CExtensionProvider] default_extension_provider()
+ cdef void set_default_extension_provider(
+ const shared_ptr[CExtensionProvider]& provider)
+
cdef cppclass CConversionOptions \
"arrow::engine::ConversionOptions":
+ CConversionOptions()
ConversionStrictness conversion_strictness
function[CNamedTableProvider] named_table_provider
Review Comment:
Hmm..why do we expose extension_provider and not named_tap_provider then?
--
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 #34156: GH-34150: [C++] [Python] Fix improper initialization of ConversionOptions
Posted by "rtpsw (via GitHub)" <gi...@apache.org>.
rtpsw commented on PR #34156:
URL: https://github.com/apache/arrow/pull/34156#issuecomment-1427781818
@westonpace, could you suggest what tests to add?
--
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] icexelloss commented on a diff in pull request #34156: GH-34150: [C++] [Python] Fix improper initialization of ConversionOptions
Posted by "icexelloss (via GitHub)" <gi...@apache.org>.
icexelloss commented on code in PR #34156:
URL: https://github.com/apache/arrow/pull/34156#discussion_r1105916065
##########
python/pyarrow/includes/libarrow_substrait.pxd:
##########
@@ -34,10 +50,26 @@ cdef extern from "arrow/engine/substrait/options.h" namespace "arrow::engine" no
BEST_EFFORT \
"arrow::engine::ConversionStrictness::BEST_EFFORT"
+ cdef cppclass CExtensionDetails \
+ "arrow::ending::ExtensionDetails"
Review Comment:
This should be "engine"?
Does it not error out if this is wrong?
--
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 a diff in pull request #34156: GH-34150: [C++] [Python] Fix improper initialization of ConversionOptions
Posted by "rtpsw (via GitHub)" <gi...@apache.org>.
rtpsw commented on code in PR #34156:
URL: https://github.com/apache/arrow/pull/34156#discussion_r1106125463
##########
python/pyarrow/includes/libarrow_substrait.pxd:
##########
@@ -34,10 +50,26 @@ cdef extern from "arrow/engine/substrait/options.h" namespace "arrow::engine" no
BEST_EFFORT \
"arrow::engine::ConversionStrictness::BEST_EFFORT"
+ cdef cppclass CExtensionDetails \
+ "arrow::ending::ExtensionDetails"
Review Comment:
It doesn't error in CI either. Surprising.
--
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 #34156: GH-34150: [C++] [Python] Fix improper initialization of ConversionOptions
Posted by "rtpsw (via GitHub)" <gi...@apache.org>.
rtpsw commented on PR #34156:
URL: https://github.com/apache/arrow/pull/34156#issuecomment-1430211113
> @rtpsw This is tested internally and works?
There are no new tests, but the change in `ConversionOptions` is covered by existing tests. The difficulty with a distinguishing test for this change, i.e., a test that fails pre-PR code but passes post-PR, is that it would need to somehow tinker with the order of initialization (see #34150) - there is no precedent of doing this in Arrow that I know of.
> (I am surprised that nothing breaks because of the engine typo)
Yes, that surprised me too. I'll try to figure out why.
--
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