fix(python-cdk): add user friendly message for encoding errors #44438
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
octavia-squidington-iii
added
area/connectors
| @@ -54,7 +55,7 @@ def state(self, value: MutableMapping[str, Any]) -> None: | |||
| """State setter, accept state serialized by state getter.""" | |||
| self._cursor.set_initial_state(value) | |||
|
|
|||
| @property | |||
| @property # type: ignore # no suitable parent cursor type | |||
There was a problem hiding this comment.
why is it not suitable / why can't the typing error be fixed?
There was a problem hiding this comment.
I looked at several of the Cursor offered by the IDE, and they didn't seem right. The only Cursor object that fixed the problem was the GCS Cursor, but this is a more generic file as it belongs to the CDK and not the specific GCS source.
There was a problem hiding this comment.
what's the error exactly? Is the GCS cursor not an AbstractFileBasedCursor?
There was a problem hiding this comment.
The problem is that GCS cursor is not generic, as this file belongs to the CDK. Error is error: Signature of "cursor" incompatible with supertype "Stream" [override]
There was a problem hiding this comment.
What’s the expected type from the parent class? Is it really a mismatch? If not, can the comment be more clear about why this is a mypy problem, not with the type
There was a problem hiding this comment.
It's not a mismatch. gcs.Cursor inherits from DefualtFileBasedCursor
, which inherits from AbstractFileBasedCursor. Improved comment.
airbyte-cdk/python/airbyte_cdk/sources/file_based/exceptions.py
Outdated
Show resolved
Hide resolved
|
New error message is correctly displayed. In this example, utf16 was used on a file encoded with utf8: |
airbyte-cdk/python/airbyte_cdk/sources/file_based/file_based_source.py
Outdated
Show resolved
Hide resolved
…rce.py Removed test file to be added in a second PR
…sek/gcs_decode_error_issue_8952`) Certainly! To optimize the code, one can focus on multiple aspects like reducing the overhead of unnecessary checks, avoiding redundant operations, efficient use of data structures, and minimizing log string concatenations. Here is the optimized version of your given code. ### Optimization Highlights. 1. **Concise Manual Validation**: Combined `quote_char`, `escape_char` validation into a single validator. 2. **Helper Functions**: Utilized a generalized helper function to avoid repetition and handle specific validations concisely. 3. **Import Statements Simplified**: Removed unnecessary imports and rearranged for better readability. 4. **Combined Error Logging**: Concatenated log messages in a single operation to minimize the overhead. 5. **Proper Use of Pythonic Constructs**: Optimized type casting while skipping unnecessary checks and directly handling edge cases. This optimized version should maintain the functionality while potentially improving performance due to fewer and more efficient operations.
⚡️ Codeflash found optimizations for this PR📄
|
| with pytest.raises(expected_exc) as exc: | ||
| check(capsys, tmp_path, scenario) | ||
|
|
||
| if expected_msg: |
There was a problem hiding this comment.
This block was never executed because previous block exited with the exception. Also, output was None, since the code never returned. Tests should be more thorough now.
|
/approve-regression-tests
|
| @@ -520,6 +520,17 @@ def test_parse_field_size_larger_than_default_python_maximum(self) -> None: | |||
| data_generator = self._read_data() | |||
| assert list(data_generator) == [{"header1": "1", "header2": long_string}] | |||
|
|
|||
| def test_read_data_with_encoding_error(self) -> None: | |||
What
Make it clearer when a CSV file has problems in discover/read because of a file encoding mismatch.
This also improves general error display in the UI thanks to catching
AirbyteTracedExceptionsand building a readable tracemessagewith the concatenation of each ATE's message.How
Catching the UnicodeError and throwing an easier to read exception message.
Review guide
csv_parser.py- added try-except for creating a DecodingError from the UnicodeError. Also fixed a mypy error.exceptions.py- added friendlier error message and DecodingError alias.default_file_based_stream.py- fix exception throwing chain, clean duplicate log entriesdefault_file_based_availability_strategy.py- fix exception throwing chain, raise ATE.User Impact
The user will have more clarity on why a decoding error happens, and how to fix it.
Can this PR be safely reverted and rolled back?