You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "brijrajk (via GitHub)" <gi...@apache.org> on 2026/05/28 03:54:01 UTC
[PR] [SPARK-43847][PYTHON] Throw structured error when reading Protobuf descriptor file fails [spark]
brijrajk opened a new pull request, #56174:
URL: https://github.com/apache/spark/pull/56174
### What changes were proposed in this pull request?
`_read_descriptor_set_file()` in `pyspark.sql.protobuf.functions` now raises
`PySparkRuntimeError` with error class `PROTOBUF_DESCRIPTOR_FILE_NOT_FOUND`
instead of propagating a raw Python `OSError` / `FileNotFoundError` when the
descriptor file cannot be read.
The error class and its message already existed on the Scala side
(`common/utils/src/main/resources/error/error-conditions.json`); this PR adds
the matching entry to `python/pyspark/errors/error-conditions.json` and wires
it up in the Python code.
### Why are the changes needed?
Before this fix, users who pass a wrong or missing descriptor file path get a
bare Python OS error with no Spark context:
```
FileNotFoundError: [Errno 2] No such file or directory: '/path/to/file.desc'
```
After this fix they get a structured Spark error consistent with the rest of
the PySpark error framework:
```
PySparkRuntimeError: [PROTOBUF_DESCRIPTOR_FILE_NOT_FOUND]
Error reading Protobuf descriptor file at path: /path/to/file.desc.
```
### Does this PR introduce _any_ user-facing change?
Yes. Users who catch `OSError` / `FileNotFoundError` around `from_protobuf` /
`to_protobuf` calls that supply a `descFilePath` will now need to catch
`PySparkRuntimeError` instead (or the common base `PySparkException`). The
error message is clearer and includes the file path.
### How was this patch tested?
Added `python/pyspark/sql/tests/test_protobuf.py` with
`test_read_descriptor_set_file_not_found`, which asserts that a missing path
raises `PySparkRuntimeError` with the correct error class and `filePath`
message parameter.
Run locally with:
```
PYTHONPATH=python python3 -m unittest pyspark.sql.tests.test_protobuf -v
```
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Sonnet 4.6
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-43847][PYTHON] Throw structured error when reading Protobuf descriptor file fails [spark]
Posted by "nchammas (via GitHub)" <gi...@apache.org>.
nchammas commented on code in PR #56174:
URL: https://github.com/apache/spark/pull/56174#discussion_r4142684190
##########
common/utils/src/main/resources/error/error-conditions.json:
##########
@@ -6919,6 +6919,12 @@
],
"sqlState" : "42K0G"
},
+ "PROTOBUF_DESCRIPTOR_FILE_READ_FAILED" : {
Review Comment:
@HyukjinKwon - I think in a future PR it _would_ make sense to add this to the Scala side and replace the (IMO incorrect) uses of `CANNOT_PARSE_PROTOBUF_DESCRIPTOR`: [case 1], [case 2]
In both these cases no attempt is made to parse the bytes. The failures are before any parsing, so the current error condition name is misleading. `PROTOBUF_DESCRIPTOR_FILE_READ_FAILED` is more appropriate.
This is in contrast to other cases where there is a genuine problem specifically with parsing: [case 3]
Not a huge deal, and maybe we don't want to change existing behavior. But something to consider, especially if it becomes more important for clients to distinguish these types of failures.
[case 1]: https://github.com/apache/spark/blob/578105289d82158f7d04d7976d547038d1a270c2/sql/api/src/main/scala/org/apache/spark/sql/util/ProtobufUtils.scala#L36
[case 2]: https://github.com/apache/spark/blob/578105289d82158f7d04d7976d547038d1a270c2/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/ProtobufDescriptorFileReader.scala#L89-L95
[case 3]: https://github.com/apache/spark/blob/578105289d82158f7d04d7976d547038d1a270c2/connector/protobuf/src/main/scala/org/apache/spark/sql/protobuf/utils/ProtobufUtils.scala#L310-L315
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-43847][PYTHON] Throw structured error when reading Protobuf descriptor file fails [spark]
Posted by "brijrajk (via GitHub)" <gi...@apache.org>.
brijrajk commented on code in PR #56174:
URL: https://github.com/apache/spark/pull/56174#discussion_r4143129819
##########
common/utils/src/main/resources/error/error-conditions.json:
##########
@@ -6919,6 +6919,12 @@
],
"sqlState" : "42K0G"
},
+ "PROTOBUF_DESCRIPTOR_FILE_READ_FAILED" : {
Review Comment:
Done — removed `PROTOBUF_DESCRIPTOR_FILE_READ_FAILED` from `common/utils/src/main/resources/error/error-conditions.json`. It now only lives in `python/pyspark/errors/error-conditions.json`.
##########
python/pyspark/sql/tests/test_protobuf.py:
##########
@@ -0,0 +1,78 @@
+#
+# 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.
+#
+
+import tempfile
+import unittest
+
+from pyspark.errors import PySparkRuntimeError
+from pyspark.sql.protobuf.functions import _read_descriptor_set_file
+from pyspark.testing.connectutils import should_test_connect
+from pyspark.testing.utils import PySparkErrorTestUtils
+
+
+class ProtobufFunctionsTests(unittest.TestCase, PySparkErrorTestUtils):
Review Comment:
Moved the tests into `python/pyspark/sql/protobuf/functions.py` itself — the `ProtobufFunctionsTests` class now lives inside the `if __name__ == "__main__":` block and runs before the existing doctest-based `_test()`. The separate `test_protobuf.py` and its `modules.py` entry are 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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-43847][PYTHON] Throw structured error when reading Protobuf descriptor file fails [spark]
Posted by "nchammas (via GitHub)" <gi...@apache.org>.
nchammas commented on PR #56174:
URL: https://github.com/apache/spark/pull/56174#issuecomment-5631040586
For this specific problem we are following a precedent established on the Scala side: https://github.com/apache/spark/blob/4dc04afa85a97daa1b8e12ee3f222687fd25f82d/common/utils/src/main/resources/error/error-conditions.json#L6837-L6842
Are you questioning whether Spark exceptions are appropriate to throw from `pyspark.sql.connect.protobuf`?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-43847][PYTHON] Throw structured error when reading Protobuf descriptor file fails [spark]
Posted by "nchammas (via GitHub)" <gi...@apache.org>.
nchammas commented on code in PR #56174:
URL: https://github.com/apache/spark/pull/56174#discussion_r3906470556
##########
python/pyspark/sql/connect/protobuf/functions.py:
##########
@@ -24,6 +24,7 @@
from pyspark.sql.column import Column
from pyspark.sql.connect.functions.builtin import _invoke_function, _options_to_col, _to_col, lit
from pyspark.sql.protobuf import functions as PyProtobufFunctions
+from pyspark.sql.protobuf.functions import _read_descriptor_set_file
Review Comment:
Nit: We've already imported the functions just above. Perhaps we should just use that reference instead of adding a separate import.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-43847][PYTHON] Throw structured error when reading Protobuf descriptor file fails [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #56174:
URL: https://github.com/apache/spark/pull/56174#discussion_r4140926080
##########
python/pyspark/sql/tests/test_protobuf.py:
##########
@@ -0,0 +1,78 @@
+#
+# 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.
+#
+
+import tempfile
+import unittest
+
+from pyspark.errors import PySparkRuntimeError
+from pyspark.sql.protobuf.functions import _read_descriptor_set_file
+from pyspark.testing.connectutils import should_test_connect
+from pyspark.testing.utils import PySparkErrorTestUtils
+
+
+class ProtobufFunctionsTests(unittest.TestCase, PySparkErrorTestUtils):
Review Comment:
Can we find a proper place for this instead of adding a new file?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-43847][PYTHON] Throw structured error when reading Protobuf descriptor file fails [spark]
Posted by "brijrajk (via GitHub)" <gi...@apache.org>.
brijrajk commented on code in PR #56174:
URL: https://github.com/apache/spark/pull/56174#discussion_r4140994123
##########
common/utils/src/main/resources/error/error-conditions.json:
##########
@@ -6919,6 +6919,12 @@
],
"sqlState" : "42K0G"
},
+ "PROTOBUF_DESCRIPTOR_FILE_READ_FAILED" : {
Review Comment:
Got it — will remove the entry from `common/utils/src/main/resources/error/error-conditions.json`. The `PROTOBUF_DESCRIPTOR_FILE_READ_FAILED` condition will only live in `python/pyspark/errors/error-conditions.json` since it is only raised from Python.
##########
python/pyspark/sql/tests/test_protobuf.py:
##########
@@ -0,0 +1,78 @@
+#
+# 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.
+#
+
+import tempfile
+import unittest
+
+from pyspark.errors import PySparkRuntimeError
+from pyspark.sql.protobuf.functions import _read_descriptor_set_file
+from pyspark.testing.connectutils import should_test_connect
+from pyspark.testing.utils import PySparkErrorTestUtils
+
+
+class ProtobufFunctionsTests(unittest.TestCase, PySparkErrorTestUtils):
Review Comment:
Would adding a `ProtobufFunctionsTests` class at the bottom of `python/pyspark/sql/protobuf/functions.py` work? That module is already registered in `dev/sparktestsupport/modules.py` (line 643) and runs via `python/run-tests`, so no new file or modules.py entry would be needed. The `test_protobuf.py` and its modules.py entry would 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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-43847][PYTHON] Throw structured error when reading Protobuf descriptor file fails [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #56174:
URL: https://github.com/apache/spark/pull/56174#discussion_r4141215027
##########
python/pyspark/sql/tests/test_protobuf.py:
##########
@@ -0,0 +1,78 @@
+#
+# 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.
+#
+
+import tempfile
+import unittest
+
+from pyspark.errors import PySparkRuntimeError
+from pyspark.sql.protobuf.functions import _read_descriptor_set_file
+from pyspark.testing.connectutils import should_test_connect
+from pyspark.testing.utils import PySparkErrorTestUtils
+
+
+class ProtobufFunctionsTests(unittest.TestCase, PySparkErrorTestUtils):
Review Comment:
Yeah. I am asking this cuz if you add new file, you would need to add another file like test_connect_parity_*.py to reuse this test (for testing Spark Connect). Just finding existing place would be much easier and simpler.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-43847][PYTHON] Throw structured error when reading Protobuf descriptor file fails [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #56174:
URL: https://github.com/apache/spark/pull/56174#discussion_r4140923118
##########
common/utils/src/main/resources/error/error-conditions.json:
##########
@@ -6919,6 +6919,12 @@
],
"sqlState" : "42K0G"
},
+ "PROTOBUF_DESCRIPTOR_FILE_READ_FAILED" : {
Review Comment:
I think you don't need change 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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-43847][PYTHON] Throw structured error when reading Protobuf descriptor file fails [spark]
Posted by "brijrajk (via GitHub)" <gi...@apache.org>.
brijrajk commented on PR #56174:
URL: https://github.com/apache/spark/pull/56174#issuecomment-5903272332
Folded in the changes from #59142 per @HyukjinKwon's suggestion. The squashed commit now includes:
- `OSError` catch block -> `PROTOBUF_DESCRIPTOR_FILE_READ_FAILED` (new error condition) in addition to `FileNotFoundError` -> `PROTOBUF_DESCRIPTOR_FILE_NOT_FOUND`
- Connect module `_read_descriptor_set_file` delegates to the classic module (no duplicate `open()` call)
- Two additional test cases: `test_read_descriptor_set_file_read_error` and `test_descriptor_file_read_error_connect`
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-43847][PYTHON] Throw structured error when reading Protobuf descriptor file fails [spark]
Posted by "brijrajk (via GitHub)" <gi...@apache.org>.
brijrajk commented on PR #56174:
URL: https://github.com/apache/spark/pull/56174#issuecomment-5496650639
Thanks for the review @nchammas! Both points addressed, rebased onto master and squashed into a single commit.
- **`OSError` -> `FileNotFoundError`**: agreed, mapping every `OSError` to "descriptor file not found" was wrong. Only `FileNotFoundError` maps to `PROTOBUF_DESCRIPTOR_FILE_NOT_FOUND` now.
- **Connect path**: you were right, and the reason I missed it is that `pyspark.sql.connect.protobuf.functions` had its own duplicate copy of `_read_descriptor_set_file()`. Rather than patch the copy, I deleted it and made the Connect module import the Classic implementation, so there is a single implementation and both paths raise the same structured error.
The test now runs the same assertion against both modules, with the Connect case guarded by `should_test_connect`. Both pass locally:
```
test_read_descriptor_set_file_not_found ... ok
test_read_descriptor_set_file_not_found_connect ... ok
Ran 2 tests in 0.005s
OK
```
PR description updated to cover the Connect change.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-43847][PYTHON] Throw structured error when reading Protobuf descriptor file fails [spark]
Posted by "gaogaotiantian (via GitHub)" <gi...@apache.org>.
gaogaotiantian commented on PR #56174:
URL: https://github.com/apache/spark/pull/56174#issuecomment-5879829657
If there's a TODO then I'm good with having a pyspark exception. In this specific case, maybe there are some other native exceptions we can catch to convert to pyspark exception - but that's out of the scope of this 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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-43847][PYTHON] Throw structured error when reading Protobuf descriptor file fails [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #56174:
URL: https://github.com/apache/spark/pull/56174#issuecomment-5901892464
why don't we just fold the change into here in one 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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-43847][PYTHON] Throw structured error when reading Protobuf descriptor file fails [spark]
Posted by "brijrajk (via GitHub)" <gi...@apache.org>.
brijrajk commented on PR #56174:
URL: https://github.com/apache/spark/pull/56174#issuecomment-4582222015
Could a committer please review this? It raises a structured `PySparkRuntimeError` with error class `PROTOBUF_DESCRIPTOR_FILE_NOT_FOUND` instead of a bare `FileNotFoundError` when the descriptor file path is invalid — consistent with the rest of the PySpark error framework (SPARK-43847).
cc @HyukjinKwon @LuciferYang
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-43847][PYTHON] Throw structured error when reading Protobuf descriptor file fails [spark]
Posted by "brijrajk (via GitHub)" <gi...@apache.org>.
brijrajk commented on code in PR #56174:
URL: https://github.com/apache/spark/pull/56174#discussion_r3905895172
##########
python/pyspark/sql/protobuf/functions.py:
##########
@@ -286,9 +287,14 @@ def to_protobuf(
def _read_descriptor_set_file(filePath: str) -> bytes:
- # TODO(SPARK-43847): Throw structured errors like "PROTOBUF_DESCRIPTOR_FILE_NOT_FOUND" etc.
- with open(filePath, "rb") as f:
- return f.read()
+ try:
+ with open(filePath, "rb") as f:
+ return f.read()
+ except OSError as e:
Review Comment:
Good catch, fixed. Narrowed to `FileNotFoundError` so `PermissionError`, `IsADirectoryError`, and the rest keep propagating unchanged.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-43847][PYTHON] Throw structured error when reading Protobuf descriptor file fails [spark]
Posted by "nchammas (via GitHub)" <gi...@apache.org>.
nchammas commented on PR #56174:
URL: https://github.com/apache/spark/pull/56174#issuecomment-5639822660
I don't have a sense of whether the lack of PySpark exceptions in this area is just an accident of history, or some quasi-intentional choice. @HyukjinKwon?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-43847][PYTHON] Throw structured error when reading Protobuf descriptor file fails [spark]
Posted by "brijrajk (via GitHub)" <gi...@apache.org>.
brijrajk commented on PR #56174:
URL: https://github.com/apache/spark/pull/56174#issuecomment-4925957374
Could a committer please review this? It raises a structured `PySparkRuntimeError` with error class `PROTOBUF_DESCRIPTOR_FILE_NOT_FOUND` instead of a bare `FileNotFoundError` when the descriptor file path is invalid — consistent with the rest of the PySpark error framework (SPARK-43847).
cc @HyukjinKwon @itholic
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-43847][PYTHON] Throw structured error when reading Protobuf descriptor file fails [spark]
Posted by "brijrajk (via GitHub)" <gi...@apache.org>.
brijrajk commented on PR #56174:
URL: https://github.com/apache/spark/pull/56174#issuecomment-5837713364
To help answer @nchammas's question: the TODO comment in the current `master` at [functions.py:291](https://github.com/apache/spark/blob/master/python/pyspark/sql/protobuf/functions.py#L291) reads:
```python
# TODO(SPARK-43847): Throw structured errors like "PROTOBUF_DESCRIPTOR_FILE_NOT_FOUND" etc.
```
So the intent to raise structured errors here was explicitly recorded when the function was written — this PR is implementing that TODO rather than introducing a new direction. That makes the "accident of history vs intentional choice" question a bit clearer: the absence was always meant to be temporary.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-43847][PYTHON] Throw structured error when reading Protobuf descriptor file fails [spark]
Posted by "brijrajk (via GitHub)" <gi...@apache.org>.
brijrajk commented on PR #56174:
URL: https://github.com/apache/spark/pull/56174#issuecomment-5895426045
Follow-up PR #59142 ([SPARK-59872](https://issues.apache.org/jira/browse/SPARK-59872)) extends this work to cover all `OSError` subclasses (not just `FileNotFoundError`) and adds a new `PROTOBUF_DESCRIPTOR_FILE_READ_FAILED` error condition for cases like `PermissionError` and `IsADirectoryError`. It also eliminates the duplicate `open()` call in the Connect module by delegating to the classic module. Mentioning here so reviewers of this PR are aware — the two can be reviewed/merged independently.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-43847][PYTHON] Throw structured error when reading Protobuf descriptor file fails [spark]
Posted by "nchammas (via GitHub)" <gi...@apache.org>.
nchammas commented on code in PR #56174:
URL: https://github.com/apache/spark/pull/56174#discussion_r3904664123
##########
python/pyspark/sql/protobuf/functions.py:
##########
@@ -286,9 +287,14 @@ def to_protobuf(
def _read_descriptor_set_file(filePath: str) -> bytes:
- # TODO(SPARK-43847): Throw structured errors like "PROTOBUF_DESCRIPTOR_FILE_NOT_FOUND" etc.
- with open(filePath, "rb") as f:
- return f.read()
+ try:
+ with open(filePath, "rb") as f:
+ return f.read()
+ except OSError as e:
Review Comment:
There are [lots of reasons](https://docs.python.org/3.14/library/exceptions.html#os-exceptions) a program may raise `OSError`. I don't think it's correct to map them all to "protobuf descriptor file not found". You probably want `FileNotFoundError`.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-43847][PYTHON] Throw structured error when reading Protobuf descriptor file fails [spark]
Posted by "brijrajk (via GitHub)" <gi...@apache.org>.
brijrajk commented on PR #56174:
URL: https://github.com/apache/spark/pull/56174#issuecomment-5893225644
Thanks @nchammas and @gaogaotiantian! Happy to follow up with a `PROTOBUF_DESCRIPTOR_FILE_READ_FAILED` error condition for `PermissionError` and similar cases in a separate 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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-43847][PYTHON] Throw structured error when reading Protobuf descriptor file fails [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #56174:
URL: https://github.com/apache/spark/pull/56174#discussion_r4142762092
##########
common/utils/src/main/resources/error/error-conditions.json:
##########
@@ -6919,6 +6919,12 @@
],
"sqlState" : "42K0G"
},
+ "PROTOBUF_DESCRIPTOR_FILE_READ_FAILED" : {
Review Comment:
Agree. I think we should actually add it there when we fix it in Scala side.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-43847][PYTHON] Throw structured error when reading Protobuf descriptor file fails [spark]
Posted by "brijrajk (via GitHub)" <gi...@apache.org>.
brijrajk commented on code in PR #56174:
URL: https://github.com/apache/spark/pull/56174#discussion_r3906501753
##########
python/pyspark/sql/connect/protobuf/functions.py:
##########
@@ -24,6 +24,7 @@
from pyspark.sql.column import Column
from pyspark.sql.connect.functions.builtin import _invoke_function, _options_to_col, _to_col, lit
from pyspark.sql.protobuf import functions as PyProtobufFunctions
+from pyspark.sql.protobuf.functions import _read_descriptor_set_file
Review Comment:
Done, dropped the extra import and used the existing `PyProtobufFunctions` reference.
That removed the symbol the Connect test was importing, so I reworked the test to call the real Connect `from_protobuf` / `to_protobuf` with a bogus `descFilePath` instead of the private helper. The descriptor file is read before the session is touched, so it needs no server, and it now covers the actual public entry points.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-43847][PYTHON] Throw structured error when reading Protobuf descriptor file fails [spark]
Posted by "gaogaotiantian (via GitHub)" <gi...@apache.org>.
gaogaotiantian commented on PR #56174:
URL: https://github.com/apache/spark/pull/56174#issuecomment-5631345457
Yes, I was wondering if raising a pyspark exception is better than what we have now. Matching the scala behavior could be a motivation, but is the scenario the same? I did not check the scala side code. We don't raise any pyspark exception in `pyspark.sql.connect.protobuf` and that is a kind of "consistency" - not saying that it's the correct one. That's the part I need some convincing.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-43847][PYTHON] Throw structured error when reading Protobuf descriptor file fails [spark]
Posted by "gaogaotiantian (via GitHub)" <gi...@apache.org>.
gaogaotiantian commented on PR #56174:
URL: https://github.com/apache/spark/pull/56174#issuecomment-5624826547
Can we take a step back and discuss why this is better than the existing solution? I mean `protobuf/functions.py` python native exceptions in other functions as well - why do we believe it should raise spark native exceptions?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-43847][PYTHON] Throw structured error when reading Protobuf descriptor file fails [spark]
Posted by "brijrajk (via GitHub)" <gi...@apache.org>.
brijrajk commented on code in PR #56174:
URL: https://github.com/apache/spark/pull/56174#discussion_r4140994123
##########
common/utils/src/main/resources/error/error-conditions.json:
##########
@@ -6919,6 +6919,12 @@
],
"sqlState" : "42K0G"
},
+ "PROTOBUF_DESCRIPTOR_FILE_READ_FAILED" : {
Review Comment:
Got it, will remove the entry from `common/utils/src/main/resources/error/error-conditions.json`. The `PROTOBUF_DESCRIPTOR_FILE_READ_FAILED` condition will only live in `python/pyspark/errors/error-conditions.json` since it is only raised from Python.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-43847][PYTHON] Throw structured error when reading Protobuf descriptor file fails [spark]
Posted by "nchammas (via GitHub)" <gi...@apache.org>.
nchammas commented on PR #56174:
URL: https://github.com/apache/spark/pull/56174#issuecomment-5867302809
Oh right, the TODO (removed in this PR) answers the question of intent. And I take @HyukjinKwon's approval as confirmation that this is the right direction to go in.
Unless @gaogaotiantian has any further concerns, I'll merge this in this week.
@brijrajk - If you're interested in this area, it would make sense in a future PR to address the other file read failures like `PermissionError` and perhaps map them to a new `PROTOBUF_DESCRIPTOR_FILE_READ_FAILED` error condition specific to protobuf. (On the Scala side other read failures are currently mapped to `CANNOT_PARSE_PROTOBUF_DESCRIPTOR`, which I don't think is ideal.)
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org