[WIP] [SIP-6] Remove backend logic of tablevis.#6667
Conversation
conglei
changed the title
| if (formData.viz_type === 'word_cloud' && !forceExplore) { | ||
| directory = '/api/v1/query/'; | ||
| if (formData.viz_type === 'word_cloud' || | ||
| formData.viz_type === 'table' && |
There was a problem hiding this comment.
Perhaps worthwhile defining an array of viz_types to "whitelist" for the query endpoint and checking that the viz_type from form_data is in that array (since we'll be adding new ones soon)?
kristw
changed the title
| @handle_api_exception | ||
| @has_access_api | ||
| @expose('/v1/query', methods=['POST']) | ||
| @expose('/v1/query/', methods=['POST']) |
There was a problem hiding this comment.
hmm, I thought we decided to drop the / to avoid calling the endpoint with /v1/query/?slice_id..., no?
| @handle_api_exception | ||
| @has_access_api | ||
| @expose('/v1/form_data', methods=['GET']) | ||
| @expose('/v1/form_data/', methods=['GET']) |
|
|
||
| if query_obj and not is_loaded: | ||
| try: | ||
| print(query_obj) |
| } | ||
|
|
||
| export enum MetricsKey { | ||
| METRICS = 'metrics', |
There was a problem hiding this comment.
I see. So when the keys are either metrics or percent_metrics, the values are always arrays?
| groupby?: string[]; | ||
| columns?: string[]; | ||
| all_columns?: string[]; | ||
| limit?: string; |
There was a problem hiding this comment.
should this be number instead of string?
| columns?: string[]; | ||
| all_columns?: string[]; | ||
| limit?: string; | ||
| row_limit: string; |
There was a problem hiding this comment.
ditto. Should this be number?
| } & BaseFormData; | ||
|
|
||
| type FormData = SqlaFormData | DruidFormData; | ||
| type FormData = BaseFormData & SqlaFormData & DruidFormData; |
There was a problem hiding this comment.
Why &? Shouldn't a data source be either SqlAlchemy or Druid? 🤔 Also either type will "inherit" the command fields from BaseFormData. It seems the type of FormData should stay the same as before, no?
| granularity: string; | ||
| groupby?: string[]; | ||
| metrics?: Metric[]; | ||
| groupby: string[]; |
There was a problem hiding this comment.
Not all of these fields are required as this interface should accommodate all query objects (e.g. groupby is optional in the query object for word cloud). I think a good way to type this is to match a given field with whether it is required in the api signature on the server side.
| // specific viz, which is a subtype of the generic formData shared among all viz types. | ||
| export default function buildQueryObject<T extends FormData>(formData: T): QueryObject { | ||
| const extras = { | ||
| druid_time_origin: formData.druid_time_origin || '', |
There was a problem hiding this comment.
Are all these fields in extras required?
| import { FormData as GenericFormData } from 'src/query'; | ||
| import { RawMetric } from 'src/query/Metric'; | ||
|
|
||
| // FormData specific to the wordcloud viz |
| all_columns: string[]; | ||
| percent_metrics: RawMetric[]; | ||
| include_time: boolean; | ||
| order_by_cols: any[]; |
There was a problem hiding this comment.
Can we type this without any?
| this.metrics = []; | ||
| for (const key of Object.keys(MetricKey)) { | ||
| const metric = formData[MetricKey[key] as MetricKey]; | ||
| let metric = formData[MetricKey[key] as MetricKey]; |
There was a problem hiding this comment.
Would recommend using const unless we're reassigning metric.
| static convertMetric(metric: RawMetric) { | ||
| if (!metric) { | ||
| return null; | ||
| } if (typeof metric === 'string') { |
| } | ||
| } | ||
| for (const key of Object.keys(MetricsKey)) { | ||
| let metrics = formData[MetricsKey[key] as MetricsKey]; |
There was a problem hiding this comment.
Would recommend using const here instead of let as well.
|
|
||
| export default function buildQuery(formData: FormData) { | ||
| // Set the single QueryObject's groupby field with series in formData | ||
| return buildQueryContext(formData, (baseQueryObject) => { |
There was a problem hiding this comment.
Haven't reviewed the new buildQuery for table view. Can take another pass after we address the type related comments above.
| @@ -1,3 +1,55 @@ | |||
| const DTTM_ALIAS = '__timestamp'; | |||
There was a problem hiding this comment.
Can we port this file to TypeScript? 🙂
xtinec
force-pushed
the
feature--embeddable-charts-pilot
branch
from
c9c6ff1 to
f59594b
Compare
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
3 participants
This PR is to move the backend logic of TableVis to frontend.
move
get_datato frontend.add more query object fields in frontend.
move 'query_object' to frontend.
@xtinec