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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
