[SYNPY-1749]Allow quote, apostrophe and ellipsis in store_row_async#1316
[SYNPY-1749]Allow quote, apostrophe and ellipsis in store_row_async#1316
Conversation
|
@danlu1 Is this still WIP, or are you looking for reviews? |
|
@andrewelamb sorry I should have marked this a draft. |
…ctly when upload data from a dataframe
…ger output json string
|
The integration test failures are in the recordset and submission modules and do not appear to be related to my changes. |
linglp
left a comment
There was a problem hiding this comment.
I think overall it looks good. The tests can be consolidated a bit to test all the edge cases in fewer integration tests to improve performance, and the docstring can be updated to reflect the new state of the code since json.dumps() was removed. There's also some logic that can be simplified in the redundant checks where sample_values is created but never actually used. The function name could also be more descriptive of what it actually does now (sanitizing special values rather than just converting dtypes)
tests/integration/synapseclient/models/async/test_table_async.py
Outdated
Show resolved
Hide resolved
tests/integration/synapseclient/models/synchronous/test_table.py
Outdated
Show resolved
Hide resolved
BryanFauble
left a comment
There was a problem hiding this comment.
This is looking great, once we get the last few items handled (and develop merged in), I can approve!
BryanFauble
left a comment
There was a problem hiding this comment.
Nice work on this fix -- the backslash-escaping approach for embedded quotes makes sense, and the Ellipsis/pd.NA handling is solid. I flagged one issue that I think needs to be addressed before merge, plus a few cleanup nits.
Note: This comment was drafted with AI assistance and reviewed by me for accuracy.
Problem:
A JSON serialization issue occurs when a DataFrame passed to store_row_async contains a list or dictionary with strings that include both double quotes and apostrophes.
Solution:
"escapechar": "\\"toto_csv_kwargsinstore_rows_asyncto_csv_kwargsto_stream_and_update_from_dfso it can take the passedto_csv_kwargsvalues for downstream data processing.Testing:
Unit test and integration test have been added.