[CIVP-24324] FIX remove `_get_headers` in `read_civis_sql`

civisanalytics / civis-python

Civis API Python Client

BSD 3-Clause "New" or "Revised" License

34 stars 26 forks source link

[CIVP-24324] FIX remove `_get_headers` in `read_civis_sql` #415

Closed madopal closed 2 years ago

madopal commented 3 years ago

This PR removes the helper function _get_headers used by civis.io.read_civis_sql.

Originally, @madopal discovered a bug in this helper function. John Kulzick then commented that this helper function should in fact be removed based on the current state of the internal Civis Platform code.

For reference, the following is @madopal 's original PR description:

As it stands now, if an error is tripped in _get_headers, it silently fails. However, a simple fail can be invoked if the sql being checked is terminated with a semicolon. So a strip of the semicolon is added to cover a few more cases & get the optimization of that routine. Because the entire failure is labeled NOQA, tests aren't really possible. However, it was run with some queries to verify it doesn't blow up.

salilgupta1 commented 3 years ago

Sorry, I'm a bit confused. You want to remove semi-colons because it will throw the error or you want to remove the semi-colon because it will further suppress the error?

madopal commented 3 years ago

So, it seems like the benefit of looking up the header can be done regardless of whether or not the query is terminated by a semicolon, no? If so, then we need to either 1) strip the semicolon when we get it and try to get the header or 2) document that if any SQL query is semicolon terminated, it will silently fail & skip the optimization. I had assumed the former, as if it fails it's no worse than how it is now, but if we have a reason for not gaining the benefit of the header when the query is semicolon-terminated, then it should at least be mentioned in the higher level call, right?

salilgupta1 commented 3 years ago

ok, I get what you are saying. I have no opinion since i'm not familiar with this code. @mheilman do you have any thoughts?

jkulzick commented 3 years ago

The _get_headers code should just be removed. It was added at a time when SQL Scripts did not support doing a parallel unload if headers were requested. At that time, rather than fixing it on the API side, it was easier to add this code on the client side. The API side has since been addressed making the _get_headers code redundant. The bug this PR addresses has already been addressed in the API side code.

I'm happy to provide context to things in the Python API client like this, but we do need to expand the bus factor and get other people involved in CR.

madopal commented 3 years ago

Happy to just close this if that's the solution @jkulzick, unless you'd rather I modify it to remove _get_headers?

jkulzick commented 3 years ago

Happy to just close this if that's the solution @jkulzick, unless you'd rather I modify it to remove _get_headers?

@madopal yeah, we should modify this to remove _get_headers

jacksonlee-civis commented 2 years ago

@skomanduri Tagging you for review here, as we'd need someone with knowledge of the Platform code. In particular, is Kulzick's comment still accurate at this point given the current state of Platform? Thank you.

skomanduri commented 2 years ago

@jacksonlee-civis Yep, the comment is still accurate! This logic has been moved into Platform so it can be removed from the client.