Skip to content

remove udfs from newtab_component_content view - #9791

Open
jaredsnyder wants to merge 3 commits into
mainfrom
clean_newtab_component_content_view
Open

remove udfs from newtab_component_content view#9791
jaredsnyder wants to merge 3 commits into
mainfrom
clean_newtab_component_content_view

Conversation

@jaredsnyder

Copy link
Copy Markdown
Contributor

This view doesn't propagate the date filter required by the configuration of moz-fx-data-shared-prod.firefox_desktop_derived.newtab_component_content_v1 for some reason, so it doesn't work as expected and this causes confusion. This PR removes the extra fields so it is just a reflection of the underlying data with nothing extra

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This PR reduces firefox_desktop.newtab_component_content to a plain SELECT * passthrough over firefox_desktop_derived.newtab_component_content_v1, removing the hardcoded submission_date >= '2024-01-01' filter and the derived layout_type, num_tiles_per_row, tile_size, and row_number columns.

Dropping the hardcoded date filter is the right call: with require_partition_filter: true on the underlying table, the window functions blocked predicate pushdown, so the view scanned everything since 2024-01-01 and a user's submission_date predicate had no effect. Consumers will now need to supply their own partition filter, which is the standard contract for passthrough views in this dataset. My one concern is the columns that disappear along with that logic — details inline. The two mozfun.newtab UDFs are still used by firefox_desktop/newtab_visits_daily/view.sql, so they aren't orphaned by this change.

),
0
) AS row_number
*

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

issue: this drops four columns (layout_type, num_tiles_per_row, tile_size, row_number) from a user-facing view, and none of them exist in firefox_desktop_derived/newtab_component_content_v1/schema.yaml, so after this change they are not available anywhere. docs/reference/recommended_practices.md (Views) says a view is a stable interface that should maintain compatibility with existing queries, and that breaking changes must be communicated to fx-data-dev@mozilla.org. Nothing in this repo selects these fields, but Looker/ad-hoc consumers of row_number in particular would silently break.

Since the goal is to stop the view from forcing a full-table scan, the fields can move into the derived table instead of being deleted: newtab_component_content_v1/query.sql already runs per @submission_date and the row_number window only partitions by newtab_visit_id/layout_type, so the whole computation fits inside the daily query — add the columns there plus to schema.yaml, and the view stays a passthrough with no behavior change for consumers. If the fields are genuinely unwanted, please send the deprecation notice to fx-data-dev@mozilla.org.

Either way, query.sql:1's comment (get the window width by visit, which is needed for rownumber) and the newtab_window_inner_width column it explains become orphaned if the rownumber logic is dropped rather than moved.

@scholtzan

Copy link
Copy Markdown
Collaborator

Integration report

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.

2 participants