fix: SIP-15 time_range_endpoints not using default option

[rubenSastre]

SUMMARY

since SIP-15 is enabled, the time_range_endpoints is hardcoded to [start,end), without possibility of taking value set on extra params on database, or on default parameter on conf.py

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

ADDITIONAL INFORMATION

• [x ] Has associated issue:
• Changes UI
• Requires DB Migration.
• Confirm DB Migration upgrade and downgrade tested.
• Introduces new feature or API
• Removes existing feature or API

[codecov-io]

Codecov Report

Merging #11998 (cda15be) into master (cc44a2c) will decrease coverage by 2.62%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #11998      +/-   ##
==========================================
- Coverage   66.26%   63.63%   -2.63%     
==========================================
  Files         941      942       +1     
  Lines       45715    45827     +112     
  Branches     4395     4400       +5     
==========================================
- Hits        30293    29164    -1129     
- Misses      15287    16487    +1200     
- Partials      135      176      +41     

Flag	Coverage Δ
cypress	?
javascript	62.69% <ø> (+0.01%)	⬆️
python	64.20% <100.00%> (-0.06%)	⬇️

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

Impacted Files	Coverage Δ
superset/config.py	90.07% <100.00%> (ø)
superset/views/core.py	74.74% <100.00%> (+0.03%)	⬆️
superset/views/utils.py	81.77% <100.00%> (ø)
superset-frontend/src/explore/App.jsx	0.00% <0.00%> (-100.00%)	⬇️
superset-frontend/src/dashboard/App.jsx	0.00% <0.00%> (-100.00%)	⬇️
superset-frontend/src/explore/index.jsx	0.00% <0.00%> (-100.00%)	⬇️
superset-frontend/src/dashboard/index.jsx	0.00% <0.00%> (-100.00%)	⬇️
superset-frontend/src/setup/setupColors.js	0.00% <0.00%> (-100.00%)	⬇️
superset-frontend/src/chart/ChartContainer.jsx	0.00% <0.00%> (-100.00%)	⬇️
superset-frontend/src/explore/reducers/index.js	0.00% <0.00%> (-100.00%)	⬇️
... and 148 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cc44a2c...cda15be. Read the comment docs.

[rubenSastre]

@villebro could you have a look?

[villebro]

Thanks @rubenSastre , I'll take a look as soon as I can! (marking myself as reviewer to not forget)

[villebro]

Thanks for the PR @rubenSastre and sorry for the long review time. I agree with the changes made here, but would still like to hear @john-bodley chime in before merging.

[villebro]

I'm surprised these are not in line with the comment in utils. I would be interested in hearing @john-bodley's thoughts on this change, as it looks like this is a bug.

[john-bodley]

@rubenSastre I believe there's a disconnect between you PR description and code change. The existing code (at least in the places you changed) seems correct to me, i.e., we always want to ensure new slices and all slices after the grace period ends adhere to the [start, end) interval definition.

More specifically, the target (desired) time interval is purposely not configurable (and thus is hard coded). The source intervals may vary by institution and can be defined at the database level or fallback to the configuration file if undefined per here.

What is the specific issue you're facing? The disconnect in your two screenshots could be because either:

1. The slice is a new slice.
2. The grace period has expired.

[rubenSastre]

@john-bodley , what I detect is that the database extra value and/or the DEFAULT_TIME_RANGE_ENDPOINT is not working, all the new and the existing charts will be force to be [start, end). This should be the default rule, but we should be able to give the option of setting a different rule for the endpoints.

My changes will use the DEFAULT_TIME_RANGE_ENDPOINT as the value to use for the new charts and to popup the alert of updating if having a different while the the grace period is set.

[john-bodley]

@rubenSastre per your comment,

... , all the new and the existing charts will be force to be [start, end).

This is the intended behavior, i.e., the time range endpoints are not configurable and per SIP-15 ensure that [start, end) is used consistency throughout Superset for all connectors (SQL and the Druid REST API) as previously this wasn't the case. You can refer to SIP-15 regarding the motivation for this change.

[rubenSastre]

@john-bodley ,reading this, so if we don´t want to be an optional configuration has not sense, as what I did is to use the SIP_15_DEFAULT_TIME_RANGE_ENDPOINTS to give this option.

By the way then, the extra parameter for database option doesn´t work when a new chart is created, could you have a look? I don´t know which is the best option as some times the chart_id doesn´t exist, at the begining so [start, end) is set on the creation.

[john-bodley]

@rubenSastre the SIP_15_DEFAULT_TIME_RANGE_ENDPOINTS and database overrides exist merely so deployments can define how time range intervals were defined for said environment pre-SIP-15, i.e., if a database was using timestamps the time range may have been behaving as [inclusive, inclusive] whereas if the database was using date strings the time range may have been behaving as [exclusive, inclusive]. With SIP-15 enabled all time ranges (regardless of connector or database) behave as [inclusive, exclusive) and are not configurable.

[yongchand]

@john-bodley I feel like a lot of people want [inclusive, inclusive] according to slack community, including me. Are you still concrete with opinion for [inclusive, exclusive)? What is the point of SIP_15_DEFAULT_TIME_RANGE_ENDPOINTS exactly? I am willing to commit (including this PR) to better use SIP_15_DEFAULT_TIME_RANGE_ENDPOINTS