Skip to content

Commit

Permalink
fix: null value and empty string in filter
Browse files Browse the repository at this point in the history
  • Loading branch information
zhaoyongjie committed Jan 27, 2022
1 parent 4b89ac7 commit 03535f1
Show file tree
Hide file tree
Showing 7 changed files with 60 additions and 31 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 @@ -346,7 +347,11 @@ const AdhocFilterEditPopoverSimpleTabContent: React.FC<Props> = props => {
endpoint: `/superset/filter/${datasource.type}/${datasource.id}/${col}/`,
})
.then(({ json }) => {
setSuggestions(json);
setSuggestions(
json.map((item: null | number | boolean | string) =>
optionLabel(item),
),
);
setLoadingComparatorSuggestions(false);
})
.catch(() => {
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 @@ -404,7 +404,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 @@ -439,9 +439,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 @@ -921,9 +921,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
56 changes: 46 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,57 @@ 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

# 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
# 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 03535f1

Please sign in to comment.