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

fix(core): don't add metrics to query object when in raw records mode#995

Merged
ktmud merged 6 commits intomasterfrom
raw-records-no-metrics
Mar 10, 2021
Merged

fix(core): don't add metrics to query object when in raw records mode#995
ktmud merged 6 commits intomasterfrom
raw-records-no-metrics

Conversation

@ktmud
Copy link
Contributor

@ktmud ktmud commented Mar 8, 2021

🏆 Enhancements
🐛 Bug Fix

Don't set metrics in QueryObject is it is in raw records mode. Then we can assume it is an aggregation query as long as metrics is defined, even if it's just empty array.

This allows a clean deprecation of the groupby field.

See discussions in apache/superset#13434 for details.

@vercel
Copy link

vercel bot commented Mar 8, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/superset/superset-ui/3Wd5Hz3n77VcSgo6aZV56gjv9qpV
✅ Preview: https://superset-ui-git-raw-records-no-metrics-superset.vercel.app

@codecov
Copy link

codecov bot commented Mar 8, 2021

Codecov Report

Merging #995 (b2e158b) into master (099dc5a) will decrease coverage by 0.00%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #995      +/-   ##
==========================================
- Coverage   27.90%   27.90%   -0.01%     
==========================================
  Files         413      414       +1     
  Lines        8510     8512       +2     
  Branches     1199     1205       +6     
==========================================
  Hits         2375     2375              
- Misses       5982     5983       +1     
- Partials      153      154       +1     
Impacted Files Coverage Δ
...chart-controls/src/shared-controls/dndControls.tsx 31.25% <0.00%> (-4.05%) ⬇️
...et-ui-chart-controls/src/shared-controls/index.tsx 41.93% <0.00%> (-0.62%) ⬇️
.../superset-ui-core/src/query/types/QueryFormData.ts 100.00% <ø> (ø)
...ins/plugin-chart-echarts/src/BoxPlot/buildQuery.ts 40.00% <0.00%> (-6.67%) ⬇️
.../plugin-chart-echarts/src/Timeseries/buildQuery.ts 37.50% <0.00%> (-12.50%) ⬇️
...ckages/superset-ui-chart-controls/src/constants.ts 100.00% <100.00%> (ø)
...s/superset-ui-core/src/query/extractQueryFields.ts 100.00% <100.00%> (ø)
...kages/superset-ui-core/src/query/getMetricLabel.ts 100.00% <100.00%> (ø)
packages/superset-ui-core/test/fixtures.ts 100.00% <100.00%> (ø)
plugins/plugin-chart-table/src/buildQuery.ts 63.04% <100.00%> (ø)
... and 1 more

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 099dc5a...b2e158b. Read the comment docs.

@ktmud ktmud changed the title fix(core): dont set metrics in raw records mode fix(core): don't add metrics to query object when in raw records mode Mar 8, 2021
description?: string | null;
expression?: string | null;
database_expression?: string | null;
python_date_format?: string | null;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Consolidate types from Column and ColumnMeta.

columns: [],
metrics: [],
orderby: [],
orderby: undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we test when metrics and/or orderby are not set too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some existing tests already covered this.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants