Add sql search_metric_contribution #1077

din-din-din · 2020-06-15T22:55:07Z

Add sql folder search_metric_contribution under search_derived. Please review and let @benmiroglio and me know if there are any problems! Thanks!

benmiroglio · 2020-06-17T14:43:29Z

@fbertsch would you mind taking a look at this? Ding is an intern who's working on productionizing a table to help us look at questions like the following on a daily basis:

"What percent of Ad Clicks are contributed by users in the top [10, 20, 30] percent of the SAP distribution?"

fbertsch

By default, our tables are partitioned by submission_date. Right now this table doesn't have that field, so this will end up failing.

Additionally, this table will only really be useful at the end of the month; during the middle of the money, the fields (e.g. search_with_ads) will only have half the current month.

Additionally, months aren't even; some are longer than others.

I recommend instead including the @submission_date parameter as the submission_date field. Then read the past 27 days before that day (submission_date >= DATE_SUB(@submission_date, INTERVAL 27 DAY), and get the aggregates of those fields over that entire time period. That way we get daily updates of this table, every day is getting quantiles over the same time period, and it should match other metrics we use over monthly intervals.

fbertsch · 2020-06-17T15:23:25Z

@din-din-din, let me know if you'd like to meet to discuss those changes. Happy to help out.

benmiroglio · 2020-06-17T21:07:45Z

This LGTM, @fbertsch please confirm :)

fbertsch

Just a few more changes. Getting there! Thanks for persevering.

fbertsch · 2020-06-18T13:01:44Z

sql/search_derived/search_metric_contribution/metadata.yaml

+description: >
+  A table shows how much of one metric is contributed by another metric.
+owners:
+  - 


Suggested change

-

- [email protected]

fbertsch · 2020-06-18T13:02:19Z

sql/search_derived/search_metric_contribution/metadata.yaml

@@ -0,0 +1,9 @@
+friendly_name: Search Metric Contribution
+description: >
+  A table shows how much of one metric is contributed by another metric.


This could use a bit more fleshing out: What specific questions does this solve? What are the fields? What is it derived from?

fbertsch · 2020-06-18T13:02:29Z

sql/search_derived/search_metric_contribution/metadata.yaml

+labels:
+  schedule: daily
+scheduling:
+  dag_name: 


This also seems to be empty.

fbertsch · 2020-06-18T13:06:53Z

sql/search_derived/search_metric_contribution/query.sql

+  FROM
+    `moz-fx-data-shared-prod.search.search_clients_daily`
+  WHERE
+    submission_date >= DATE_SUB(@submission_date, INTERVAL 27 DAY)


Suggested change

submission_date >= DATE_SUB(@submission_date, INTERVAL 27 DAY)

submission_date BETWEEN DATE_SUB(@submission_date, INTERVAL 27 DAY) AND @submission_date

This prevents us from reading the future during backfills.

fbertsch · 2020-06-18T13:10:01Z

sql/search_derived/search_metric_contribution/query.sql

+  WHERE
+    submission_date >= DATE_SUB(@submission_date, INTERVAL 27 DAY)
+  GROUP BY
+    1


Per our sql style guide, we want explicitly named columns.

fbertsch · 2020-06-18T13:14:18Z

sql/search_derived/search_metric_contribution/query.sql

+  GROUP BY
+    1
+),
+monthly_aggregates AS (


This and the CTE below can be merged into one (with all SUM fields and all APPROX_QUANTILE fields).

fbertsch · 2020-06-18T13:15:56Z

sql/search_derived/search_metric_contribution/query.sql

@@ -0,0 +1,213 @@
+-- temp function returns how much of one metric is contributed by the quantile of another metric
+CREATE TEMP FUNCTION get_quantile(metric1 FLOAT64, metric2 FLOAT64, quantile FLOAT64) AS (


This should be moved into the udfs directory, and given a more descriptive name. The name of the file you create there will be udf/{udf_name}.sql, and inside that file use CREATE OR REPLACE FUNCTION udf.{udf_name}.

I added udf.quantile_search_metric_contribution and called this function in query. But the log showed a 'function not found' error. Why was this issue?

Would you mind reviewing the new udf I added? Otherwise I can't add derived datasets I believe. Let me know if there were any problems! Thanks! :)

fbertsch · 2020-06-18T13:17:13Z

sql/search_derived/search_metric_contribution/metadata.yaml

+labels:
+  schedule: daily
+scheduling:
+  dag_name: 


Suggested change

dag_name:

dag_name: bqetl_search

Thanks for all the comments. I will review these and get back to you!

- add quantile_search_metric_contribution.sql in udf folder - some fixes on search_metric_contribution - update search_metric_contribution metadata

benmiroglio · 2020-06-22T20:36:21Z

dthorn mentioned in #1081

Yeah, that's a known-issue with PRs that add a persistent UDF and use it in the same PR. The process for this is that we deploy the UDF manually right before merging the PR, and dry-run again to ensure it works as expected. Alternately the UDF can be moved to a separate PR.

@fbertsch are you able to "manually deploy" this UDF as dthorn describes? Or should we move the udf to a separate PR?

fbertsch

Yup, I can get this working. Thanks for the contribution @din-din-din!

fbertsch · 2020-06-23T14:29:32Z

udf/quantile_search_metric_contribution.sql

+  assert_equals(1.0, udf.quantile_search_metric_contribution(73, 1, 60)),
+  assert_null(udf.quantile_search_metric_contribution(29, 9, 60)),
+  assert_equals(8.0, udf.quantile_search_metric_contribution(118, 8, 21)),
+  assert_null(udf.quantile_search_metric_contribution(58, 5, 21));


This test is incorrect. It returns 5 because 58 > 21.

Thanks @fbertsch! I fixed that. Let me know if there were any other problems!

fbertsch

🚢

add search_metric_contribution

5aa163a

fbertsch suggested changes Jun 17, 2020

View reviewed changes

update submission_date time period

25ebee6

fbertsch suggested changes Jun 18, 2020

View reviewed changes

fbertsch reviewed Jun 18, 2020

View reviewed changes

Ding Ding added 3 commits June 18, 2020 15:13

Update search_metric_contribution and add udf function

80650bc

- add quantile_search_metric_contribution.sql in udf folder - some fixes on search_metric_contribution - update search_metric_contribution metadata

Remove query to add UDF first

4e24c02

Add search_metric_contribution

a715f8f

fbertsch approved these changes Jun 23, 2020

View reviewed changes

fbertsch suggested changes Jun 23, 2020

View reviewed changes

Ding Ding and others added 2 commits June 23, 2020 11:06

Fix udf test

a9d1d75

Merge branch 'master' into search_metric_contribution_branch

b8888ed

fbertsch approved these changes Jun 23, 2020

View reviewed changes

fbertsch merged commit 2cba4ac into mozilla:master Jun 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add sql search_metric_contribution #1077

Add sql search_metric_contribution #1077

din-din-din commented Jun 15, 2020

benmiroglio commented Jun 17, 2020

fbertsch left a comment

fbertsch commented Jun 17, 2020

benmiroglio commented Jun 17, 2020

fbertsch left a comment

fbertsch Jun 18, 2020

fbertsch Jun 18, 2020

fbertsch Jun 18, 2020

fbertsch Jun 18, 2020

fbertsch Jun 18, 2020

fbertsch Jun 18, 2020

fbertsch Jun 18, 2020

din-din-din Jun 18, 2020

din-din-din Jun 22, 2020

fbertsch Jun 18, 2020 •

edited

Loading

din-din-din Jun 18, 2020

benmiroglio commented Jun 22, 2020

fbertsch left a comment

fbertsch Jun 23, 2020

din-din-din Jun 23, 2020

fbertsch left a comment

	submission_date >= DATE_SUB(@submission_date, INTERVAL 27 DAY)
	submission_date BETWEEN DATE_SUB(@submission_date, INTERVAL 27 DAY) AND @submission_date

		@@ -0,0 +1,213 @@
		-- temp function returns how much of one metric is contributed by the quantile of another metric
		CREATE TEMP FUNCTION get_quantile(metric1 FLOAT64, metric2 FLOAT64, quantile FLOAT64) AS (

Add sql search_metric_contribution #1077

Add sql search_metric_contribution #1077

Conversation

din-din-din commented Jun 15, 2020

benmiroglio commented Jun 17, 2020

fbertsch left a comment

Choose a reason for hiding this comment

fbertsch commented Jun 17, 2020

benmiroglio commented Jun 17, 2020

fbertsch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbertsch Jun 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benmiroglio commented Jun 22, 2020

fbertsch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbertsch left a comment

Choose a reason for hiding this comment

fbertsch Jun 18, 2020 •

edited

Loading