feat(jinja): add advanced temporal filter functionality

[villebro]

SUMMARY

Currently the from_dttm and to_dttm Jinja variables make it possible to reference the time range endpoints from within the Jinja context. However, they have some shortcomings:

• to_dttm always points to the current day, even if there's no time range present in the query (this appears to be a quirk of how the get_since_until helper function works).
• They are always formatted using the .isoformat() method. This means that it's difficult to use them for database-specific formatting that's readily available in BaseEngineSpec.convert_dttm.
• It's impossible to access a specific time filter if there are multiple time filters present.
• In the case of virtual datasets, it's not possible to remove the time filter from the outer query (this is possible for regular filter types using the filter_value and get_filters macros).

This PR adds a new Jinja macro called get_time_filter, which returns a dataclass with the following properties:

• from_expr: The SQL expression representing the start of the time range, if available
• to_expr: The SQL expression representing the end of the time range, if available
• time_range: The (usually) human readable raw value of the time filter (e.g. "No filter", "Last week" etc)

Please refer to the added docs for how the feature works in practice:

TESTING INSTRUCTIONS

• Run examples in added docs
• Added unit tests (100 % coverage for added logic)

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.67%. Comparing base (76d897e) to head (0212fa9).
Report is 692 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #30142       +/-   ##
===========================================
+ Coverage   60.48%   83.67%   +23.18%     
===========================================
  Files        1931      529     -1402     
  Lines       76236    38334    -37902     
  Branches     8568        0     -8568     
===========================================
- Hits        46114    32075    -14039     
+ Misses      28017     6259    -21758     
+ Partials     2105        0     -2105     

Flag	Coverage Δ
hive	48.85% <37.83%> (-0.31%)	⬇️
javascript	?
mysql	76.60% <37.83%> (?)
postgres	76.70% <37.83%> (?)
presto	53.39% <37.83%> (-0.41%)	⬇️
python	83.67% <100.00%> (+20.18%)	⬆️
sqlite	76.16% <37.83%> (?)
unit	60.45% <100.00%> (+2.82%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[nytai]

So something that comes up often in the templateized datasets using these filter values is having defaults so that users can run the "sync columns from source" action and have their query execute. The dataset modal does have "Template parameters" setting but that actually ends up overriding anything set at the dashboard or chart level which is kind of unexpected.

I've been advising users to always use if/else to check if the filter values are defined otherwise fallback to a reasonable default (eg last week or day), so that their query is valid even if not run in a dashboard or chart context. It would be good to bake this default behavior into the jinja expression so that expressing this is a little less verbose. Something like:

get_time_filter(..., default_time_range="Last Week") and maybe even get_time_filter(..., default_ from_expr ="2024-09-04", default_to_expr="2024-09-05")

[villebro]

So something that comes up often in the templateized datasets using these filter values is having defaults so that users can run the "sync columns from source" action and have their query execute. The dataset modal does have "Template parameters" setting but that actually ends up overriding anything set at the dashboard or chart level which is kind of unexpected.

I've been advising users to always use if/else to check if the filter values are defined otherwise fallback to a reasonable default (eg last week or day), so that their query is valid even if not run in a dashboard or chart context. It would be good to bake this default behavior into the jinja expression so that expressing this is a little less verbose. Something like:

get_time_filter(..., default_time_range="Last Week") and maybe even get_time_filter(..., default_from_expr ="2024-09-04", default_to_expr="2024-09-05")

That's a really good idea 👍 To keep things simple, I would prefer only having default_time_range, as it would be possible to express the explicit dates in the latter example with the time range syntax as follows: "2024-09-04 : 2024-09-05". And maybe even simplifying the naming further to keep it in line with the existing filter_values, and just calling it default:

def get_time_filter(
    self,
    column: str | None = None,
    default: str | None = None,
    target_type: str | None = None,
    remove_filter: bool = False,
) -> TimeFilter:
    ...
    # logic to populate time_range from filter value
    ...
    if time_range == NO_TIME_RANGE and default:
        time_range = default
    ...

[villebro]

@nytai I've added the default option in the latest commit: 288e6af

[nytai]

LGTM

[sfirke]

Docs & comments: A few documentation language suggestions and I pointed out a place where a sentence stops halfway.

Code: I glanced at the code, it's mostly beyond me so I'll defer to others there.

Overall: seems like a nice improvement in functionality, I appreciate the complete documentation to go with it 🙏

[villebro]

Thanks for the review @sfirke ! All very good callouts, I've addressed the comments in 0212fa9

[giftig]

LGTM

[villebro]

I added a self-assigned task for removing docs for from_dttm and to_dttm in favor of this macro in 5.0: https://github.com/orgs/apache/projects/345?pane=issue&itemId=78905534