Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RS-788 Add support for organic searches with ads to the mobile search counts tables #5598

Merged
merged 14 commits into from
May 29, 2024

Conversation

pissac17
Copy link
Contributor

@pissac17 pissac17 commented May 16, 2024

Checklist for reviewer:

  • Commits should reference a bug or github issue, if relevant (if a bug is referenced, the pull request should include the bug number in the title).
  • If the PR comes from a fork, trigger integration CI tests by running the Push to upstream workflow and provide the <username>:<branch> of the fork as parameter. The parameter will also show up
    in the logs of the manual-trigger-required-for-fork CI task together with more detailed instructions.
  • If adding a new field to a query, ensure that the schema and dependent downstream schemas have been updated.
  • When adding a new derived dataset, ensure that data is not available already (fully or partially) and recommend extending an existing dataset in favor of creating new ones. Data can be available in the bigquery-etl repository, looker-hub or in looker-spoke-default.

For modifications to schemas in restricted namespaces (see CODEOWNERS):

┆Issue is synchronized with this Jira Task

@dataops-ci-bot

This comment has been minimized.

@pissac17 pissac17 requested a review from alekhyamoz May 16, 2024 17:18
@dataops-ci-bot

This comment has been minimized.

@alekhyamoz
Copy link
Contributor

@pissac17 does this PR need to include the search revenue levers tables as well?

@pissac17
Copy link
Contributor Author

@alekhyamoz No this one does not have the changes to search_revenue_levers yet. I'll be working on including these tomorrow.

- include search_with_ads_organic columns for Bing, Google and DDG
@pissac17 pissac17 requested a review from a team as a code owner May 24, 2024 18:33
@pissac17 pissac17 requested a review from skahmann3 May 24, 2024 18:34
@pissac17
Copy link
Contributor Author

@alekhyamoz @skahmann3 I've added the changes needed to search_revenue_levers_daily_v1 tables to include search_with_ads_organic field.

Copy link
Contributor

@skahmann3 skahmann3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Looking forward to having this data in levers_daily!

@dataops-ci-bot

This comment has been minimized.

@alekhyamoz alekhyamoz force-pushed the RS_788_Add_support_for_organic_searches_with_ads branch from fec2e3d to 15dddaa Compare May 28, 2024 15:22
@dataops-ci-bot

This comment has been minimized.

@dataops-ci-bot

This comment has been minimized.

@alekhyamoz
Copy link
Contributor

@pissac17 the query - sql/moz-fx-data-shared-prod/search_derived/search_revenue_levers_daily_v1/query.sql is failing when I run in BQ. Can you please fix this?

@alekhyamoz alekhyamoz force-pushed the RS_788_Add_support_for_organic_searches_with_ads branch from 7a942c2 to bdca04c Compare May 28, 2024 15:58
@dataops-ci-bot

This comment has been minimized.

@dataops-ci-bot

This comment has been minimized.

@pissac17
Copy link
Contributor Author

pissac17 commented May 28, 2024

Looking at the error message, search_revenue_levers_daily_v1 table pulls data from the mobile_search_clients_daily table, which doesn’t have the new column yet. This could cause the test checks to fail,I'll be opening a separate PR for this instead

@dataops-ci-bot

This comment has been minimized.

@dataops-ci-bot

This comment has been minimized.

@dataops-ci-bot

This comment has been minimized.

@alekhyamoz alekhyamoz force-pushed the RS_788_Add_support_for_organic_searches_with_ads branch from 4cf4234 to ae98440 Compare May 29, 2024 01:52
@dataops-ci-bot

This comment has been minimized.

@dataops-ci-bot

This comment has been minimized.

reverting back to original code for search_revenue_levers table
@dataops-ci-bot

This comment has been minimized.

@dataops-ci-bot

This comment has been minimized.

@dataops-ci-bot

This comment has been minimized.

@alekhyamoz alekhyamoz enabled auto-merge (squash) May 29, 2024 17:16
@dataops-ci-bot
Copy link

Integration report for "Merge branch 'main' into RS_788_Add_support_for_organic_searches_with_ads"

sql.diff

Click to expand!
diff -bur --no-dereference --new-file /tmp/workspace/main-generated-sql/sql/moz-fx-data-shared-prod/search_derived/mobile_search_aggregates_v1/query.sql /tmp/workspace/generated-sql/sql/moz-fx-data-shared-prod/search_derived/mobile_search_aggregates_v1/query.sql
--- /tmp/workspace/main-generated-sql/sql/moz-fx-data-shared-prod/search_derived/mobile_search_aggregates_v1/query.sql	2024-05-29 17:16:54.000000000 +0000
+++ /tmp/workspace/generated-sql/sql/moz-fx-data-shared-prod/search_derived/mobile_search_aggregates_v1/query.sql	2024-05-29 17:16:55.000000000 +0000
@@ -22,6 +22,7 @@
   SUM(search_with_ads) AS search_with_ads,
   SUM(unknown) AS unknown,
   CAST(NULL AS string) normalized_engine,
+  SUM(search_with_ads_organic) AS search_with_ads_organic,
 FROM
   mobile_search_clients_daily_v1
 WHERE
diff -bur --no-dereference --new-file /tmp/workspace/main-generated-sql/sql/moz-fx-data-shared-prod/search_derived/mobile_search_aggregates_v1/schema.yaml /tmp/workspace/generated-sql/sql/moz-fx-data-shared-prod/search_derived/mobile_search_aggregates_v1/schema.yaml
--- /tmp/workspace/main-generated-sql/sql/moz-fx-data-shared-prod/search_derived/mobile_search_aggregates_v1/schema.yaml	2024-05-29 17:16:54.000000000 +0000
+++ /tmp/workspace/generated-sql/sql/moz-fx-data-shared-prod/search_derived/mobile_search_aggregates_v1/schema.yaml	2024-05-29 17:16:55.000000000 +0000
@@ -68,3 +68,6 @@
 - mode: NULLABLE
   name: ad_click_organic
   type: INTEGER
+- mode: NULLABLE
+  name: search_with_ads_organic
+  type: INTEGER
diff -bur --no-dereference --new-file /tmp/workspace/main-generated-sql/sql/moz-fx-data-shared-prod/search_derived/mobile_search_clients_daily_v1/query.sql /tmp/workspace/generated-sql/sql/moz-fx-data-shared-prod/search_derived/mobile_search_clients_daily_v1/query.sql
--- /tmp/workspace/main-generated-sql/sql/moz-fx-data-shared-prod/search_derived/mobile_search_clients_daily_v1/query.sql	2024-05-29 17:16:54.000000000 +0000
+++ /tmp/workspace/generated-sql/sql/moz-fx-data-shared-prod/search_derived/mobile_search_clients_daily_v1/query.sql	2024-05-29 17:16:55.000000000 +0000
@@ -722,6 +722,12 @@
             SUBSTR(search.key, STRPOS(search.key, '.') + 1),
             search.search_type
           )
+      WHEN search.search_type = 'search-with-ads'
+        THEN IF(
+            REGEXP_CONTAINS(search.key, '\\.'),
+            SUBSTR(search.key, STRPOS(search.key, '.') + 1),
+            search.search_type
+          )
       ELSE search.search_type
     END AS source,
     search.value AS search_count,
@@ -774,6 +780,8 @@
     CASE
       WHEN search_type = 'ad-click'
         THEN IF(STARTS_WITH(source, 'in-content.organic'), 'ad-click-organic', search_type)
+      WHEN search_type = 'search-with-ads'
+        THEN IF(STARTS_WITH(source, 'in-content.organic'), 'search-with-ads-organic', search_type)
       WHEN STARTS_WITH(source, 'in-content.sap.')
         THEN 'tagged-sap'
       WHEN REGEXP_CONTAINS(source, '^in-content.*-follow-on')
@@ -856,6 +864,15 @@
       )
     ) AS search_with_ads,
     SUM(
+      IF(
+        search_type != 'search-with-ads-organic'
+        OR engine IS NULL
+        OR search_count > 10000,
+        0,
+        search_count
+      )
+    ) AS search_with_ads_organic,
+    SUM(
       IF(search_type != 'unknown' OR engine IS NULL OR search_count > 10000, 0, search_count)
     ) AS unknown,
     udf.mode_last(ARRAY_AGG(country)) AS country,

Link to full diff

@alekhyamoz alekhyamoz merged commit 9814059 into main May 29, 2024
20 checks passed
@alekhyamoz alekhyamoz deleted the RS_788_Add_support_for_organic_searches_with_ads branch May 29, 2024 17:29
lucia-vargas-a pushed a commit that referenced this pull request May 29, 2024
… counts tables (#5598)

* adding organic searches with ads to this table

* updating mobile_search_aggregates table with search_with_ads_organic column

* updating the search revenue lever table

- include search_with_ads_organic columns for Bing, Google and DDG

* Fix CI issues

* Fix tests CI failure

* fix tests

* Fix test sql failure

* Update query.sql

reverting back to original code for search_revenue_levers table

---------

Co-authored-by: Alekhya Kommasani <akommasani@mozilla.com>
Co-authored-by: Alekhya <88394696+alekhyamoz@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants