From eed21a0138a640f1829a30491a83404705c1eda5 Mon Sep 17 00:00:00 2001 From: Karl-Aksel Puulmann Date: Wed, 6 Nov 2024 14:08:20 +0200 Subject: [PATCH] APIv2: Remove cruft (v2 flag, experimental_reduced_joins) (#4780) * Remove query.v2 flag This was originally used for making sure queries use the right table as migrating to APIv2. This is no longer needed * Remove experimental_reduced_joins flag --- .github/workflows/elixir.yml | 10 +-- lib/plausible/stats/breakdown.ex | 1 - .../stats/legacy/legacy_query_builder.ex | 1 - lib/plausible/stats/query.ex | 17 +--- lib/plausible/stats/sql/where_builder.ex | 10 +-- lib/plausible/stats/table_decider.ex | 55 ++++--------- lib/plausible/stats/timeseries.ex | 1 - test/plausible/stats/table_decider_test.exs | 82 ++++++++----------- test/test_helper.exs | 7 +- 9 files changed, 61 insertions(+), 123 deletions(-) diff --git a/.github/workflows/elixir.yml b/.github/workflows/elixir.yml index 30f2b3295d..73fd160e40 100644 --- a/.github/workflows/elixir.yml +++ b/.github/workflows/elixir.yml @@ -16,25 +16,25 @@ env: jobs: build: - name: "Build and test (${{ matrix.mix_env }}, ${{ matrix.postgres_image }}${{ matrix.test_read_team_schemas_and_experimental_reduced_joins == '1' && ', read_team_schemas_and_experimental_reduced_joins' || '' }})" + name: "Build and test (${{ matrix.mix_env }}, ${{ matrix.postgres_image }}${{ matrix.test_read_team_schemas == '1' && ', read_team_schemas' || '' }})" runs-on: ubuntu-latest strategy: matrix: mix_env: ["test", "ce_test"] postgres_image: ["postgres:16"] - test_read_team_schemas_and_experimental_reduced_joins: ["0"] + test_read_team_schemas: ["0"] include: - mix_env: "test" postgres_image: "postgres:15" - test_read_team_schemas_and_experimental_reduced_joins: "0" + test_read_team_schemas: "0" - mix_env: "test" postgres_image: "postgres:16" - test_read_team_schemas_and_experimental_reduced_joins: "1" + test_read_team_schemas: "1" env: MIX_ENV: ${{ matrix.mix_env }} - TEST_READ_TEAM_SCHEMAS_AND_EXPERIMENTAL_REDUCED_JOINS: ${{ matrix.test_read_team_schemas_and_experimental_reduced_joins }} + TEST_READ_TEAM_SCHEMAS: ${{ matrix.test_read_team_schemas }} services: postgres: image: ${{ matrix.postgres_image }} diff --git a/lib/plausible/stats/breakdown.ex b/lib/plausible/stats/breakdown.ex index 6e44c8437e..3e0699b605 100644 --- a/lib/plausible/stats/breakdown.ex +++ b/lib/plausible/stats/breakdown.ex @@ -31,7 +31,6 @@ defmodule Plausible.Stats.Breakdown do dimensions: transform_dimensions(dimension), filters: query.filters ++ dimension_filters(dimension), pagination: %{limit: limit, offset: (page - 1) * limit}, - v2: true, # Allow pageview and event metrics to be queried off of sessions table legacy_breakdown: true, remove_unavailable_revenue_metrics: true diff --git a/lib/plausible/stats/legacy/legacy_query_builder.ex b/lib/plausible/stats/legacy/legacy_query_builder.ex index 821424db9f..f555fa491e 100644 --- a/lib/plausible/stats/legacy/legacy_query_builder.ex +++ b/lib/plausible/stats/legacy/legacy_query_builder.ex @@ -22,7 +22,6 @@ defmodule Plausible.Stats.Legacy.QueryBuilder do |> put_preloaded_goals(site) |> put_order_by(params) |> put_include_comparisons(site, params) - |> Query.put_experimental_reduced_joins(site, params) |> Query.put_imported_opts(site, params) on_ee do diff --git a/lib/plausible/stats/query.ex b/lib/plausible/stats/query.ex index e8aa529a1f..6449344b27 100644 --- a/lib/plausible/stats/query.ex +++ b/lib/plausible/stats/query.ex @@ -10,12 +10,10 @@ defmodule Plausible.Stats.Query do include_imported: false, skip_imported_reason: nil, now: nil, - experimental_reduced_joins?: false, latest_import_end_date: nil, metrics: [], order_by: nil, timezone: nil, - v2: false, legacy_breakdown: false, remove_unavailable_revenue_metrics: false, preloaded_goals: [], @@ -34,8 +32,7 @@ defmodule Plausible.Stats.Query do query = struct!(__MODULE__, Map.to_list(query_data)) |> put_imported_opts(site, %{}) - |> put_experimental_reduced_joins(site, params) - |> struct!(v2: true, now: DateTime.utc_now(:second), debug_metadata: debug_metadata) + |> struct!(now: DateTime.utc_now(:second), debug_metadata: debug_metadata) on_ee do query = Plausible.Stats.Sampling.put_threshold(query, site, params) @@ -52,18 +49,6 @@ defmodule Plausible.Stats.Query do Legacy.QueryBuilder.from(site, params, debug_metadata, now) end - def put_experimental_reduced_joins(query, site, params) do - if Map.has_key?(params, "experimental_reduced_joins") do - struct!(query, - experimental_reduced_joins?: Map.get(params, "experimental_reduced_joins") == "true" - ) - else - struct!(query, - experimental_reduced_joins?: FunWithFlags.enabled?(:experimental_reduced_joins, for: site) - ) - end - end - def date_range(query, options \\ []) do date_range = Plausible.Stats.DateTimeRange.to_date_range(query.utc_time_range, query.timezone) diff --git a/lib/plausible/stats/sql/where_builder.ex b/lib/plausible/stats/sql/where_builder.ex index cad54c53a2..0b076bd620 100644 --- a/lib/plausible/stats/sql/where_builder.ex +++ b/lib/plausible/stats/sql/where_builder.ex @@ -7,8 +7,6 @@ defmodule Plausible.Stats.SQL.WhereBuilder do import Plausible.Stats.Time, only: [utc_boundaries: 2] import Plausible.Stats.Filters.Utils, only: [page_regex: 1] - alias Plausible.Stats.Query - use Plausible.Stats.SQL.Fragments require Logger @@ -121,10 +119,10 @@ defmodule Plausible.Stats.SQL.WhereBuilder do defp add_filter( :events, - %Query{experimental_reduced_joins?: true}, + _query, [_, "visit:" <> key | _rest] = filter ) do - # Filter events query if experimental_reduced_joins? is true + # Filter events query with visit dimension if possible field_name = String.to_existing_atom(key) if Enum.member?(@sessions_only_visit_fields, field_name) do @@ -134,10 +132,6 @@ defmodule Plausible.Stats.SQL.WhereBuilder do end end - defp add_filter(:events, _query, [_, "visit:" <> _key | _rest]) do - true - end - defp add_filter(:sessions, _query, [_, "visit:entry_props:" <> prop_name | _rest] = filter) do filter_custom_prop(prop_name, :entry_meta, filter) end diff --git a/lib/plausible/stats/table_decider.ex b/lib/plausible/stats/table_decider.ex index c798220044..5af91af420 100644 --- a/lib/plausible/stats/table_decider.ex +++ b/lib/plausible/stats/table_decider.ex @@ -12,13 +12,13 @@ defmodule Plausible.Stats.TableDecider do def events_join_sessions?(query) do query.filters |> dimensions_used_in_filters() - |> Enum.any?(&(filters_partitioner(query, &1) == :session)) + |> Enum.any?(&(dimension_partitioner(query, &1) == :session)) end def sessions_join_events?(query) do query.filters |> dimensions_used_in_filters() - |> Enum.any?(&(filters_partitioner(query, &1) == :event)) + |> Enum.any?(&(dimension_partitioner(query, &1) == :event)) end def partition_metrics(metrics, query) do @@ -34,10 +34,10 @@ defmodule Plausible.Stats.TableDecider do %{event: event_only_filters, session: session_only_filters} = query.filters |> dimensions_used_in_filters() - |> partition(query, &filters_partitioner/2) + |> partition(query, &dimension_partitioner/2) %{event: event_only_dimensions, session: session_only_dimensions} = - partition(query.dimensions, query, &filters_partitioner/2) + partition(query.dimensions, query, &dimension_partitioner/2) cond do # Only one table needs to be queried @@ -62,16 +62,16 @@ defmodule Plausible.Stats.TableDecider do end end - defp metric_partitioner(%Query{v2: true}, :conversion_rate), do: :either - defp metric_partitioner(%Query{v2: true}, :group_conversion_rate), do: :either - defp metric_partitioner(%Query{v2: true}, :visitors), do: :either - defp metric_partitioner(%Query{v2: true}, :visits), do: :either # Note: This is inaccurate when filtering but required for old backwards compatibility defp metric_partitioner(%Query{legacy_breakdown: true}, :pageviews), do: :either defp metric_partitioner(%Query{legacy_breakdown: true}, :events), do: :either - defp metric_partitioner(_, :conversion_rate), do: :event - defp metric_partitioner(_, :group_conversion_rate), do: :event + defp metric_partitioner(_, :conversion_rate), do: :either + defp metric_partitioner(_, :group_conversion_rate), do: :either + defp metric_partitioner(_, :visitors), do: :either + defp metric_partitioner(_, :visits), do: :either + defp metric_partitioner(_, :percentage), do: :either + defp metric_partitioner(_, :average_revenue), do: :event defp metric_partitioner(_, :total_revenue), do: :event defp metric_partitioner(_, :pageviews), do: :event @@ -80,42 +80,21 @@ defmodule Plausible.Stats.TableDecider do defp metric_partitioner(_, :visit_duration), do: :session defp metric_partitioner(_, :views_per_visit), do: :session - # Metrics which used to only be queried from one table but can be calculated from either - defp metric_partitioner(%Query{experimental_reduced_joins?: true}, :visits), do: :either - defp metric_partitioner(%Query{experimental_reduced_joins?: true}, :visitors), do: :either - - defp metric_partitioner(_, :visits), do: :session - defp metric_partitioner(_, :visitors), do: :event # Calculated metrics - handled on callsite separately from other metrics. defp metric_partitioner(_, :time_on_page), do: :other defp metric_partitioner(_, :total_visitors), do: :other - defp metric_partitioner(_, :percentage), do: :either # Sample percentage is included in both tables if queried. defp metric_partitioner(_, :sample_percent), do: :sample_percent - defp metric_partitioner(%Query{experimental_reduced_joins?: false}, unknown) do - raise ArgumentError, "Metric #{unknown} not supported without experimental_reduced_joins?" - end + defp dimension_partitioner(_, "event:" <> _), do: :event + defp dimension_partitioner(_, "visit:entry_page"), do: :session + defp dimension_partitioner(_, "visit:entry_page_hostname"), do: :session + defp dimension_partitioner(_, "visit:exit_page"), do: :session + defp dimension_partitioner(_, "visit:exit_page_hostname"), do: :session - defp metric_partitioner(_, _), do: :either + defp dimension_partitioner(_, "visit:" <> _), do: :either - defp filters_partitioner(_, "event:" <> _), do: :event - defp filters_partitioner(_, "visit:entry_page"), do: :session - defp filters_partitioner(_, "visit:entry_page_hostname"), do: :session - defp filters_partitioner(_, "visit:exit_page"), do: :session - defp filters_partitioner(_, "visit:exit_page_hostname"), do: :session - - defp filters_partitioner(%Query{experimental_reduced_joins?: true}, "visit:" <> _), - do: :either - - defp filters_partitioner(_, "visit:" <> _), - do: :session - - defp filters_partitioner(%Query{experimental_reduced_joins?: false}, {unknown, _}) do - raise ArgumentError, "Filter #{unknown} not supported without experimental_reduced_joins?" - end - - defp filters_partitioner(_, _), do: :either + defp dimension_partitioner(_, _), do: :either @default %{event: [], session: [], either: [], other: [], sample_percent: []} defp partition(values, query, partitioner) do diff --git a/lib/plausible/stats/timeseries.ex b/lib/plausible/stats/timeseries.ex index e5d94f6f1e..6fc8ac89d5 100644 --- a/lib/plausible/stats/timeseries.ex +++ b/lib/plausible/stats/timeseries.ex @@ -23,7 +23,6 @@ defmodule Plausible.Stats.Timeseries do metrics: transform_metrics(metrics, %{conversion_rate: :group_conversion_rate}), dimensions: [time_dimension(query)], order_by: [{time_dimension(query), :asc}], - v2: true, remove_unavailable_revenue_metrics: true ) |> QueryOptimizer.optimize() diff --git a/test/plausible/stats/table_decider_test.exs b/test/plausible/stats/table_decider_test.exs index 96d34023ff..4798554897 100644 --- a/test/plausible/stats/table_decider_test.exs +++ b/test/plausible/stats/table_decider_test.exs @@ -5,69 +5,66 @@ defmodule Plausible.Stats.TableDeciderTest do import Plausible.Stats.TableDecider describe "events_join_sessions?" do - test "with experimental_reduced_joins disabled" do - assert not events_join_sessions?(make_query(false, [])) - assert not events_join_sessions?(make_query(false, ["event:name"])) - assert events_join_sessions?(make_query(false, ["visit:source"])) - assert events_join_sessions?(make_query(false, ["visit:entry_page"])) - assert events_join_sessions?(make_query(false, ["visit:exit_page"])) - end - - test "with experimental_reduced_joins enabled" do - assert not events_join_sessions?(make_query(true, [])) - assert not events_join_sessions?(make_query(true, ["event:name"])) - assert not events_join_sessions?(make_query(true, ["visit:source"])) - assert events_join_sessions?(make_query(true, ["visit:entry_page"])) - assert events_join_sessions?(make_query(true, ["visit:exit_page"])) + test "with simple filters" do + assert not events_join_sessions?(make_query([])) + assert not events_join_sessions?(make_query(["event:name"])) + assert not events_join_sessions?(make_query(["visit:source"])) + assert events_join_sessions?(make_query(["visit:entry_page"])) + assert events_join_sessions?(make_query(["visit:exit_page"])) end test "with nested filter" do - assert not events_join_sessions?(make_query([["not", ["is", "event:name", []]]])) - assert events_join_sessions?(make_query([["not", ["is", "visit:exit_page", []]]])) + assert not events_join_sessions?( + make_query_full_filters([["not", ["is", "event:name", []]]]) + ) + + assert events_join_sessions?( + make_query_full_filters([["not", ["is", "visit:exit_page", []]]]) + ) end end describe "partition_metrics" do test "with no metrics or filters" do - query = make_query(false, []) + query = make_query([]) assert partition_metrics([], query) == {[], [], []} end test "session-only metrics accordingly" do - query = make_query(false, []) + query = make_query([]) assert partition_metrics([:bounce_rate, :views_per_visit], query) == {[], [:bounce_rate, :views_per_visit], []} end test "event-only metrics accordingly" do - query = make_query(false, []) + query = make_query([]) assert partition_metrics([:total_revenue, :visitors], query) == {[:total_revenue, :visitors], [], []} end test "filters from both, event-only metrics" do - query = make_query(false, ["event:name", "visit:source"]) + query = make_query(["event:name", "visit:source"]) assert partition_metrics([:total_revenue], query) == {[:total_revenue], [], []} end test "filters from both, session-only metrics" do - query = make_query(false, ["event:name", "visit:source"]) + query = make_query(["event:name", "visit:source"]) assert partition_metrics([:bounce_rate], query) == {[], [:bounce_rate], []} end test "session filters but no session metrics" do - query = make_query(false, ["visit:source"]) + query = make_query(["visit:source"]) assert partition_metrics([:total_revenue], query) == {[:total_revenue], [], []} end test "sample_percent is added to both types of metrics" do - query = make_query(false, []) + query = make_query([]) assert partition_metrics([:total_revenue, :sample_percent], query) == {[:total_revenue, :sample_percent], [], []} @@ -80,24 +77,14 @@ defmodule Plausible.Stats.TableDeciderTest do end test "other metrics put in its own result" do - query = make_query(false, []) + query = make_query([]) assert partition_metrics([:time_on_page, :percentage, :total_visitors], query) == {[], [:percentage], [:time_on_page, :total_visitors]} end - test "raises if unknown metric" do - query = make_query(false, []) - - assert_raise ArgumentError, fn -> - partition_metrics([:foobar], query) - end - end - end - - describe "partition_metrics with experimental_reduced_joins enabled" do test "metrics that can be calculated on either when event-only metrics" do - query = make_query(true, []) + query = make_query([]) assert partition_metrics([:total_revenue, :visitors], query) == {[:total_revenue, :visitors], [], []} @@ -106,7 +93,7 @@ defmodule Plausible.Stats.TableDeciderTest do end test "metrics that can be calculated on either when session-only metrics" do - query = make_query(true, []) + query = make_query([]) assert partition_metrics([:bounce_rate, :visitors], query) == {[], [:bounce_rate, :visitors], []} @@ -116,64 +103,63 @@ defmodule Plausible.Stats.TableDeciderTest do end test "metrics that can be calculated on either are biased to events" do - query = make_query(true, []) + query = make_query([]) assert partition_metrics([:bounce_rate, :total_revenue, :visitors], query) == {[:total_revenue, :visitors], [:bounce_rate], []} end test "sample_percent is handled with either metrics" do - query = make_query(true, []) + query = make_query([]) assert partition_metrics([:visitors, :sample_percent], query) == {[], [:visitors, :sample_percent], []} end test "metric can be calculated on either, but filtering on events" do - query = make_query(true, ["event:name"]) + query = make_query(["event:name"]) assert partition_metrics([:visitors], query) == {[:visitors], [], []} end test "metric can be calculated on either, but filtering on events and sessions" do - query = make_query(true, ["event:name", "visit:exit_page"]) + query = make_query(["event:name", "visit:exit_page"]) assert partition_metrics([:visitors], query) == {[], [:visitors], []} end test "metric can be calculated on either, filtering on either" do - query = make_query(true, ["visit:source"]) + query = make_query(["visit:source"]) assert partition_metrics([:visitors], query) == {[], [:visitors], []} end test "metric can be calculated on either, filtering on sessions" do - query = make_query(true, ["visit:exit_page"]) + query = make_query(["visit:exit_page"]) assert partition_metrics([:visitors], query) == {[], [:visitors], []} end test "query dimensions lean metric" do - assert partition_metrics([:visitors], make_query(true, [], ["event:name"])) == + assert partition_metrics([:visitors], make_query([], ["event:name"])) == {[:visitors], [], []} - assert partition_metrics([:visitors], make_query(true, [], ["visit:source"])) == + assert partition_metrics([:visitors], make_query([], ["visit:source"])) == {[], [:visitors], []} - assert partition_metrics([:visitors], make_query(true, [], ["visit:exit_page"])) == + assert partition_metrics([:visitors], make_query([], ["visit:exit_page"])) == {[], [:visitors], []} end end - defp make_query(experimental_reduced_joins?, filter_keys, dimensions \\ []) do + defp make_query(filter_keys, dimensions \\ []) do Query.from(build(:site), %{ - "experimental_reduced_joins" => to_string(experimental_reduced_joins?), "filters" => Enum.map(filter_keys, fn filter_key -> ["is", filter_key, []] end), "dimensions" => dimensions }) end - defp make_query(filters) do + defp make_query_full_filters(filters) do Query.from(build(:site), %{ "dimensions" => [], "filters" => filters diff --git a/test/test_helper.exs b/test/test_helper.exs index d405644c56..963a846083 100644 --- a/test/test_helper.exs +++ b/test/test_helper.exs @@ -6,14 +6,11 @@ end Mox.defmock(Plausible.HTTPClient.Mock, for: Plausible.HTTPClient.Interface) Application.ensure_all_started(:double) -# Temporary flag to test `read_team_schemas` and `experimental_reduced_joins` -# flags on all tests. -if System.get_env("TEST_READ_TEAM_SCHEMAS_AND_EXPERIMENTAL_REDUCED_JOINS") == "1" do +# Temporary flag to test `read_team_schemas` flag on all tests. +if System.get_env("TEST_READ_TEAM_SCHEMAS") == "1" do FunWithFlags.enable(:read_team_schemas) - FunWithFlags.enable(:experimental_reduced_joins) else FunWithFlags.disable(:read_team_schemas) - FunWithFlags.disable(:experimental_reduced_joins) end Ecto.Adapters.SQL.Sandbox.mode(Plausible.Repo, :manual)