-
Notifications
You must be signed in to change notification settings - Fork 14k
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
feat: Make Jinja template applied in timestamp columns #17237
feat: Make Jinja template applied in timestamp columns #17237
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution, really nice feature! I left a small proposal to make the feature support all query properties available.
superset/connectors/sqla/models.py
Outdated
col = literal_column(self.expression, type_=type_) | ||
tp = self.table.get_template_processor() | ||
expression = tp.process_template(self.expression) | ||
col = literal_column(expression, type_=type_) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make sure the template processor has access to all query parameters which are defined early in the get_sqla_query
method (see
superset/superset/connectors/sqla/models.py
Lines 970 to 982 in 4316fe6
template_kwargs = { | |
"columns": columns, | |
"from_dttm": from_dttm.isoformat() if from_dttm else None, | |
"groupby": groupby, | |
"metrics": metrics, | |
"row_limit": row_limit, | |
"row_offset": row_offset, | |
"time_column": granularity, | |
"time_grain": time_grain, | |
"to_dttm": to_dttm.isoformat() if to_dttm else None, | |
"table_columns": [col.column_name for col in self.columns], | |
"filter": filter, | |
} |
superset/superset/connectors/sqla/models.py
Line 995 in 4316fe6
template_processor = self.get_template_processor(**template_kwargs) |
get_timesstamp_expression
that can then be used if available.
Codecov Report
@@ Coverage Diff @@
## master #17237 +/- ##
=======================================
Coverage 76.95% 76.96%
=======================================
Files 1038 1038
Lines 55612 55614 +2
Branches 7590 7590
=======================================
+ Hits 42798 42803 +5
+ Misses 12564 12561 -3
Partials 250 250
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the change that was spot-on with what I was hoping for!
SUMMARY
Fix the timestamp expression in sqla model.py, adding jinja template processor to it.
TESTING INSTRUCTIONS
Apply the above changes, run
docker build -t apache/superset:latest-dev .
and then usedocker-compose
to start the superset app. Go to localhost:8088 in browser and login to superset.Go to dataset, find cleaned_sales_data and go to calculated columns, add a new item like this:
Then go to the chart Proportion of Revenue by Product Line, choose the jinja_date as time column and run. The chart should works fine without error. View the query and the order_{{ jinja_tmpl }} is already replaced by "order_date".
ADDITIONAL INFORMATION
"ENABLE_TEMPLATE_PROCESSING": True