-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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
fix(chart-data-api): support numeric temporal columns #13138
fix(chart-data-api): support numeric temporal columns #13138
Conversation
3def36b
to
5ba5a73
Compare
Codecov Report
@@ Coverage Diff @@
## master #13138 +/- ##
===========================================
+ Coverage 53.06% 66.79% +13.73%
===========================================
Files 489 492 +3
Lines 17314 29026 +11712
Branches 4482 0 -4482
===========================================
+ Hits 9187 19389 +10202
- Misses 8127 9637 +1510
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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.
left a comment.
also not sure if related or not but maybe this should be applied when importing a dataset?
https://github.com/apache/superset/blob/master/superset/datasets/commands/importers/v1/utils.py#L139
or it is only related to when loading the data into a viz
5ba5a73
to
3ea3022
Compare
) -> pd.DataFrame: | ||
if DTTM_ALIAS not in df.columns: | ||
return df | ||
df = df.copy() |
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.
Is the copy
operation necessary here? This could be very expensive on memory for large result sets.
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.
Good point @robdiciuccio . I'm a bit allergic to functions that mutate their inputs (even Pandas is moving away from this pattern), but this could in fact have a noticeable performance hit. I'm going to be touching this code in the coming days, so I'll make sure to address this then (either confirm that the copy is shallow enough to not cause a perf hit or remove the copy operation).
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.
Thanks, @villebro. DataFrame.copy() is deep by default.
SUMMARY
Some database connectors (at least the native Pinot connector) return a numeric value (either as second or millisecord epoch) instead of a regular timestamp. This was working correctly on
viz.py
, but support was lacking on/api/v1/chart/data
. This centralizes the logic in a util, simplifies the code and adds tests that were previously missing.TEST PLAN
CI + new tests
ADDITIONAL INFORMATION