Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: count(distinct column_name) in metrics #19842

Merged
merged 1 commit into from
Apr 26, 2022
Merged

fix: count(distinct column_name) in metrics #19842

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 17 additions & 4 deletions superset-frontend/src/explore/components/controls/MetricControl/AdhocMetric.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,10 @@
* specific language governing permissions and limitations
* under the License.
*/
import { sqlaAutoGeneratedMetricRegex } from 'src/explore/constants';
import {
sqlaAutoGeneratedMetricRegex,
AGGREGATES,
} from 'src/explore/constants';

export const EXPRESSION_TYPES = {
SIMPLE: 'SIMPLE',
Expand Down Expand Up @@ -86,20 +89,30 @@ export default class AdhocMetric {
}

getDefaultLabel() {
const label = this.translateToSql(true);
const label = this.translateToSql({ useVerboseName: true });
return label.length < 43 ? label : `${label.substring(0, 40)}...`;
}

translateToSql(useVerboseName = false) {
translateToSql(
params = { useVerboseName: false, transformCountDistinct: false },
) {
if (this.expressionType === EXPRESSION_TYPES.SIMPLE) {
const aggregate = this.aggregate || '';
// eslint-disable-next-line camelcase
const column =
useVerboseName && this.column?.verbose_name
params.useVerboseName && this.column?.verbose_name
? `(${this.column.verbose_name})`
: this.column?.column_name
? `(${this.column.column_name})`
: '';
// transform from `count_distinct(column)` to `count(distinct column)`
if (
params.transformCountDistinct &&
aggregate === AGGREGATES.COUNT_DISTINCT &&
/^\(.*\)$/.test(column)
) {
return `COUNT(DISTINCT ${column.slice(1, -1)})`;
}
Comment on lines 102 to +115
Copy link
Member

@villebro villebro Apr 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I had to reread these lines a few times to understand what was going on (mostly there from before this PR so not your fault!). IMO it would be more readable if we could first do something like

const column =
  params.useVerboseName && this.column?.verbose_name
    this.column.verbose_name
    : this.column?.column_name
    ? this.column.column_name
    : '';

and then something like

if (params.transformCountDistinct && aggregate === AGGREGATES.COUNT_DISTINCT) {
  return `COUNT(DISTINCT ${column})`;
}
return `${aggregate}($column)`;

Copy link
Member Author

Choose a reason for hiding this comment

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

The previous logic may not have been covered by UT, so I skipped these very carefully. I think it would be better to add some UTs to this part of the code and then modify it all together.

Of course, if this needs to be done in this PR, I'm all for it. What do you think about this?

Copy link
Member

@villebro villebro Apr 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Agreed, if you think this is dangerous to refactor let's do it in a separate PR 👍

return aggregate + column;
}
if (this.expressionType === EXPRESSION_TYPES.SQL) {
Expand Down
36 changes: 36 additions & 0 deletions superset-frontend/src/explore/components/controls/MetricControl/AdhocMetric.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,4 +216,40 @@ describe('AdhocMetric', () => {
expect(adhocMetric3.aggregate).toBe(AGGREGATES.AVG);
expect(adhocMetric3.column.column_name).toBe('value');
});

it('should transform count_distinct SQL and do not change label if does not set metric label', () => {
const withBrackets = new AdhocMetric({
column: { type: 'TEXT', column_name: '(column-with-barckets)' },
aggregate: AGGREGATES.COUNT_DISTINCT,
hasCustomLabel: false,
});
expect(withBrackets.translateToSql({ transformCountDistinct: true })).toBe(
'COUNT(DISTINCT (column-with-barckets))',
);
expect(withBrackets.getDefaultLabel()).toBe(
'COUNT_DISTINCT((column-with-barckets))',
);

const withoutBrackets = new AdhocMetric({
column: { type: 'TEXT', column_name: 'column-without-barckets' },
aggregate: AGGREGATES.COUNT_DISTINCT,
hasCustomLabel: false,
});
expect(
withoutBrackets.translateToSql({ transformCountDistinct: true }),
).toBe('COUNT(DISTINCT column-without-barckets)');
expect(withoutBrackets.getDefaultLabel()).toBe(
'COUNT_DISTINCT(column-without-barckets)',
);

const emptyColumnName = new AdhocMetric({
column: { type: 'TEXT', column_name: '' },
aggregate: AGGREGATES.COUNT_DISTINCT,
hasCustomLabel: false,
});
expect(
emptyColumnName.translateToSql({ transformCountDistinct: true }),
).toBe('COUNT_DISTINCT');
expect(emptyColumnName.getDefaultLabel()).toBe('COUNT_DISTINCT');
});
});
5 changes: 4 additions & 1 deletion ...t-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -465,7 +465,10 @@ export default class AdhocMetricEditPopover extends React.PureComponent {
onChange={this.onSqlExpressionChange}
width="100%"
showGutter={false}
value={adhocMetric.sqlExpression || adhocMetric.translateToSql()}
value={
adhocMetric.sqlExpression ||
adhocMetric.translateToSql({ transformCountDistinct: true })
}
editorProps={{ $blockScrolling: true }}
enableLiveAutocompletion
className="filter-sql-editor"
Expand Down