Skip to content

Commit

Permalink
fix: count(distinct column_name) in metrics (apache#19842)
Browse files Browse the repository at this point in the history
  • Loading branch information
zhaoyongjie authored May 11, 2022
1 parent fbf8e33 commit df8f39a
Show file tree
Hide file tree
Showing 3 changed files with 57 additions and 5 deletions.
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)})`;
}
return aggregate + column;
}
if (this.expressionType === EXPRESSION_TYPES.SQL) {
Expand Down
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');
});
});
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

0 comments on commit df8f39a

Please sign in to comment.