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/11/03 17:01:56 UTC
[GitHub] [arrow] jorisvandenbossche opened a new pull request, #14583: ARROW-18229: [Python] Check schema argument type in RecordBatchReader.from_batches
jorisvandenbossche opened a new pull request, #14583:
URL: https://github.com/apache/arrow/pull/14583
(and a similar fix in `unify_schemas`, which currently would also segfault it passing it a non-schema object)
--
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] jorisvandenbossche commented on a diff in pull request #14583: ARROW-18229: [Python] Check schema argument type in RecordBatchReader.from_batches
Posted by GitBox <gi...@apache.org>.
jorisvandenbossche commented on code in PR #14583:
URL: https://github.com/apache/arrow/pull/14583#discussion_r1017816679
##########
python/pyarrow/tests/test_ipc.py:
##########
@@ -1153,9 +1153,17 @@ def make_batches():
batches = iter(UserList(make_batches())) # weakrefable
wr = weakref.ref(batches)
- with pa.ipc.RecordBatchReader.from_batches(make_schema(),
- batches) as reader:
+ with pa.RecordBatchReader.from_batches(make_schema(),
+ batches) as reader:
batches = None
assert wr() is not None
assert list(reader) == make_batches()
assert wr() is None
+
+ # ensure we get proper error when not passing a schema
+ # (https://issues.apache.org/jira/browse/ARROW-18229)
+ batches = make_batches()
+ with pytest.raises(TypeError):
+ reader = pa.RecordBatchReader.from_batches(
+ [('field', pa.int64())], batches)
Review Comment:
Yes, I also mentioned this on the JIRA, but we don't do that anywhere else right now where we accept a schema (I think), so we can open a new JIRA to improve that in general
--
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] jorisvandenbossche merged pull request #14583: ARROW-18229: [Python] Check schema argument type in RecordBatchReader.from_batches
Posted by GitBox <gi...@apache.org>.
jorisvandenbossche merged PR #14583:
URL: https://github.com/apache/arrow/pull/14583
--
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] jorisvandenbossche commented on a diff in pull request #14583: ARROW-18229: [Python] Check schema argument type in RecordBatchReader.from_batches
Posted by GitBox <gi...@apache.org>.
jorisvandenbossche commented on code in PR #14583:
URL: https://github.com/apache/arrow/pull/14583#discussion_r1017815603
##########
python/pyarrow/tests/test_ipc.py:
##########
@@ -1153,9 +1153,17 @@ def make_batches():
batches = iter(UserList(make_batches())) # weakrefable
wr = weakref.ref(batches)
- with pa.ipc.RecordBatchReader.from_batches(make_schema(),
- batches) as reader:
+ with pa.RecordBatchReader.from_batches(make_schema(),
+ batches) as reader:
batches = None
assert wr() is not None
assert list(reader) == make_batches()
assert wr() is None
+
+ # ensure we get proper error when not passing a schema
+ # (https://issues.apache.org/jira/browse/ARROW-18229)
+ batches = make_batches()
+ with pytest.raises(TypeError):
+ reader = pa.RecordBatchReader.from_batches(
+ [('field', pa.int64())], batches)
+ str(reader.schema)
Review Comment:
With this PR, it is the `from_batches` that is already raising the TypeErrror, but the original segfault came from inspecting a null schema, so therefore I left the code that reproduced the original issue (although now it it not strictly needed)
--
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] jorisvandenbossche commented on a diff in pull request #14583: ARROW-18229: [Python] Check schema argument type in RecordBatchReader.from_batches
Posted by GitBox <gi...@apache.org>.
jorisvandenbossche commented on code in PR #14583:
URL: https://github.com/apache/arrow/pull/14583#discussion_r1017820400
##########
python/pyarrow/tests/test_ipc.py:
##########
@@ -1153,9 +1153,17 @@ def make_batches():
batches = iter(UserList(make_batches())) # weakrefable
wr = weakref.ref(batches)
- with pa.ipc.RecordBatchReader.from_batches(make_schema(),
- batches) as reader:
+ with pa.RecordBatchReader.from_batches(make_schema(),
+ batches) as reader:
batches = None
assert wr() is not None
assert list(reader) == make_batches()
assert wr() is None
+
+ # ensure we get proper error when not passing a schema
+ # (https://issues.apache.org/jira/browse/ARROW-18229)
+ batches = make_batches()
+ with pytest.raises(TypeError):
+ reader = pa.RecordBatchReader.from_batches(
+ [('field', pa.int64())], batches)
+ str(reader.schema)
Review Comment:
> the original segfault came from inspecting a null schema
That's probably something to fix separately as well, although there is no way to create such a null schema from Python manually, except through a bug like this (it is the cython `shared_ptr[CSchema]()`)
--
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] jorisvandenbossche commented on a diff in pull request #14583: ARROW-18229: [Python] Check schema argument type in RecordBatchReader.from_batches
Posted by GitBox <gi...@apache.org>.
jorisvandenbossche commented on code in PR #14583:
URL: https://github.com/apache/arrow/pull/14583#discussion_r1017820400
##########
python/pyarrow/tests/test_ipc.py:
##########
@@ -1153,9 +1153,17 @@ def make_batches():
batches = iter(UserList(make_batches())) # weakrefable
wr = weakref.ref(batches)
- with pa.ipc.RecordBatchReader.from_batches(make_schema(),
- batches) as reader:
+ with pa.RecordBatchReader.from_batches(make_schema(),
+ batches) as reader:
batches = None
assert wr() is not None
assert list(reader) == make_batches()
assert wr() is None
+
+ # ensure we get proper error when not passing a schema
+ # (https://issues.apache.org/jira/browse/ARROW-18229)
+ batches = make_batches()
+ with pytest.raises(TypeError):
+ reader = pa.RecordBatchReader.from_batches(
+ [('field', pa.int64())], batches)
+ str(reader.schema)
Review Comment:
> the original segfault came from inspecting a null schema
That's probably something to fix separately as well, although there is no way to create this from Python, except through a bug like this (it is the cython `shared_ptr[CSchema]()`)
--
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 #14583: ARROW-18229: [Python] Check schema argument type in RecordBatchReader.from_batches
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #14583:
URL: https://github.com/apache/arrow/pull/14583#issuecomment-1302438841
:warning: Ticket **has not been started in JIRA**, please click 'Start Progress'.
--
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 #14583: ARROW-18229: [Python] Check schema argument type in RecordBatchReader.from_batches
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #14583:
URL: https://github.com/apache/arrow/pull/14583#discussion_r1017809161
##########
python/pyarrow/tests/test_ipc.py:
##########
@@ -1153,9 +1153,17 @@ def make_batches():
batches = iter(UserList(make_batches())) # weakrefable
wr = weakref.ref(batches)
- with pa.ipc.RecordBatchReader.from_batches(make_schema(),
- batches) as reader:
+ with pa.RecordBatchReader.from_batches(make_schema(),
+ batches) as reader:
batches = None
assert wr() is not None
assert list(reader) == make_batches()
assert wr() is None
+
+ # ensure we get proper error when not passing a schema
+ # (https://issues.apache.org/jira/browse/ARROW-18229)
+ batches = make_batches()
+ with pytest.raises(TypeError):
+ reader = pa.RecordBatchReader.from_batches(
+ [('field', pa.int64())], batches)
Review Comment:
TBH, it would be nice if the list of fields was implicitly converted into a schema here :-)
##########
python/pyarrow/tests/test_ipc.py:
##########
@@ -1153,9 +1153,17 @@ def make_batches():
batches = iter(UserList(make_batches())) # weakrefable
wr = weakref.ref(batches)
- with pa.ipc.RecordBatchReader.from_batches(make_schema(),
- batches) as reader:
+ with pa.RecordBatchReader.from_batches(make_schema(),
+ batches) as reader:
batches = None
assert wr() is not None
assert list(reader) == make_batches()
assert wr() is None
+
+ # ensure we get proper error when not passing a schema
+ # (https://issues.apache.org/jira/browse/ARROW-18229)
+ batches = make_batches()
+ with pytest.raises(TypeError):
+ reader = pa.RecordBatchReader.from_batches(
+ [('field', pa.int64())], batches)
+ str(reader.schema)
Review Comment:
Why the `str` call? Which line is failing with a `TypeError`?
##########
python/pyarrow/ipc.pxi:
##########
@@ -757,7 +757,7 @@ cdef class RecordBatchReader(_Weakrefable):
return self
@staticmethod
- def from_batches(schema, batches):
+ def from_batches(Schema schema, batches):
Review Comment:
I don't think this protects `schema` from being None, though?
--
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] jorisvandenbossche commented on a diff in pull request #14583: ARROW-18229: [Python] Check schema argument type in RecordBatchReader.from_batches
Posted by GitBox <gi...@apache.org>.
jorisvandenbossche commented on code in PR #14583:
URL: https://github.com/apache/arrow/pull/14583#discussion_r1017816679
##########
python/pyarrow/tests/test_ipc.py:
##########
@@ -1153,9 +1153,17 @@ def make_batches():
batches = iter(UserList(make_batches())) # weakrefable
wr = weakref.ref(batches)
- with pa.ipc.RecordBatchReader.from_batches(make_schema(),
- batches) as reader:
+ with pa.RecordBatchReader.from_batches(make_schema(),
+ batches) as reader:
batches = None
assert wr() is not None
assert list(reader) == make_batches()
assert wr() is None
+
+ # ensure we get proper error when not passing a schema
+ # (https://issues.apache.org/jira/browse/ARROW-18229)
+ batches = make_batches()
+ with pytest.raises(TypeError):
+ reader = pa.RecordBatchReader.from_batches(
+ [('field', pa.int64())], batches)
Review Comment:
Yes, I also mentioned this on the JIRA, but we don't do that anywhere else right now where we accept a schema (I think), so we can open a new JIRA to improve that in general (I just want to prevent the segfault 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] ursabot commented on pull request #14583: ARROW-18229: [Python] Check schema argument type in RecordBatchReader.from_batches
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #14583:
URL: https://github.com/apache/arrow/pull/14583#issuecomment-1309829586
Benchmark runs are scheduled for baseline = 0ef4510dffd31849aa4cfcef661f591a4360fe4c and contender = 9c9da1037102fb7761a8a327b46fc0d21077f263. 9c9da1037102fb7761a8a327b46fc0d21077f263 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/5b3e8faf4b6744cb9ad5697c000f53d8...2404e27eb2fc4c0c882cf51f0552d53b/)
[Failed :arrow_down:0.58% :arrow_up:0.03%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/0f12e61de0624028ba77e8e12bd60518...303ccb8e6ed845a4b9b07cd62bc43940/)
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/07768634729f427bbd49c2c427a37108...f0b229030dd14833a75294f2f66cf44f/)
[Finished :arrow_down:0.49% :arrow_up:0.0%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/267c90dc34014c05986de974c38292c8...8aec32654cdd442bb476a03c5a0107bd/)
Buildkite builds:
[Finished] [`9c9da103` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/1841)
[Failed] [`9c9da103` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/1863)
[Finished] [`9c9da103` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/1829)
[Finished] [`9c9da103` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/1855)
[Finished] [`0ef4510d` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/1840)
[Finished] [`0ef4510d` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/1862)
[Finished] [`0ef4510d` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/1828)
[Finished] [`0ef4510d` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/1854)
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] jorisvandenbossche commented on a diff in pull request #14583: ARROW-18229: [Python] Check schema argument type in RecordBatchReader.from_batches
Posted by GitBox <gi...@apache.org>.
jorisvandenbossche commented on code in PR #14583:
URL: https://github.com/apache/arrow/pull/14583#discussion_r1017813988
##########
python/pyarrow/ipc.pxi:
##########
@@ -757,7 +757,7 @@ cdef class RecordBatchReader(_Weakrefable):
return self
@staticmethod
- def from_batches(schema, batches):
+ def from_batches(Schema schema, batches):
Review Comment:
Ah, yes, will add a `not None`
--
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 #14583: ARROW-18229: [Python] Check schema argument type in RecordBatchReader.from_batches
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #14583:
URL: https://github.com/apache/arrow/pull/14583#issuecomment-1302438816
https://issues.apache.org/jira/browse/ARROW-18229
--
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 #14583: ARROW-18229: [Python] Check schema argument type in RecordBatchReader.from_batches
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #14583:
URL: https://github.com/apache/arrow/pull/14583#discussion_r1017819803
##########
python/pyarrow/tests/test_ipc.py:
##########
@@ -1153,9 +1153,17 @@ def make_batches():
batches = iter(UserList(make_batches())) # weakrefable
wr = weakref.ref(batches)
- with pa.ipc.RecordBatchReader.from_batches(make_schema(),
- batches) as reader:
+ with pa.RecordBatchReader.from_batches(make_schema(),
+ batches) as reader:
batches = None
assert wr() is not None
assert list(reader) == make_batches()
assert wr() is None
+
+ # ensure we get proper error when not passing a schema
+ # (https://issues.apache.org/jira/browse/ARROW-18229)
+ batches = make_batches()
+ with pytest.raises(TypeError):
+ reader = pa.RecordBatchReader.from_batches(
+ [('field', pa.int64())], batches)
+ str(reader.schema)
Review Comment:
The problem is that the test might succeed now if `from_batches` succeeds but `str` raises. So we should remove this `str` call, IMHO.
--
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