feat: expand new chart data endpoint coverage #9903
Conversation
Codecov Report
@@ Coverage Diff @@
## master #9903 +/- ##
==========================================
+ Coverage 71.35% 71.37% +0.01%
==========================================
Files 585 585
Lines 30889 30913 +24
Branches 3227 3246 +19
==========================================
+ Hits 22041 22063 +22
- Misses 8739 8741 +2
Partials 109 109
Continue to review full report at Codecov.
|
villebro
force-pushed
the
villebro/csv-chart-data
branch
4 times, most recently
from
3784018
to
3b01cac
Compare
villebro
force-pushed
the
villebro/csv-chart-data
branch
from
3b01cac
to
f362d29
Compare
villebro
changed the title
villebro
changed the title
| @@ -109,6 +109,7 @@ def _try_json_readsha(filepath, length): # pylint: disable=unused-argument | |||
|
|
|||
| ROW_LIMIT = 50000 | |||
| VIZ_ROW_LIMIT = 10000 | |||
| SAMPLES_ROW_LIMIT = 1000 | |||
| dashboardId, | ||
| requestParams, | ||
| ) | ||
| : v1ChartDataRequest(formData, force, requestParams); |
There was a problem hiding this comment.
I set up legacyChartDataRequest and v1ChartDataRequest to have different function signatures on purpose. I think it's better to keep v1ChartDataRequest simpler rather than conforming to a legacy set of parameters.
Feel free to keep the change if you prefer. Just wanted to state my original intent there.
There was a problem hiding this comment.
Good point.. The original reason I harmonized the signatures is that the new request function ended up requiring additional parameters that were in line with the legacy request (force and requestParams), so I ended up throwing method in there to make the signatures equal. But you're completely right that there's really no reason why they need to be equal, and going forward I anticipate the next version will probably not be backwards compatible with this request. So I'll either drop method from the new request or add a comment to explain that they've been made equal for convenience purposes only, and that it isn't strictly required.
There was a problem hiding this comment.
I agree with your assessment @suddjian , no point in making the signatures identical. I dropped method from v1ChartDataRequest to make sure we don't unnecessarily hang on to legacy constructs in the codebase.
| // new API returns an object with an array of restults | ||
| // problem: json holds a list of results, when before we were just getting one result. | ||
| // problem: response holds a list of results, when before we were just getting one result. | ||
| // `queryResponse` state is used all over the place. |
There was a problem hiding this comment.
shoud this queryResponse be changed to chartDataResponse to keep things in step?
villebro
force-pushed
the
villebro/csv-chart-data
branch
from
bfc6bfe
to
4e48e6f
Compare
* feat: implement new chart API for additional components * Fix python tests * Fix tests * Fix lint * fix camel case error in requestParams * lint * fix samples row limit * Add samples row limit to config * remove unnecessary code * lint * Address review comments
mistercrunch
added
🏷️ bot
SUMMARY
Currently only the legacy chart data endpoint is supported for the following operations:
This PR hides the legacy/non-legacy chart data request logic behind a single function
getChartDataRequest, which determines whether the legacy/superset/explore_jsonor new/api/v1/chart/dataendpoint should be used, and adds support for requesting samples, raw json results, CSV and SQL Query toQueryContext.TEST PLAN
CI + local testing
ADDITIONAL INFORMATION