-
Notifications
You must be signed in to change notification settings - Fork 13.8k
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
[Jinja] Make Presto template functions backwards compatible #7993
[Jinja] Make Presto template functions backwards compatible #7993
Conversation
superset/jinja_context.py
Outdated
|
||
return self._first_latest_partition(table_name) | ||
|
||
def _first_latest_partition(self, table_name: str) -> str: |
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.
Do we need this function as it's only called once from above. Additionally the logic looks similar to latest_partitions
and thus it seems line #253 could simply be,
return self.latest_partitions(table_name)[0]
Note for both latest_partition
and latest_partitions
an IndexError
could be thrown and thus it's probably prudent to add mention in the docstring, i.e.,
:param table_name: table name in the format `schema.table`
:return: the first (or only) value in the latest partition array
:raises IndexError: If no partition exists
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.
updated
d5eb101
to
029ef2a
Compare
Codecov Report
@@ Coverage Diff @@
## master #7993 +/- ##
=========================================
+ Coverage 65.57% 65.6% +0.02%
=========================================
Files 469 469
Lines 22414 22417 +3
Branches 2431 2431
=========================================
+ Hits 14699 14707 +8
+ Misses 7594 7589 -5
Partials 121 121
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.
Small change request: There's a reference to latest_partition('some_table')
in docs/sqllab.rst
; I'm assuming that method is deprecated in favour of first_latest_partition('first table')
? I'm hoping to break out the template part into a page of it's own soon as templating isn't really a subset of SQLLab, but until then it would be good to keep the existing docs as up to date as possible.
029ef2a
to
e99df38
Compare
@villebro comment addressed! |
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.
Hehe thanks @etr2460 sorry for being a pain! 😁
Not a pain at all! it's important to keep documentation up to date :D |
CATEGORY
Choose one
SUMMARY
#7782 changed the behavior of the
presto.latest_partition
Jinja template function to return an array instead of a string, breaking all sql metrics/slices that used it. This PR makes thelatest_partition
function behave the same as prior to this change, introduces a new functionpresto.latest_partitions
that can be used with this new behavior, and provides documentation for the change in code.It might be worth renaming these functions if we were ever to do a breaking release of superset, but i don't see that happening any time soon.
TEST PLAN
Tested locally, CI
ADDITIONAL INFORMATION
REVIEWERS
@michellethomas @john-bodley @betodealmeida @DiggidyDave @xtinec