From f0673648c219c6375a792d37d1945e606e498399 Mon Sep 17 00:00:00 2001 From: Ben Halpern Date: Mon, 18 Dec 2023 14:20:56 -0500 Subject: [PATCH] Add preferred_article_ids to billboards (#20437) * Add preferred_article_ids to billboards * Adjust query * Refactor method * Refactor method * Fix input type * Add feature flag --- .../api/v1/billboards_controller.rb | 2 +- app/models/billboard.rb | 67 ++++++++++---- ...208165353_add_article_ids_to_billboards.rb | 9 ++ ...454_add_article_ids_index_to_billboards.rb | 7 ++ db/schema.rb | 4 +- spec/models/billboard_spec.rb | 91 ++++++++++++++----- spec/requests/api/v1/billboards_spec.rb | 6 +- 7 files changed, 139 insertions(+), 47 deletions(-) create mode 100644 db/migrate/20231208165353_add_article_ids_to_billboards.rb create mode 100644 db/migrate/20231208165454_add_article_ids_index_to_billboards.rb diff --git a/app/controllers/api/v1/billboards_controller.rb b/app/controllers/api/v1/billboards_controller.rb index 35da36353..f19b2e900 100644 --- a/app/controllers/api/v1/billboards_controller.rb +++ b/app/controllers/api/v1/billboards_controller.rb @@ -51,7 +51,7 @@ module Api :name, :organization_id, :placement_area, :published, :tag_list, :type_of, :exclude_article_ids, :weight, :audience_segment_type, :audience_segment_id, :priority, - :custom_display_label, :template, :render_mode, + :custom_display_label, :template, :render_mode, :preferred_article_ids, # Permitting twice allows both comma-separated string and array values :target_geolocations, target_geolocations: [] end diff --git a/app/models/billboard.rb b/app/models/billboard.rb index 7e3c330e2..77651b3ac 100644 --- a/app/models/billboard.rb +++ b/app/models/billboard.rb @@ -92,7 +92,7 @@ class Billboard < ApplicationRecord # Here we sample from only billboards with fewer than 1000 impressions (with a fallback # if there are none of those, causing an extra query, but that shouldn't happen very often). relation = billboards_for_display.seldom_seen(area) - weighted_random_selection(relation) || billboards_for_display.sample + weighted_random_selection(relation, article&.id) || billboards_for_display.sample else # large range, 65% # Ads that get engagement have a higher "success rate", and among this category, we sample from the top 15 that @@ -105,24 +105,49 @@ class Billboard < ApplicationRecord end end - def self.weighted_random_selection(relation) + def self.weighted_random_selection(relation, target_article_id = nil) base_query = relation.to_sql random_val = rand.to_f - - query = <<-SQL - WITH base AS (#{base_query}), - weighted AS ( - SELECT *, weight, - SUM(weight) OVER () AS total_weight, - SUM(weight) OVER (ORDER BY id ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW) AS running_weight - FROM base - ) - SELECT *, running_weight, ? * total_weight AS random_value FROM weighted - WHERE running_weight >= ? * total_weight - ORDER BY running_weight ASC - LIMIT 1 - SQL - + if FeatureFlag.enabled?(:article_id_adjusted_weight) + condition = target_article_id.blank? ? "FALSE" : "#{target_article_id} = ANY(preferred_article_ids)" + query = <<-SQL + WITH base AS (#{base_query}), + weighted AS ( + SELECT *, + CASE + WHEN #{condition} THEN weight * 10 + ELSE weight + END AS adjusted_weight, + SUM(CASE + WHEN #{condition} THEN weight * 10 + ELSE weight + END) OVER () AS total_weight, + SUM(CASE + WHEN #{condition} THEN weight * 10 + ELSE weight + END) OVER (ORDER BY id ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW) AS running_weight + FROM base + ) + SELECT *, running_weight, ? * total_weight AS random_value FROM weighted + WHERE running_weight >= ? * total_weight + ORDER BY running_weight ASC + LIMIT 1 + SQL + else + query = <<-SQL + WITH base AS (#{base_query}), + weighted AS ( + SELECT *, weight, + SUM(weight) OVER () AS total_weight, + SUM(weight) OVER (ORDER BY id ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW) AS running_weight + FROM base + ) + SELECT *, running_weight, ? * total_weight AS random_value FROM weighted + WHERE running_weight >= ? * total_weight + ORDER BY running_weight ASC + LIMIT 1 + SQL + end relation.find_by_sql([query, random_val, random_val]).first end @@ -196,7 +221,7 @@ class Billboard < ApplicationRecord end # rubocop:enable Style/OptionHash - # exclude_article_ids is an integer array, web inputs are comma-separated strings + # exclude_article_ids and preferred_article_ids are integer arrays, web inputs are comma-separated strings # ActiveRecord normalizes these in a bad way, so we are intervening def exclude_article_ids=(input) adjusted_input = input.is_a?(String) ? input.split(",") : input @@ -204,6 +229,12 @@ class Billboard < ApplicationRecord write_attribute :exclude_article_ids, (adjusted_input || []) end + def preferred_article_ids=(input) + adjusted_input = input.is_a?(String) ? input.split(",") : input + adjusted_input = adjusted_input&.filter_map { |value| value.presence&.to_i } + write_attribute :preferred_article_ids, (adjusted_input || []) + end + private def generate_billboard_name diff --git a/db/migrate/20231208165353_add_article_ids_to_billboards.rb b/db/migrate/20231208165353_add_article_ids_to_billboards.rb new file mode 100644 index 000000000..1acb4aa2a --- /dev/null +++ b/db/migrate/20231208165353_add_article_ids_to_billboards.rb @@ -0,0 +1,9 @@ +class AddArticleIdsToBillboards < ActiveRecord::Migration[7.0] + def up + add_column :display_ads, :preferred_article_ids, :integer, array: true, default: [] + end + + def down + safety_assured { remove_column :display_ads, :preferred_article_ids, :integer, array: true } + end +end diff --git a/db/migrate/20231208165454_add_article_ids_index_to_billboards.rb b/db/migrate/20231208165454_add_article_ids_index_to_billboards.rb new file mode 100644 index 000000000..99cd99d18 --- /dev/null +++ b/db/migrate/20231208165454_add_article_ids_index_to_billboards.rb @@ -0,0 +1,7 @@ +class AddArticleIdsIndexToBillboards < ActiveRecord::Migration[7.0] + disable_ddl_transaction! + + def change + add_index :display_ads, :preferred_article_ids, using: 'gin', algorithm: :concurrently + end +end diff --git a/db/schema.rb b/db/schema.rb index 52ccf714d..332600fce 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.0].define(version: 2023_11_29_135338) do +ActiveRecord::Schema[7.0].define(version: 2023_12_08_165454) do # These are extensions that must be enabled in order to support this database enable_extension "citext" enable_extension "ltree" @@ -483,6 +483,7 @@ ActiveRecord::Schema[7.0].define(version: 2023_11_29_135338) do t.string "name" t.bigint "organization_id" t.string "placement_area" + t.integer "preferred_article_ids", default: [], array: true t.boolean "priority", default: false t.text "processed_html" t.boolean "published", default: false @@ -496,6 +497,7 @@ ActiveRecord::Schema[7.0].define(version: 2023_11_29_135338) do t.index ["cached_tag_list"], name: "index_display_ads_on_cached_tag_list", opclass: :gin_trgm_ops, using: :gin t.index ["exclude_article_ids"], name: "index_display_ads_on_exclude_article_ids", using: :gin t.index ["placement_area"], name: "index_display_ads_on_placement_area" + t.index ["preferred_article_ids"], name: "index_display_ads_on_preferred_article_ids", using: :gin t.index ["target_geolocations"], name: "gist_index_display_ads_on_target_geolocations", using: :gist end diff --git a/spec/models/billboard_spec.rb b/spec/models/billboard_spec.rb index 3d5bb1a19..c9d09f28d 100644 --- a/spec/models/billboard_spec.rb +++ b/spec/models/billboard_spec.rb @@ -553,36 +553,79 @@ RSpec.describe Billboard do end describe ".weighted_random_selection" do - it "samples with weights correctly" do - described_class.delete_all - bb1 = create(:billboard, weight: 5) - bb2 = create(:billboard, weight: 1) - bb3 = create(:billboard, weight: 1) - bb4 = create(:billboard, weight: 2) - bb5 = create(:billboard, weight: 1) + context "when no target_article_id is provided" do + it "samples with weights correctly" do + described_class.delete_all + bb1 = create(:billboard, weight: 5) + bb2 = create(:billboard, weight: 1) + bb3 = create(:billboard, weight: 1) + bb4 = create(:billboard, weight: 2) + bb5 = create(:billboard, weight: 1) - total_weight = 5 + 1 + 1 + 2 + 1 # 10 - expected_probabilities = { - bb1.id => 5.0 / total_weight, - bb2.id => 1.0 / total_weight, - bb3.id => 1.0 / total_weight, - bb4.id => 2.0 / total_weight, - bb5.id => 1.0 / total_weight - } + total_weight = 5 + 1 + 1 + 2 + 1 # 10 + expected_probabilities = { + bb1.id => 5.0 / total_weight, + bb2.id => 1.0 / total_weight, + bb3.id => 1.0 / total_weight, + bb4.id => 2.0 / total_weight, + bb5.id => 1.0 / total_weight + } - counts = Hash.new(0) - num_trials = 5_000 + counts = Hash.new(0) + num_trials = 5_000 - num_trials.times do - id = described_class.weighted_random_selection(described_class.all).id - counts[id] += 1 + num_trials.times do + id = described_class.weighted_random_selection(described_class.all).id + counts[id] += 1 + end + + counts.each do |id, count| + observed_probability = count.to_f / num_trials + expected_probability = expected_probabilities[id] + + expect(observed_probability).to be_within(0.025).of(expected_probability) + end end + end - counts.each do |id, count| - observed_probability = count.to_f / num_trials - expected_probability = expected_probabilities[id] + context "when a target_article_id is provided" do + it "favors records containing the target_article_id" do + allow(FeatureFlag).to receive(:enabled?).with(:article_id_adjusted_weight).and_return(true) + described_class.delete_all + target_article_id = 123 + favored_weight_multiplier = 10 - expect(observed_probability).to be_within(0.025).of(expected_probability) + # Create billboards with different weights and article_ids + bb1 = create(:billboard, weight: 5, preferred_article_ids: [target_article_id]) + bb2 = create(:billboard, weight: 1, preferred_article_ids: []) + bb3 = create(:billboard, weight: 1, preferred_article_ids: [target_article_id]) + bb4 = create(:billboard, weight: 2, preferred_article_ids: []) + bb5 = create(:billboard, weight: 1, preferred_article_ids: []) + + # Adjusted total weight accounting for the favored records + total_weight = (5 * favored_weight_multiplier) + 1 + (1 * favored_weight_multiplier) + 2 + 1 + expected_probabilities = { + bb1.id => (5.0 * favored_weight_multiplier) / total_weight, + bb2.id => 1.0 / total_weight, + bb3.id => (1.0 * favored_weight_multiplier) / total_weight, + bb4.id => 2.0 / total_weight, + bb5.id => 1.0 / total_weight + } + + counts = Hash.new(0) + num_trials = 5_000 + + num_trials.times do + id = described_class.weighted_random_selection(described_class.all, target_article_id).id + counts[id] += 1 + end + + counts.each do |id, count| + observed_probability = count.to_f / num_trials + expected_probability = expected_probabilities[id] + + expect(observed_probability).to be_within(0.025).of(expected_probability) + end end end end diff --git a/spec/requests/api/v1/billboards_spec.rb b/spec/requests/api/v1/billboards_spec.rb index d6311e30c..4610e5831 100644 --- a/spec/requests/api/v1/billboards_spec.rb +++ b/spec/requests/api/v1/billboards_spec.rb @@ -52,7 +52,7 @@ RSpec.describe "Api::V1::Billboards" do "success_rate", "tag_list", "type_of", "updated_at", "creator_id", "exclude_article_ids", "audience_segment_type", "audience_segment_id", - "custom_display_label", "template", "render_mode", + "custom_display_label", "template", "render_mode", "preferred_article_ids", "priority", "weight", "target_geolocations") expect(response.parsed_body["target_geolocations"]).to contain_exactly("US-WA", "CA-BC") end @@ -72,7 +72,7 @@ RSpec.describe "Api::V1::Billboards" do "success_rate", "tag_list", "type_of", "updated_at", "creator_id", "exclude_article_ids", "audience_segment_type", "audience_segment_id", - "custom_display_label", "template", "render_mode", + "custom_display_label", "template", "render_mode", "preferred_article_ids", "priority", "weight", "target_geolocations") expect(response.parsed_body["target_geolocations"]).to contain_exactly("US-WA", "CA-BC") end @@ -138,7 +138,7 @@ RSpec.describe "Api::V1::Billboards" do "success_rate", "tag_list", "type_of", "updated_at", "creator_id", "exclude_article_ids", "audience_segment_type", "audience_segment_id", - "custom_display_label", "template", "render_mode", + "custom_display_label", "template", "render_mode", "preferred_article_ids", "priority", "weight", "target_geolocations") end