Dedupe qualified name for GroupByMetricSpecs

[courtneyholcomb]

Description

Typically, a spec's qualified name is <entity_links>__<element_name>. In the case of GroupByMetricSpecs, this name isn't guaranteed to be unique. That's because the metric_subquery_entity_links are not accounted for. This will also be a problem in SQL generation, since you could end up with a query that references two different group by metrics by the same column name. To avoid that, here I added some deduping logic for the qualified name.

1. If entity_links matches metric_subquery_entity_links, use the standard qualified name.
2. If they don't match, include both, e.g., <entity_links>__<metric_subquery_entity_links>__<element_name>. You can still tell where one path ends and the next begins because they will both end with the same entity link, which is the last link before the element name.

This is definitely ugly!! I am suggesting any alternate suggestions to make this work.

[tlento]

I have three concerns:

1. This naming expansion only works if the link paths are different, but if they're the same it won't help. I don't know how often they'll be different but it seems like a bunch of cases will slip through here.
2. The optimized SQL isn't applying the updated aliases. Does this indicate that we'll still have problems if there are duplicate metric names?
3. This is outside the scope of this PR specifically, but it seems to me that we need a robust aliasing approach for these group by metrics. I don't have great ideas here - the group by item resolver is probably too fine-grained to the metric element in the filter/group by list (so if it's the same metric in group by and filter, or used twice in the filter, we may end up with multiple aliases when those should be collapsed).

[courtneyholcomb]

1. This naming expansion only works if the link paths are different, but if they're the same it won't help. I don't know how often they'll be different but it seems like a bunch of cases will slip through here.

🤔 I don't think I'm understanding this concern. In what scenario would we have duplicates?
If the link paths are the same, we won't expand the name (though we could if consistency is preferable to sometimes-readability). But that un-expanded name still represents a unique join path. e.g., listing__country__bookings vs. listing__country__listing__country__bookings - both would be unique.

2. The optimized SQL isn't applying the updated aliases. Does this indicate that we'll still have problems if there are duplicate metric names?

🤦‍♀️🤦‍♀️🤦‍♀️ Ok, so the snapshots that looked weird in #1166 were just things that got added to the wrong PR. I think I must have made a commit at some point while in the middle of generating snapshots, so a few of the snapshots got updated in the wrong commit. So you don't see those updates here because they're lower in the stack. I need to rebase this one now that I've removed those snapshots, and then update the snapshots again here.

[tlento]

🤔 I don't think I'm understanding this concern. In what scenario would we have duplicates?
If the link paths are the same, we won't expand the name (though we could if consistency is preferable to sometimes-readability). But that un-expanded name still represents a unique join path. e.g., listing__country__bookings vs. listing__country__listing__country__bookings - both would be unique.

It's entirely possible I'm just completely confused here, but if I have, say, a ratio metric that uses the same metric filter reference we'd end up with duplicates in the query output because they are actually the same, and expanding the names like this doesn't help that unless the join paths are different between the group by metric and each of those ratio metrics.

I don't know how common it will be for those join paths to be different, but it's not clear to me that making these long chain sequence names is worth it.

That said, it just occurred to me that we only allow this in the where filter. As long as we don't expose this naming scheme in the output table schemas it doesn't matter much one way or the other - but we'd need to make sure we remove it before allowing group by usage.

🤦‍♀️🤦‍♀️🤦‍♀️ Ok, so the snapshots that looked weird in #1166 were just things that got added to the wrong PR. I think I must have made a commit at some point while in the middle of generating snapshots, so a few of the snapshots got updated in the wrong commit. So you don't see those updates here because they're lower in the stack. I need to rebase this one now that I've removed those snapshots, and then update the snapshots again here.

The snapshots are still showing the same issue. The comment has the expanded alias, but the SQL itself does not. Is that expected?

[tlento]

Like here we have the link, but on line 30 we do not. I don't think that's common with FilterElementsNode accesses - it typically matches, right?

[courtneyholcomb]

Yeah sorry I haven't been able to update snapshots yet! Slowly rebasing up the stack 😓

[courtneyholcomb]

Ok good catch on this!! I wasn't using the qualified name as the column name for group by metrics. Wasn't breaking anything, except our typical column naming convention 😉 I added a commit to fix that.

[tlento]

Ok, LGTM, and nice find on the weird entity link omission pieces.

Let's run with this updated naming for now while we sort out the aliasing situation and whatever all else might be needed to enable using these things in group by statements. I don't think we want the nomenclature here exposed to end users on the output side, but that isn't going to happen just yet.

[tlento]

If I had just ripped this whole class out when I first noticed it none of this would've happened. Lesson learned. Thanks for making the property access nature of this class even more obvious. :)