[bugfix] Fixing regression introduced in #4396

[john-bodley]

This PR fixes a regression introduced in #4396 which returned the error No data even when the query was unsuccessful for other reasons. This PR also refactors the No data logic (previously defined in two places) and also registered that the data was loaded if the query didn't fail. Note it seems like BaseViz.get_df(...) may never throw an exception give this (and thus I'm uncertain whether this logic is needed) hence the additional check that the query succeeded is necessary to confirm that the data was loaded.

@mistercrunch from our offline conversation I found this difficult to write specific test cases, i.e., we discovered this regression in Presto when the underlying table metadata had changed either a column or the table no-longer existed. When I tried to mock this behavior, I was able to exercise the issue as in SQLite the inconsistency was discovered whilst the query was being compiled rather than during query execution.

to: @mistercrunch
cc: @graceguo-supercat @michellethomas @timifasubaa

[john-bodley]

The try/except block may no longer be required per the PR description. This would make the stacktrace obsolete as well.

[john-bodley]

Note I'm not certain why we need to throw an exception here, as previously the error would be present in error_message.

[mistercrunch]

I feel like we need some unit tests here making sure errors bubble up somehow. It should be easy to generate a No Data exception with a filter. For the other test case that is more representative (currently says No Data but should say something else) you could reference a metric that does not exist in the query, and change the query method to make sure it actually raises a specific message Metric referenced does not exist anymore and make sure it comes through.

[john-bodley]

@mistercrunch for the later simply referencing an invalid metric doesn't actually cover the specific case we discovered as the compiler detects this, i.e., prior to this fix it wouldn't return No data. I can look more into this.

[john-bodley]

@mistercrunch I've updated the logic and added a couple of unit tests which test the two scenarios.

[john-bodley]

Note that the df.empty check is only required by the test_get_df_returns_empty_df unit test due to the mocking. Otherwise it would be safe to proceed with an empty pd.DataFrame.

[john-bodley]

It's more consistent to log the payload error as opposed to throwing an exception here.

[john-bodley]

I added this for consistency, i.e. get_df(...) should return a pd.DataFrame().

[john-bodley]

@mistercrunch would you mind taking another look at this? I made a few small tweaks after adding the addition of a couple of unit tests.

[michellethomas]

lgtm

[mistercrunch]

Sorry for the delay, was traveling last week with limited attention. Thanks @michellethomas for merging it!

[mistercrunch]

I think this broke the brittle markup viz in trunk. Has to do with the fact that the payload gets a error key that says No data and the frontend then refuses to render the viz.

[john-bodley]

Sorry @mistercrunch. I think I may have a fix and will add an additional unit test for the markup test. I think the solution should be:

class MarkupViz(BaseViz):
    def get_df(self, query_obj=None):
       return None  # like before

and

def get_payload(self, query_obj=None):
    ...
    if df is not None and df.empty:  # previously if df is None or df.empty
        payload['error'] = 'No data'

[mistercrunch]

Sounds about right, this area is pretty brittle around the no-query special cases...