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

[bugfix] temporal columns with expression fail #4890

Merged
merged 2 commits into from
Apr 27, 2018
+92 −24
Merged

[bugfix] temporal columns with expression fail #4890

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6a7b25f
[bugfix] temporal columns with expression fail
mistercrunch Apr 26, 2018
3ac0ba5
Add tests
mistercrunch Apr 27, 2018
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
5 changes: 5 additions & 0 deletions superset/connectors/base/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,11 @@ def values_for_column(self, column_name, limit=10000):
def default_query(qry):
return qry

def get_column(self, column_name):
for col in self.columns:
if col.column_name == column_name:
return col


class BaseColumn(AuditMixinNullable, ImportMixin):
"""Interface for column"""
Expand Down
6 changes: 4 additions & 2 deletions superset/connectors/sqla/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ def get_timestamp_expression(self, time_grain):
expr = self.expression or self.column_name
if not self.expression and not time_grain:
return column(expr, type_=DateTime).label(DTTM_ALIAS)
literal = '{col}'
if time_grain:
pdf = self.python_date_format
if pdf in ('epoch_s', 'epoch_ms'):
Expand All @@ -130,8 +131,9 @@ def get_timestamp_expression(self, time_grain):
elif pdf == 'epoch_ms':
expr = db_spec.epoch_ms_to_dttm().format(col=expr)
grain = self.table.database.grains_dict().get(time_grain)
literal = grain.function if grain else '{col}'
literal = expr.format(col=expr)
if grain:
literal = grain.function
literal = expr.format(col=expr)
Copy link
Contributor

@xrmx xrmx Apr 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Isn't this line always overriding the one before? the one before should be expr = grain.function or there should be an else branch somewhere

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think this should be

literal = literal.format(col=expr)

Copy link
Member

Choose a reason for hiding this comment

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

See #4892

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I think we're both wrong on this one :( , I'm going to clean this up a bit more and write many tests covering the whole function.

return literal_column(literal, type_=DateTime).label(DTTM_ALIAS)

@classmethod
Expand Down
22 changes: 11 additions & 11 deletions superset/data/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ def load_world_bank_health_n_pop():
"compare_lag": "10",
"compare_suffix": "o10Y",
"limit": "25",
"granularity": "year",
"granularity_sqla": "year",
"groupby": [],
"metric": 'sum__SP_POP_TOTL',
"metrics": ["sum__SP_POP_TOTL"],
Expand Down Expand Up @@ -593,7 +593,7 @@ def load_birth_names():
"compare_lag": "10",
"compare_suffix": "o10Y",
"limit": "25",
"granularity": "ds",
"granularity_sqla": "ds",
"groupby": [],
"metric": 'sum__num',
"metrics": ["sum__num"],
Expand Down Expand Up @@ -642,7 +642,7 @@ def load_birth_names():
datasource_id=tbl.id,
params=get_slice_json(
defaults,
viz_type="big_number", granularity="ds",
viz_type="big_number", granularity_sqla="ds",
compare_lag="5", compare_suffix="over 5Y")),
Slice(
slice_name="Genders",
Expand Down Expand Up @@ -675,7 +675,7 @@ def load_birth_names():
params=get_slice_json(
defaults,
viz_type="line", groupby=['name'],
granularity='ds', rich_tooltip=True, show_legend=True)),
granularity_sqla='ds', rich_tooltip=True, show_legend=True)),
Slice(
slice_name="Average and Sum Trends",
viz_type='dual_line',
Expand All @@ -684,7 +684,7 @@ def load_birth_names():
params=get_slice_json(
defaults,
viz_type="dual_line", metric='avg__num', metric_2='sum__num',
granularity='ds')),
granularity_sqla='ds')),
Slice(
slice_name="Title",
viz_type='markup',
Expand Down Expand Up @@ -729,7 +729,7 @@ def load_birth_names():
datasource_id=tbl.id,
params=get_slice_json(
defaults,
viz_type="big_number_total", granularity="ds",
viz_type="big_number_total", granularity_sqla="ds",
filters=[{
'col': 'gender',
'op': 'in',
Expand Down Expand Up @@ -876,7 +876,7 @@ def load_unicode_test_data():
tbl = obj

slice_data = {
"granularity": "dttm",
"granularity_sqla": "dttm",
"groupby": [],
"metric": 'sum__value',
"row_limit": config.get("ROW_LIMIT"),
Expand Down Expand Up @@ -954,7 +954,7 @@ def load_random_time_series_data():
tbl = obj

slice_data = {
"granularity": "day",
"granularity_sqla": "day",
"row_limit": config.get("ROW_LIMIT"),
"since": "1 year ago",
"until": "now",
Expand Down Expand Up @@ -1017,7 +1017,7 @@ def load_country_map_data():
tbl = obj

slice_data = {
"granularity": "",
"granularity_sqla": "",
"since": "",
"until": "",
"where": "",
Expand Down Expand Up @@ -1092,7 +1092,7 @@ def load_long_lat_data():
tbl = obj

slice_data = {
"granularity": "day",
"granularity_sqla": "day",
"since": "2014-01-01",
"until": "now",
"where": "",
Expand Down Expand Up @@ -1172,7 +1172,7 @@ def load_multiformat_time_series_data():
slice_data = {
"metric": 'count',
"granularity_sqla": col.column_name,
"granularity": "day",
"granularity_sqla": "day",
"row_limit": config.get("ROW_LIMIT"),
"since": "1 year ago",
"until": "now",
Expand Down
6 changes: 6 additions & 0 deletions tests/model_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,3 +105,9 @@ def test_grains_dict(self):
self.assertEquals(d.get('day').function, 'DATE({col})')
self.assertEquals(d.get('P1D').function, 'DATE({col})')
self.assertEquals(d.get('Time Column').function, '{col}')

def test_get_timestamp_expression(self):
tbl = self.get_table_by_name('birth_names')
ds_col = tbl.get_column('ds')
sqla_literal = ds_col.get_timestamp_expression(None)
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a test thats passing a value into get_timestamp_expression() to make sure we are returning the proper literal_col

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 problem is the test will fail depending on the database it's working on. I guess I can figure out which engine it's on (postgres/mysql) and do the proper assertion depending on that

Copy link
Contributor

Choose a reason for hiding this comment

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

Or just add one test per database:
https://docs.python.org/2/library/unittest.html#skipping-tests-and-expected-failures

self.assertEquals(str(sqla_literal.compile()), 'ds')