Skip to content
This repository has been archived by the owner on Dec 10, 2021. It is now read-only.

feat: 🎸 Improved QueryObject to handle more fields #116

Merged
merged 3 commits into from
Mar 12, 2019

Conversation

conglei
Copy link
Contributor

@conglei conglei commented Mar 7, 2019

🏆 Enhancements

The commit is to ensure the feature parity between frontend and backend QueryOjbect

The commit is to ensure the feature parity between frontend and backend
QueryOjbect
@conglei conglei requested a review from xtinec March 7, 2019 21:54
@conglei conglei requested a review from a team as a code owner March 7, 2019 21:54
@codecov
Copy link

codecov bot commented Mar 7, 2019

Codecov Report

Merging #116 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #116   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          74     74           
  Lines         898    923   +25     
  Branches      211    224   +13     
=====================================
+ Hits          898    923   +25
Impacted Files Coverage Δ
...es/superset-ui-chart/src/query/buildQueryObject.ts 100% <100%> (ø) ⬆️
packages/superset-ui-chart/src/query/Metrics.ts 100% <100%> (ø) ⬆️
...es/superset-ui-dimension/src/computeMaxFontSize.ts 100% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 15effbe...f4c61c4. Read the comment docs.

Copy link
Contributor

@williaster williaster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good overall! left a couple thoughts 💬

also maybe should add a test for coverage? I think we can ignore the patch hit diff being <100% but keeping total coverage near 100% I think is our goal / ideal.

@@ -29,24 +29,31 @@ export default class Metrics {
return this.metrics.map(m => m.label);
}

private addMetric(metric: FormDataMetric) {
static convertMetric(metric: FormDataMetric): Metric {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we call this cleanMetric or something more descriptive? at first glance it's not clear what it's converting to or from

row_limit?: number;
order_desc?: boolean;
is_timeseries?: boolean;
prequeries?: any[];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can prequeries, extras, and orderby by something besides any?

where: formData.where || '',
};

const orgGroupby = formData.groupby || [];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure what org meant for these vars initially, could just extract like

const { columns = [], groupby = [] } = formData;

// OR
const groupbySet = new Set([ ...formData.columns, ...formData.groupby ]);

const orgGroupby = formData.groupby || [];
const orgColumns = formData.columns || [];
const groupbySet = new Set(orgGroupby.concat(orgColumns));
const limit = formData.limit ? Number(formData.limit) : 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does limit=0 have any weird query side effects?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the original logic from viz.py:

limit = int(form_data.get('limit') or 0)

@conglei
Copy link
Contributor Author

conglei commented Mar 12, 2019

back too 100% !

@conglei
Copy link
Contributor Author

conglei commented Mar 12, 2019

should I merge this PR? @williaster

Copy link
Contributor

@kristw kristw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants