apache / superset

Apache Superset is a Data Visualization and Data Exploration Platform
https://superset.apache.org/
Apache License 2.0
62.8k stars 13.87k forks source link

fix(db_engine_specs): add a few missing time grains to Postgres spec #30325

Closed sfirke closed 1 month ago

sfirke commented 1 month ago

SUMMARY

I added to the Postgres engine spec definitions for some of the time grains that are defined in the base spec but had not been added to Postgres yet.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

image

TESTING INSTRUCTIONS

In an ephemeral environment, confirm that the grains are there and that the generated queries are correct.

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 83.89%. Comparing base (76d897e) to head (0bcedac). Report is 760 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #30325 +/- ## =========================================== + Coverage 60.48% 83.89% +23.40% =========================================== Files 1931 533 -1398 Lines 76236 38489 -37747 Branches 8568 0 -8568 =========================================== - Hits 46114 32289 -13825 + Misses 28017 6200 -21817 + Partials 2105 0 -2105 ``` | [Flag](https://app.codecov.io/gh/apache/superset/pull/30325/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | | |---|---|---| | [hive](https://app.codecov.io/gh/apache/superset/pull/30325/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `49.00% <ø> (-0.16%)` | :arrow_down: | | [javascript](https://app.codecov.io/gh/apache/superset/pull/30325/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | | | [mysql](https://app.codecov.io/gh/apache/superset/pull/30325/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `76.83% <ø> (?)` | | | [postgres](https://app.codecov.io/gh/apache/superset/pull/30325/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `76.93% <ø> (?)` | | | [presto](https://app.codecov.io/gh/apache/superset/pull/30325/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `53.51% <ø> (-0.30%)` | :arrow_down: | | [python](https://app.codecov.io/gh/apache/superset/pull/30325/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `83.89% <ø> (+20.40%)` | :arrow_up: | | [sqlite](https://app.codecov.io/gh/apache/superset/pull/30325/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `76.38% <ø> (?)` | | | [unit](https://app.codecov.io/gh/apache/superset/pull/30325/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `60.61% <ø> (+2.98%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

sfirke commented 1 month ago

@villebro I can mimic those tests for Postgres. Though I wonder about the rigor of comparing the code in my spec to pasting the same code in a test. What if instead/also I had it test the transformation of a specific timestamp? Like that rounding ... 00:15:01 to 1 minute intervals gives ... 00:15:00 etc. Unless that's already happening here and I'm missing it.

villebro commented 1 month ago

@villebro I can mimic those tests for Postgres. Though I wonder about the rigor of comparing the code in my spec to pasting the same code in a test. What if instead/also I had it test the transformation of a specific timestamp? Like that rounding ... 00:15:01 to 1 minute intervals gives ... 00:15:00 etc. Unless that's already happening here and I'm missing it.

So the reason why these tests are valuable, despite looking like pure duplication, is because a change might happen that causes these time grain definitions to no longer be used. For example, I once did a big refactor of the db engine specs (#7676), and that refactor caused the time grains to be removed by mistake from the Hive engine spec (this was later fixed in #10084). If we would have had tests like this in place, we would have caught that error during the refactor. I know it may look funny to duplicate that functional logic to the unit tests, but having them there makes it less likely to introduce regressions, and it also makes it safer to refactor the implementation details.

Also one thing to note here, is that the unit test is in fact testing the function BaseEngineSpec.get_timestamp_expr, which is different from the map _time_grain_expressions that currently contains the expression definitions. It is highly likely that we will, at some point, change how time grains are defined, without changing the method that's used to retrieve the actual expression. So while they may look highly duplicated, they're actually slightly different.

Regarding actual functional verification that the expression actually rounds down the timestamp based on the time grain, that is something that we can't unfortunately test here, as that logic resides in Postgres. But correct me if I'm misunderstanding what you're saying.

sfirke commented 1 month ago

Thank you for that thorough explanation! I now see the point of these tests. And whoops I missed that we can't actually test the output of the time grain because that happens in Postgres, good call. Okay, I will add tests like these.