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.

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

fix(explore): bugs in Custom SQL editor in filter popover #12278

Merged
merged 3 commits into from
Jan 6, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ describe('AdhocFilterEditPopoverSimpleTabContent', () => {
comparator: null,
clause: 'WHERE',
expressionType: 'SQL',
sqlExpression: "ds = '{{ presto.latest_partition('schema.table1') }}' ",
sqlExpression: "ds = '{{ presto.latest_partition('schema.table1') }}'",
}),
);
});
Expand Down
28 changes: 28 additions & 0 deletions superset-frontend/spec/javascripts/explore/utils_spec.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import {
getExploreLongUrl,
getDataTablePageSize,
shouldUseLegacyApi,
getSimpleSQLExpression,
} from 'src/explore/exploreUtils';
import {
buildTimeRangeString,
Expand Down Expand Up @@ -298,4 +299,31 @@ describe('exploreUtils', () => {
);
});
});

describe('getSimpleSQLExpression', () => {
const subject = 'subject';
const operator = '=';
const comparator = 'comparator';
it('returns empty string when subject is undefined', () => {
expect(getSimpleSQLExpression(undefined, '=', 10)).toBe('');
expect(getSimpleSQLExpression()).toBe('');
});
it('returns subject when its provided and operator is undefined', () => {
expect(getSimpleSQLExpression(subject, undefined, 10)).toBe(subject);
expect(getSimpleSQLExpression(subject)).toBe(subject);
});
it('returns subject and operator when theyre provided and comparator is undefined', () => {
expect(getSimpleSQLExpression(subject, operator)).toBe(
`${subject} ${operator}`,
);
});
it('returns full expression when subject, operator and comparator are provided', () => {
expect(getSimpleSQLExpression(subject, operator, comparator)).toBe(
`${subject} ${operator} ${comparator}`,
);
expect(getSimpleSQLExpression(subject, operator, comparator, true)).toBe(
`${subject} ${operator} ('${comparator}')`,
);
});
});
});
7 changes: 3 additions & 4 deletions superset-frontend/src/explore/AdhocFilter.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
* under the License.
*/
import { MULTI_OPERATORS, CUSTOM_OPERATORS } from './constants';
import { getSimpleSQLExpression } from './exploreUtils';

export const EXPRESSION_TYPES = {
SIMPLE: 'SIMPLE',
Expand Down Expand Up @@ -62,9 +63,7 @@ function translateToSql(adhocMetric, { useSimple } = {}) {
const comparator = Array.isArray(adhocMetric.comparator)
? adhocMetric.comparator.join("','")
: adhocMetric.comparator || '';
return `${subject} ${operator} ${isMulti ? "('" : ''}${comparator}${
isMulti ? "')" : ''
}`;
return getSimpleSQLExpression(subject, operator, comparator, isMulti);
}
if (adhocMetric.expressionType === EXPRESSION_TYPES.SQL) {
return adhocMetric.sqlExpression;
Expand All @@ -79,7 +78,7 @@ export default class AdhocFilter {
this.subject = adhocFilter.subject;
this.operator = adhocFilter.operator?.toUpperCase();
this.comparator = adhocFilter.comparator;
this.clause = adhocFilter.clause;
this.clause = adhocFilter.clause || CLAUSES.WHERE;
this.sqlExpression = null;
} else if (this.expressionType === EXPRESSION_TYPES.SQL) {
this.sqlExpression =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ export default class AdhocFilterEditPopoverSqlTabContent extends React.Component

const clauseSelectProps = {
placeholder: t('choose WHERE or HAVING...'),
value: adhocFilter.clause || CLAUSES.WHERE,
value: adhocFilter.clause,
onChange: this.onSqlExpressionClauseChange,
};
const keywords = sqlKeywords.concat(
Expand Down
18 changes: 18 additions & 0 deletions superset-frontend/src/explore/exploreUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -308,3 +308,21 @@ export const useDebouncedEffect = (effect, delay) => {
};
}, [callback, delay]);
};

export const getSimpleSQLExpression = (
subject,
operator,
comparator,
isMulti,
) => {
let expression = subject ?? '';
if (subject && operator) {
expression += ` ${operator}`;
if (comparator) {
expression += ` ${isMulti ? "('" : ''}${comparator}${
isMulti ? "')" : ''
}`;
}
}
return expression;
};