Add clients_daily query

[relud]

No description provided.

[jklukas]

Is this using the auto-generation machinery, or is this hand-built? Is this expected to match exactly with the imported parquet-based clients_daily?

[jklukas]

This is pretty elegant. 300 lines of easy-to-scan SQL.

[jklukas]

Do you know where the ?? comes from? That's always been surprising to me, and it would be great to have it documented in a comment here.

[relud]

I'll add a comment, it comes from hindsight's geoip decoding

[jklukas]

These separators are a nice pattern. It's much easier to see where functions begin and end this way.

[jklukas]

Suggested change

	-- normalize client_id and rank by document_id
	-- normalize client_id and partition by by document_id

The usage of "rank" here is confusing to me.

[relud]

I meant to use order by, sorry I wrote this comment a while back, not sure what i was intending, especially since rank has pretty specific meaning in a bigquery sql context

[relud]

oh, i see now. partition by is definitely better, using that.

[jklukas]

I initially was expecting this to be the main summary json schema from mozilla-pipeline-schemas. Can we adopt a naming convention here that specifies bq schema? foo_v1.bq.schema.json perhaps? Ideally, this should match the naming we use for bq schemas generated in m-p-s. cc @acmiyaguchi

[relud]

yes, we can, for this PR i was just continuing to use what was already in place

[jklukas]

Feel free to ignore for the scope of this PR.

[jklukas]

Annoying. Seems like this should be equivalent to NULLIF(list, []) but apparently nullif doesn't work with array types? Can you add a comment to explain why this needs to exist?

[relud]

I will add a comment, it throws this error: NULLIF is not defined for arguments of type ARRAY<STRUCT<element STRING>>

[relud]

Is this using the auto-generation machinery, or is this hand-built? Is this expected to match exactly with the imported parquet-based clients_daily?

This is hand-built to exactly match the schema of clients_daily_v6 (the imported parquet-based one). Matching the data exactly isn't really possible because v6 uses FIRST_VALUE with no ordering and it's very non-trivial to add ordering, so it's effectively ANY_VALUE. So we could probably use udf_mode_last instead, I just forgot about switching to that when I hit the PR button.

[jklukas]

So we could probably use udf_mode_last instead, I just forgot about switching to that when I hit the PR button

I think we might as well go ahead and do that if we aren't able to match existing clients_daily exactly anyway.

I'm also just realizing that since we're ordering by timestamp DESC, we have probably been doing the wrong thing (breaking ties by taking the earliest seen value rather than latest) by using mode_last. Should we be using udf_mode_first instead?

[relud]

I think mode_last is still the right method, and we should consider fixing and backfilling the other tables that coincidentally did mode_first

[jklukas]

I think mode_last is still the right method, and we should consider fixing and backfilling the other tables that coincidentally did mode_first

Got it. I see now that you're sorting ASC here in both frames, so mode_last is correct. I agree that other tables should be fixed to match the pattern used here, which is easier to reason about than sorting DESC. Breaking out to #75

[relud]

I've updated this to use mode_last, but it's throwing BigQuery error in query operation: Resources exceeded during query execution: Not enough resources for query planning - too many subqueries or query is too complex. so we'll see how tricky i have to get to avoid that

[relud]

I was able to do mode_last without getting a too complex exception by combining all of the udf_mode_last UDFs into a single (nested) subquery expression, but it's not pretty, so I'm going to break that out into a separate PR for further discussion.