LuisSanchez commented on PR #37791: URL: https://github.com/apache/superset/pull/37791#issuecomment-6327950549
> Checked `parseCSV` directly since it's central to what this feature does, and the bot's right to flag it: `text.split(/\r?\n/)` splits on every newline including ones inside a quoted field, and `row.split(delimiter)` does the same for the delimiter, so any real-world CSV with a quoted field containing a comma or an embedded newline (extremely common in exports from Excel/Sheets/databases) will mis-parse into the wrong number of columns. Worth switching to a real CSV parser rather than hand-rolled splitting, especially since this is a preview meant to build user trust before they commit to the upload. > > The race-condition point (stale async parse/metadata response overwriting newer state after a fast file swap or modal close) showed up independently across three different threads with the same mechanism each time, which is a good sign it's real. Worth an effect cleanup/abort guard. > > No human review yet after 8 months and 7 open threads. The BOM-stripping and Excel-test-coverage ones are also fair asks, just didn't verify those myself. Hey! Sorry that I missed the previous comment, I can rebase and verify if this is still worth the new dependency or if there is something already in the repo. -- 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]
