refactor: get Axis from a helper function #21449
Conversation
zhaoyongjie
changed the title
Codecov Report
@@ Coverage Diff @@
## master #21449 +/- ##
==========================================
- Coverage 66.59% 66.53% -0.06%
==========================================
Files 1791 1792 +1
Lines 68554 68602 +48
Branches 7319 7319
==========================================
- Hits 45653 45647 -6
- Misses 21008 21064 +56
+ Partials 1893 1891 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| export const getAxis = (formData: QueryFormData): string | undefined => { | ||
| // The formData should be "raw form_data" -- the snake_case version of formData rather than camelCase. | ||
| if (!(formData.granularity_sqla || formData.x_axis)) { | ||
| return undefined; | ||
| } | ||
|
|
||
| return isDefined(formData.x_axis) | ||
| ? getColumnLabel(formData.x_axis) | ||
| : DTTM_ALIAS; | ||
| }; |
There was a problem hiding this comment.
To future proof this to n dimensions, I think it might make sense to call this helper getBaseAxes that returns an array (that way we also wouldn't need to wrap the return value in an array when passing it to the index prop). Also, I believe the return type should be based on QueryFormColumn, as it may also be an AdhocColumn. So maybe the sig should be
export const getBaseAxes = (formData: QueryFormData): QueryFormColumn[] | undefined => {...}Also, shouldn't we assume that the return value should always be defined, i.e. we always fall back to DTTM_ALIAS (at least for now) unless formData.x_axis is defined?
There was a problem hiding this comment.
This function returns a label of a column(the label of the columns, or alias clause for the SQL), so the return value might be a string rather than a structure data(QueryFormColumn).
another issue is that whether the return value is a string or a string array. both work for me, but the string array return value might not directly apply to the post processing function. I promise that if we will support the multiple axes, I am going to refactor this.
The undefined value represents a form_data that cannot apply a query with an axis. For example, if a form_data had granularity_sqla neither nor axis, this function would return an undefined value, then the caller should easily skip some logic.
| const xAxisCol = | ||
| verboseMap[xAxisOrig] || getColumnLabel(xAxisOrig || DTTM_ALIAS); | ||
| verboseMap[xAxisOrig] || getAxis(chartProps.rawFormData) || DTTM_ALIAS; |
There was a problem hiding this comment.
If we make getAxis always return a non-undefined value, then we won't need to have the || DTTM_ALIAS here, right? It feels slightly redundant, as it feels like the getAxis helper should always return a value (or raise if it can't).
There was a problem hiding this comment.
Yes, the DTTM_ALIAS is a redundant case. I consider that if a chart can't have an axis here, the chart should throw an error, but there wasn't an appropriate approach to throw an error in the Chart and TS throw a type error, so I added a redundant condition here. I think we need to catch this error in the separated PR.
zhaoyongjie
force-pushed
the
fix/switching_generic_axis
branch
from
b33494d
to
6e8cc96
Compare
mistercrunch
added
🏷️ bot
SUMMARY
Currently, the new version of Charts especially Echart VIZs supported a Time or Categorical column as the X-Axis when the Feature Flag
GENERIC_CHART_AXESwas enabled. This feature would be disabled if FF was disabled, so the Pivot Index in somePost Processing operatorswould be set to__timestampfor backward compatibility.This PR intends to move related codes into a helper function and removed unused property:
is_timeseriesin query_object. Theis_timeseriesonly was used for the legacy Viz endpoint rather than the V1 Viz endpoint, so it's a safe refactor.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION