Removing Cartesian Join

[callum-mcdata]

Removing Cartesian Join

Issue: #9

What Does This PR Do?

This PR changes the get_metric_sql macro to remove the cartesian join in the spine__values and spine CTE's.

• Current Behavior: Combinations of data that do not exist within the parent dataset are reflected in the dataset produced by the macro. See Issue Cartesian Joins Create ALL Combinations, Not All Possible Combinations #9 for example.
• New Behavior: spine__values now selects distinct combinations of all provided dimensions and then creates the spine CTE with that list of combinations.

Should This PR Be Merged?

It really depends on how strongly we feel about the cartesian join functionality. I'm unsure of what use case there would be for impossible combinations of dimensions but @joellabes has thought about this more than I have and has stated that there is a use case.

As such, I'm opening this PR and will keep it open until we can come to a community/internal determination of whether we want to support this use case! The main difference between this and my last PR is that I removed the parameter within the metrics macro. My personal opinion is that we either remove that functionality or keep it within the package - too many parameters makes writing the macro not fun.

[cla-bot]

Thanks for your pull request, and welcome to our community! We require contributors to sign our Contributor License Agreement and we don't seem to have your signature on file. Check out this article for more information on why we have a CLA.

In order for us to review and merge your code, please submit the Individual Contributor License Agreement form attached above above. If you have questions about the CLA, or if you believe you've received this message in error, don't hesitate to ping @drewbanin.

CLA has not been signed by users: @callum-mcdata

[callum-mcdata]

Just signed the CLA!

[drewbanin]

Thanks for opening this PR, @callum-mcdata!

The big constraint here is making sure that we can calculate secondary metrics (eg. QTD aggs, PoP changes, rolling sums, etc). The nice thing about cartesian joining everything is that we can then easily implement these secondary calculations on warehouses using window functions! I've always had it in my head that not doing a cartesian join would make that harder/impossible.... but is that true? As long as we join to a date spine, i don't think we should have any problems with generating window functions for the currently supported set of secondary calculations.

Is this something you've thought about? Is it right to assume that we can still implement these secondary calcs w/o doing a full set of cartesian joins? If so, then I think I'm all for this PR :)

[callum-mcdata]

I wouldn't see why this change would block secondary_calculations but that is a good callout on something that I should test.

Part of this is just my confusing terminology though 😵‍💫 . We would still be doing the cartesian join with the date spine to create the spined values that can be aggregated across time but we'd be removing the cartesian behavior in spine__values that creates those impossible combinations of values. Here's how it should work:

Source Table

Person	City	Sales	Date
Callum	Chicago	1	1/1/21
Drew	Philly	2	1/2/21
Joel	Somewhere in NZ	3	1/3/21

Old Behavior

Date	Person	City	Metric
1/1/21	Callum	Chicago	1
1/1/21	Callum	Philly	0
1/1/21	Callum	Somewhere in NZ	0
1/2/21	Callum	Chicago	0
1/2/21	Callum	Philly	0

Etc, etc.

New Behavior

Date	Person	City	Metric
1/1/21	Callum	Chicago	1
1/2/21	Callum	Chicago	0

[drewbanin]

ok - right on - feel free to assign me when this is ready for a review!

[callum-mcdata]

@drewbanin I'm working on getting my local setup to start running some data tests beyond Joel's integration tests (ie confirm the behavior above is what I see in a dataset) but if you wanna review in the meantime, go for it!

[callum-mcdata]

Okay I've run some eye-checks in Snowflake and confirmed that this behavior is working as intended! Given that it is a removal of functionality, I don't think adding integration tests to confirm this behavior is necessary - the current integration test behavior works just fine

[drewbanin]

ship it!