Basic check query tests for custom granularity #1416
Conversation
courtneyholcomb
force-pushed
the
court/parse-custom-4
branch
from
97bf039 to
c4379a5
Compare
courtneyholcomb
force-pushed
the
court/parse-custom-3
branch
from
89f3a74 to
c5cc48a
Compare
courtneyholcomb
force-pushed
the
court/parse-custom-4
branch
from
c4379a5 to
8976c73
Compare
courtneyholcomb
force-pushed
the
court/parse-custom-3
branch
from
c5cc48a to
8795dd1
Compare
courtneyholcomb
force-pushed
the
court/parse-custom-4
branch
from
8976c73 to
8795dd1
Compare
courtneyholcomb
force-pushed
the
court/parse-custom-4
branch
from
af2585f to
454411e
Compare
courtneyholcomb
changed the title
tlento
added
the
Run Tests With Other SQL Engines
tlento
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
tlento
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
tlento
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
tlento
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
github-actions
bot
removed
the
Run Tests With Other SQL Engines
courtneyholcomb
force-pushed
the
court/parse-custom-3
branch
from
a4ad053 to
8044a40
Compare
courtneyholcomb
force-pushed
the
court/parse-custom-4
branch
from
454411e to
deccccb
Compare
There was a problem hiding this comment.
Would it be appropriate(*) to also include one or more no-metric queries?
(*) - I don't even know if you are able to pass '[]' for the metrics in this test set up, so this might be a very silly question.
Otherwise, I don't know the edge cases well enough to be sure we aren't skipping any, but the broad strokes of what the PR stack has been about seem to be covered - using custom granularities, joining on them, and connecting nodes based on date parts (plus some of the edge cases like cumulative metrics and offset metrics.). Accepting.
| ) subq_15 | ||
| --- | ||
| integration_test: | ||
| name: multiple_metrics_with_custom_granularity |
There was a problem hiding this comment.
I feel like I learned a bit just reading this. I really like the way our integration tests are set up here.
There was a problem hiding this comment.
Nice!! Yeah, these end to end tests are great because they actually check that the SQL can actually execute, too, unlike most of our other tests.
| description: A derived metric queried with a custom granularity | ||
| model: SIMPLE_MODEL | ||
| metrics: [ "bookings", "listings"] | ||
| group_bys: ["metric_time__martian_day", "listing__ds__month"] |
There was a problem hiding this comment.
This is exercising the new datepart logic in the linker for custom granularities, right? (I'm just trying to confirm the things I'm learning.)
There was a problem hiding this comment.
the new datepart logic in the linker
Maybe! What do you mean by this?
There was a problem hiding this comment.
hahaha uh oh sounds like I'm learning less than I thought.
I think this query exercises the changes in #1415 - we're able to groupBy DatePart on a derived metric with custom granularity because because of that PR, right?
There was a problem hiding this comment.
Ohh ok I get what you meant by the new datepart logic in the linker now!
we're able to groupBy DatePart on a derived metric with custom granularity because because of that PR
Not quite. Date part and custom granularity can't be used together. That PR does 2 things:
- Bug fix: updates the
LinkableSpecResolverto allow non-default granularity for cumulative metrics. By non-default I don't mean custom. The default granularity for a dimension is the one it's defined at in the YAML definition. For example, if a metric is defined withDAYgranularity, the default isDAYand all other standard granularities (MONTH,YEAR, etc.) and custom granularities are considered non-default. Cumulative metrics can be queried with all granularities, which is a change we made this year, and theLinkableSpecResolverwas out of date for that logic. Cumulative metrics still cannot be queried with date part, though. This change was all in the first commit. - Adds custom granularities to the
LinkableSpecResolverfor all metrics. This change is in all the later commits.
There are a lot of different metric types and group by options to grasp all at once here 😅 So let me know if this makes sense or if you have questions about any of it!
As for this test - it is testing that we can use a custom granularity with multiple simple metrics, and that does rely on the logic from that PR!
Also, I realized just now that I didn't update the descriptions for these tests properly (good old copy/paste) and that probably contributed to the confusion. I'll update those before merging.
There was a problem hiding this comment.
ok, i think it does make sense. I may need a reminder or two about some of these details in the future, but I'm definitely putting things together a bit more as I go. :)
Thank you, Courtney!
@theyostalservice 🤔 You're right! I'm pretty sure I added them at some point at then I must have lost them in the bad rebase I did on this PR 😅 I'll add some to the top of the stack. |
Adds some check query tests for various custom granularity scenarios. I have some tasks up for other scenarios to test, but also open to more suggestions!