Skip to content

Commit

Permalink
fix: null value and empty string in filter (#18171)
Browse files Browse the repository at this point in the history
  • Loading branch information
zhaoyongjie authored Jan 27, 2022
1 parent dbe0a8b commit 20b4ae1
Show file tree
Hide file tree
Showing 10 changed files with 102 additions and 41 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import AdhocFilter, {
} from 'src/explore/components/controls/FilterControl/AdhocFilter';
import { Input } from 'src/common/components';
import { propertyComparator } from 'src/components/Select/Select';
import { optionLabel } from 'src/utils/common';

const StyledInput = styled(Input)`
margin-bottom: ${({ theme }) => theme.gridUnit * 4}px;
Expand Down Expand Up @@ -226,7 +227,9 @@ const AdhocFilterEditPopoverSimpleTabContent: React.FC<Props> = props => {
isOperatorRelevant,
onComparatorChange,
} = useSimpleTabFilterProps(props);
const [suggestions, setSuggestions] = useState<Record<string, any>>([]);
const [suggestions, setSuggestions] = useState<
Record<'label' | 'value', any>[]
>([]);
const [comparator, setComparator] = useState(props.adhocFilter.comparator);
const [loadingComparatorSuggestions, setLoadingComparatorSuggestions] =
useState(false);
Expand Down Expand Up @@ -346,7 +349,12 @@ const AdhocFilterEditPopoverSimpleTabContent: React.FC<Props> = props => {
endpoint: `/superset/filter/${datasource.type}/${datasource.id}/${col}/`,
})
.then(({ json }) => {
setSuggestions(json);
setSuggestions(
json.map((suggestion: null | number | boolean | string) => ({
value: suggestion,
label: optionLabel(suggestion),
})),
);
setLoadingComparatorSuggestions(false);
})
.catch(() => {
Expand Down Expand Up @@ -402,10 +410,7 @@ const AdhocFilterEditPopoverSimpleTabContent: React.FC<Props> = props => {
{MULTI_OPERATORS.has(operatorId) || suggestions.length > 0 ? (
<SelectWithLabel
labelText={labelText}
options={suggestions.map((suggestion: string) => ({
value: suggestion,
label: String(suggestion),
}))}
options={suggestions}
{...comparatorSelectProps}
sortComparator={propertyComparator(
typeof suggestions[0] === 'number' ? 'value' : 'label',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@
* specific language governing permissions and limitations
* under the License.
*/
import { EMPTY_STRING, NULL_STRING } from 'src/utils/common';
import { getSimpleSQLExpression } from '.';
import { Operators } from '../constants';

const params = {
subject: 'subject',
Expand All @@ -36,6 +38,12 @@ test('Should return "" if subject is falsy', () => {
).toBe('');
});

test('Should return null string and empty string', () => {
expect(getSimpleSQLExpression(params.subject, Operators.IN, [null, ''])).toBe(
`subject ${Operators.IN} (${NULL_STRING}, ${EMPTY_STRING})`,
);
});

test('Should return subject if operator is falsy', () => {
expect(getSimpleSQLExpression(params.subject, '', params.comparator)).toBe(
params.subject,
Expand Down
11 changes: 7 additions & 4 deletions superset-frontend/src/explore/exploreUtils/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import {
OPERATOR_ENUM_TO_OPERATOR_TYPE,
} from 'src/explore/constants';
import { DashboardStandaloneMode } from 'src/dashboard/util/constants';
import { optionLabel } from '../../utils/common';

const MAX_URL_LENGTH = 8000;

Expand Down Expand Up @@ -374,10 +375,12 @@ export const getSimpleSQLExpression = (subject, operator, comparator) => {
firstValue !== undefined && Number.isNaN(Number(firstValue));
const quote = isString ? "'" : '';
const [prefix, suffix] = isMulti ? ['(', ')'] : ['', ''];
const formattedComparators = comparatorArray.map(
val =>
`${quote}${isString ? String(val).replace("'", "''") : val}${quote}`,
);
const formattedComparators = comparatorArray
.map(val => optionLabel(val))
.map(
val =>
`${quote}${isString ? String(val).replace("'", "''") : val}${quote}`,
);
if (comparatorArray.length > 0) {
expression += ` ${prefix}${formattedComparators.join(', ')}${suffix}`;
}
Expand Down
3 changes: 2 additions & 1 deletion superset-frontend/src/utils/common.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import {

// ATTENTION: If you change any constants, make sure to also change constants.py

export const EMPTY_STRING = '<empty string>';
export const NULL_STRING = '<NULL>';
export const TRUE_STRING = 'TRUE';
export const FALSE_STRING = 'FALSE';
Expand Down Expand Up @@ -61,7 +62,7 @@ export function optionLabel(opt) {
return NULL_STRING;
}
if (opt === '') {
return '<empty string>';
return EMPTY_STRING;
}
if (opt === true) {
return '<true>';
Expand Down
8 changes: 3 additions & 5 deletions superset/connectors/base/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
from sqlalchemy.orm import foreign, Query, relationship, RelationshipProperty, Session

from superset import is_feature_enabled, security_manager
from superset.constants import NULL_STRING
from superset.constants import EMPTY_STRING, NULL_STRING
from superset.datasets.commands.exceptions import DatasetNotFoundError
from superset.models.helpers import AuditMixinNullable, ImportExportMixin, QueryResult
from superset.models.slice import Slice
Expand Down Expand Up @@ -406,7 +406,7 @@ def handle_single_value(value: Optional[FilterValue]) -> Optional[FilterValue]:
return utils.cast_to_num(value)
if value == NULL_STRING:
return None
if value == "<empty string>":
if value == EMPTY_STRING:
return ""
if target_column_type == utils.GenericDataType.BOOLEAN:
return utils.cast_to_boolean(value)
Expand Down Expand Up @@ -441,9 +441,7 @@ def query(self, query_obj: QueryObjectDict) -> QueryResult:
"""
raise NotImplementedError()

def values_for_column(
self, column_name: str, limit: int = 10000, contain_null: bool = True,
) -> List[Any]:
def values_for_column(self, column_name: str, limit: int = 10000) -> List[Any]:
"""Given a column, returns an iterable of distinct values
This is used to populate the dropdown showing a list of
Expand Down
4 changes: 1 addition & 3 deletions superset/connectors/druid/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -948,9 +948,7 @@ def metrics_and_post_aggs(
)
return aggs, post_aggs

def values_for_column(
self, column_name: str, limit: int = 10000, contain_null: bool = True,
) -> List[Any]:
def values_for_column(self, column_name: str, limit: int = 10000) -> List[Any]:
"""Retrieve some values for the given column"""
logger.info(
"Getting values for columns [{}] limited to [{}]".format(column_name, limit)
Expand Down
11 changes: 2 additions & 9 deletions superset/connectors/sqla/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -176,9 +176,7 @@ def query(self, query_obj: QueryObjectDict) -> QueryResult:
def get_query_str(self, query_obj: QueryObjectDict) -> str:
raise NotImplementedError()

def values_for_column(
self, column_name: str, limit: int = 10000, contain_null: bool = True,
) -> List[Any]:
def values_for_column(self, column_name: str, limit: int = 10000) -> List[Any]:
raise NotImplementedError()


Expand Down Expand Up @@ -738,9 +736,7 @@ def get_fetch_values_predicate(self) -> TextClause:
)
) from ex

def values_for_column(
self, column_name: str, limit: int = 10000, contain_null: bool = True,
) -> List[Any]:
def values_for_column(self, column_name: str, limit: int = 10000) -> List[Any]:
"""Runs query against sqla to retrieve some
sample values for the given column.
"""
Expand All @@ -756,9 +752,6 @@ def values_for_column(
if limit:
qry = qry.limit(limit)

if not contain_null:
qry = qry.where(target_col.get_sqla_col().isnot(None))

if self.fetch_values_predicate:
qry = qry.where(self.get_fetch_values_predicate())

Expand Down
1 change: 1 addition & 0 deletions superset/constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
from enum import Enum

NULL_STRING = "<NULL>"
EMPTY_STRING = "<empty string>"

CHANGE_ME_SECRET_KEY = "CHANGE_ME_TO_A_COMPLEX_RANDOM_SECRET"

Expand Down
4 changes: 1 addition & 3 deletions superset/views/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -920,9 +920,7 @@ def filter( # pylint: disable=no-self-use
datasource.raise_for_access()
row_limit = apply_max_row_limit(config["FILTER_SELECT_ROW_LIMIT"])
payload = json.dumps(
datasource.values_for_column(
column_name=column, limit=row_limit, contain_null=False,
),
datasource.values_for_column(column_name=column, limit=row_limit),
default=utils.json_int_dttm_ser,
ignore_nan=True,
)
Expand Down
76 changes: 66 additions & 10 deletions tests/integration_tests/sqla_models_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@
from sqlalchemy.sql.elements import TextClause

from superset import db
from superset.connectors.sqla.models import SqlaTable, TableColumn
from superset.connectors.sqla.models import SqlaTable, TableColumn, SqlMetric
from superset.constants import EMPTY_STRING, NULL_STRING
from superset.db_engine_specs.bigquery import BigQueryEngineSpec
from superset.db_engine_specs.druid import DruidEngineSpec
from superset.exceptions import QueryObjectValidationError
Expand Down Expand Up @@ -477,22 +478,77 @@ def test_labels_expected_on_mutated_query(self):
def test_values_for_column(self):
table = SqlaTable(
table_name="test_null_in_column",
sql="SELECT 'foo' as foo UNION SELECT 'bar' UNION SELECT NULL",
sql=(
"SELECT 'foo' as foo "
"UNION SELECT '' "
"UNION SELECT NULL "
"UNION SELECT 'null'"
),
database=get_example_database(),
)
TableColumn(column_name="foo", type="VARCHAR(255)", table=table)
SqlMetric(metric_name="count", expression="count(*)", table=table)

with_null = table.values_for_column(
column_name="foo", limit=10000, contain_null=True
)
# null value, empty string and text should be retrieved
with_null = table.values_for_column(column_name="foo", limit=10000)
assert None in with_null
assert len(with_null) == 3
assert len(with_null) == 4

# null value should be replaced
result_object = table.query(
{
"metrics": ["count"],
"filter": [{"col": "foo", "val": [NULL_STRING], "op": "IN"}],
"is_timeseries": False,
}
)
assert result_object.df["count"][0] == 1

# also accept None value
result_object = table.query(
{
"metrics": ["count"],
"filter": [{"col": "foo", "val": [None], "op": "IN"}],
"is_timeseries": False,
}
)
assert result_object.df["count"][0] == 1

# empty string should be replaced
result_object = table.query(
{
"metrics": ["count"],
"filter": [{"col": "foo", "val": [EMPTY_STRING], "op": "IN"}],
"is_timeseries": False,
}
)
assert result_object.df["count"][0] == 1

without_null = table.values_for_column(
column_name="foo", limit=10000, contain_null=False
# also accept "" string
result_object = table.query(
{
"metrics": ["count"],
"filter": [{"col": "foo", "val": [""], "op": "IN"}],
"is_timeseries": False,
}
)
assert result_object.df["count"][0] == 1

# both replaced
result_object = table.query(
{
"metrics": ["count"],
"filter": [
{
"col": "foo",
"val": [EMPTY_STRING, NULL_STRING, "null", "foo"],
"op": "IN",
}
],
"is_timeseries": False,
}
)
assert None not in without_null
assert len(without_null) == 2
assert result_object.df["count"][0] == 4


@pytest.mark.parametrize(
Expand Down

0 comments on commit 20b4ae1

Please sign in to comment.