-
Notifications
You must be signed in to change notification settings - Fork 13.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[bugfix] Fixing regression introduced in #4396 #4500
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -151,9 +151,6 @@ def get_df(self, query_obj=None): | |
# If the datetime format is unix, the parse will use the corresponding | ||
# parsing logic. | ||
if df is None or df.empty: | ||
self.status = utils.QueryStatus.FAILED | ||
if not self.error_message: | ||
self.error_message = 'No data.' | ||
return pd.DataFrame() | ||
else: | ||
if DTTM_ALIAS in df.columns: | ||
|
@@ -290,10 +287,11 @@ def get_payload(self, query_obj=None): | |
payload = self.get_df_payload(query_obj) | ||
|
||
df = payload.get('df') | ||
if df is not None and len(df.index) == 0: | ||
raise Exception('No data') | ||
if self.status != utils.QueryStatus.FAILED: | ||
payload['data'] = self.get_data(df) | ||
if df is None or df.empty: | ||
payload['error'] = 'No data' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's more consistent to log the payload error as opposed to throwing an exception here. |
||
else: | ||
payload['data'] = self.get_data(df) | ||
if 'df' in payload: | ||
del payload['df'] | ||
return payload | ||
|
@@ -327,8 +325,9 @@ def get_df_payload(self, query_obj=None): | |
if query_obj and not is_loaded: | ||
try: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The try/except block may no longer be required per the PR description. This would make the |
||
df = self.get_df(query_obj) | ||
stats_logger.incr('loaded_from_source') | ||
is_loaded = True | ||
if self.status != utils.QueryStatus.FAILED: | ||
stats_logger.incr('loaded_from_source') | ||
is_loaded = True | ||
except Exception as e: | ||
logging.exception(e) | ||
if not self.error_message: | ||
|
@@ -612,7 +611,7 @@ def query_obj(self): | |
return None | ||
|
||
def get_df(self, query_obj=None): | ||
return None | ||
return pd.DataFrame() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added this for consistency, i.e. |
||
|
||
def get_data(self, df): | ||
markup_type = self.form_data.get('markup_type') | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that the
df.empty
check is only required by thetest_get_df_returns_empty_df
unit test due to the mocking. Otherwise it would be safe to proceed with an emptypd.DataFrame
.