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

fix: speed up Detour queries by returning subsets of JSON snapshot #2946

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 14 additions & 37 deletions lib/notifications/db/detour.ex
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,12 @@ defmodule Notifications.Db.Detour do
from(d in Notifications.Db.Detour, as: :detour_notification, select_merge: d)
end

def with_detour(query \\ base()) do
with_named_binding(query, :detour, fn query, binding ->
join(query, :left, [detour_notification: n], assoc(n, ^binding), as: ^binding)
end)
end

@doc """
Retrieves detour information for notifications from the `Notifications.Db.Detour` table

Expand All @@ -77,7 +83,7 @@ defmodule Notifications.Db.Detour do
...> |> Notifications.Notification.create_activated_detour_notification_from_detour()
...>
iex> all_detour_notifications =
...> Notifications.Db.Detour.Queries.get_derived_info()
...> Notifications.Db.Detour.Queries.select_detour_notification_info()
...> |> Skate.Repo.all()
...>
iex> [
Expand All @@ -92,42 +98,13 @@ defmodule Notifications.Db.Detour do
false

"""
def get_derived_info(query \\ base()) do
from(
[detour_notification: dn] in query,
left_join: ad in assoc(dn, :detour),
as: :associated_detour,
select_merge: %{
route: ad.state["context"]["route"]["name"],
origin: ad.state["context"]["routePattern"]["name"],
headsign: ad.state["context"]["routePattern"]["headsign"],

# Ecto can't figure out how to index a JSON map via another JSON value
# because (in the ways it was tried) Ecto won't allow us to use the
# value from the associated detour, `ad`, as a value in the
# ["JSON path"](https://hexdocs.pm/ecto/Ecto.Query.API.html#json_extract_path/2).
#
# i.e., this
# ad.state["context"]["route"]["directionNames"][
# ad.state["context"]["routePattern"]["directionId"]
# ]
#
# But, Postgres _is_ able to do this, _if_ we get the types correct.
# A JSON value in Postgres is either of type JSON or JSONB, but
# - indexing a JSON array requires an `INTEGER`,
# - accessing a JSON map, requires Postgres's `TEXT` type.
#
# So because we know the `directionId` will correspond to the keys in
# `directionNames`, casting the `directionId` to `TEXT` allows us to
# access the `directionNames` JSON map
direction:
fragment(
"? -> CAST(? AS TEXT)",
ad.state["context"]["route"]["directionNames"],
ad.state["context"]["routePattern"]["directionId"]
)
}
)
def select_detour_notification_info(query \\ base()) do
query
|> with_detour()
|> Skate.Detours.Db.Detour.Queries.select_route_name(:route)
|> Skate.Detours.Db.Detour.Queries.select_route_pattern_name(:origin)
|> Skate.Detours.Db.Detour.Queries.select_route_pattern_headsign(:headsign)
|> Skate.Detours.Db.Detour.Queries.select_direction(:direction)
Comment on lines +103 to +107
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not quite happy with this, but I wasn't sure about changing the virtual field names. would be happy to discuss though.

Copy link
Member Author

Choose a reason for hiding this comment

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

#2948 is why I'm not quite happy with this, as I'm not sure how to hide these functions from the public API while also allowing renaming for situations like this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah, I don't have any better ideas here, unfortunately

end
end
end
3 changes: 2 additions & 1 deletion lib/notifications/db/notification.ex
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,8 @@ defmodule Notifications.Db.Notification do
@spec select_detour_info() :: Ecto.Query.t()
def select_detour_info(query \\ base()) do
from([notification: n] in query,
left_join: detour in subquery(Notifications.Db.Detour.Queries.get_derived_info()),
left_join:
detour in subquery(Notifications.Db.Detour.Queries.select_detour_notification_info()),
on: detour.id == n.detour_id,
select_merge: %{
detour: detour
Expand Down
203 changes: 203 additions & 0 deletions lib/skate/detours/db/detour.ex
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ defmodule Skate.Detours.Db.Detour do

alias Skate.Settings.Db.User

@type detour_status :: :active | :draft | :past

typed_schema "detours" do
field :state, :map
belongs_to :author, User
Expand All @@ -16,6 +18,31 @@ defmodule Skate.Detours.Db.Detour do
field :activated_at, :utc_datetime_usec

timestamps()

## Detour virtual fields
# -------------------------------------------------------
field(:status, Ecto.Enum, values: [:draft, :active, :past], virtual: true) ::
detour_status() | nil

# Route properties
field :route_id, :string, virtual: true
field :route_name, :string, virtual: true
field :route_pattern_id, :string, virtual: true
field :route_pattern_name, :string, virtual: true
field :headsign, :string, virtual: true
field :direction, :string, virtual: true

# Default detour properties
field :nearest_intersection, :string, virtual: true

# Activated properties
field :estimated_duration, :string, virtual: true

# Temporary field to make querying the `:state` faster and avoid needing to
# pull the entire `:state` value
field :state_value, :map, virtual: true

# -------------------------------------------------------
end

def changeset(detour, attrs) do
Expand All @@ -24,4 +51,180 @@ defmodule Skate.Detours.Db.Detour do
|> validate_required([:state])
|> foreign_key_constraint(:author_id)
end

defmodule Queries do
@moduledoc """
Defines composable queries for retrieving `Skate.Detours.Db.Detour`
"""

import Ecto.Query

def base() do
# Select nothing (`[]`) at first so further `select_merge`'s
# don't have an issue
from(Skate.Detours.Db.Detour, as: :detour, select: [])
end

@doc """
Builds a query that _selects_ data from columns or extracts from the JSON `:state`.
"""
def select_fields(query \\ base(), fields) when is_list(fields) do
%{virtual_fields: wanted_virtual_fields, fields: wanted_fields} = split_fields(fields)

query
|> add_author?(wanted_fields)
|> select_merge(^wanted_fields)
|> select_virtual_fields(wanted_virtual_fields)
end

defp split_fields(input_fields) do
schema_virtual_fields = Skate.Detours.Db.Detour.__schema__(:virtual_fields)

{virtual_fields, fields} =
Enum.split_with(input_fields, fn field -> field in schema_virtual_fields end)

%{
virtual_fields: virtual_fields,
fields: fields
}
end

defp select_virtual_fields(query, fields) when is_list(fields) do
Enum.reduce(fields, query, fn field, query ->
case field do
:route_id -> select_route_id(query)
:route_name -> select_route_name(query)
:route_pattern_id -> select_route_pattern_id(query)
:route_pattern_name -> select_route_pattern_name(query)
:headsign -> select_route_pattern_headsign(query)
:direction -> select_direction(query)
:nearest_intersection -> select_starting_intersection(query)
:estimated_duration -> select_estimated_duration(query)
:state_value -> select_state_value(query, :state_value)
_unknown -> query
end
end)
end

def sorted_by_last_updated(query \\ base()) do
order_by(query, desc: :updated_at)
end

defp add_author?(query, fields) do
if Keyword.has_key?(fields, :author) do
with_author(query)
else
query
end
end

@doc """
Joins the `Skate.Settings.Db.User` struct into the `Skate.Detours.Db.Detour`
via Ecto preload.

> ### Primary Keys required in query when using `with_author/1` {:.warning}
> When preloading structs, Ecto requires that primary key fields are also
> queried on all preloaded structs.
> This means that when querying `:author` via `select_fields`, you need to
> explicitly request `:id` on both the `Skate.Detours.Db.Detour` and the
> `Skate.Settings.Db.User`.
"""
def with_author(query \\ base()) do
from([detour: d] in query,
join: a in assoc(d, :author),
preload: [author: a]
)
end

def select_detour_list_info(query \\ base()) do
query
|> select_fields([
# Table Columns
:id,
:author_id,
:activated_at,
:updated_at,

# Virtual Fields
:route_id,
:route_name,
:route_pattern_id,
:route_pattern_name,
:headsign,
:direction,
:nearest_intersection,
:estimated_duration,
:state_value,

# Nested Fields
author: [:email, :id]
])
|> sorted_by_last_updated()
end

def select_route_id(query \\ base(), key \\ :route_id) do
select_merge(query, [detour: d], %{^key => d.state["context"]["route"]["id"]})
end

def select_route_name(query \\ base(), key \\ :route_name) do
select_merge(query, [detour: d], %{^key => d.state["context"]["route"]["name"]})
end

def select_route_pattern_name(query \\ base(), key \\ :route_pattern_name) do
select_merge(query, [detour: d], %{^key => d.state["context"]["routePattern"]["name"]})
end

def select_route_pattern_headsign(query \\ base(), key \\ :headsign) do
select_merge(query, [detour: d], %{^key => d.state["context"]["routePattern"]["headsign"]})
end

def select_route_pattern_id(query \\ base(), key \\ :route_pattern_id) do
select_merge(query, [detour: d], %{^key => d.state["context"]["routePattern"]["id"]})
end

def select_direction(query \\ base(), key \\ :direction) do
select_merge(query, [detour: d], %{
# Ecto can't figure out how to index a JSON map via another JSON value
# because (in the ways it was tried) Ecto won't allow us to use the
# value from the associated detour, `ad`, as a value in the
# ["JSON path"](https://hexdocs.pm/ecto/Ecto.Query.API.html#json_extract_path/2).
#
# i.e., this
# ad.state["context"]["route"]["directionNames"][
# ad.state["context"]["routePattern"]["directionId"]
# ]
#
# But, Postgres _is_ able to do this, _if_ we get the types correct.
# A JSON value in Postgres is either of type JSON or JSONB, but
# - indexing a JSON array requires an `INTEGER`,
# - accessing a JSON map, requires Postgres's `TEXT` type.
#
# So because we know the `directionId` will correspond to the keys in
# `directionNames`, casting the `directionId` to `TEXT` allows us to
# access the `directionNames` JSON map
firestack marked this conversation as resolved.
Show resolved Hide resolved
^key =>
fragment(
"? -> CAST(? AS TEXT)",
d.state["context"]["route"]["directionNames"],
d.state["context"]["routePattern"]["directionId"]
)
})
end

def select_starting_intersection(query \\ base(), key \\ :nearest_intersection) do
select_merge(query, [detour: d], %{
^key => d.state["context"]["nearestIntersection"]
})
end

def select_estimated_duration(query \\ base(), key \\ :estimated_duration) do
select_merge(query, [detour: d], %{
^key => d.state["context"]["selectedDuration"]
})
end

def select_state_value(query \\ base(), key \\ :state_value) do
select_merge(query, [detour: d], %{^key => %{"value" => d.state["value"]}})
end
firestack marked this conversation as resolved.
Show resolved Hide resolved
end
end
27 changes: 27 additions & 0 deletions lib/skate/detours/detour.ex
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,33 @@ defmodule Skate.Detours.Detour do
}
end

def from(status, %{
id: id,
author_id: author_id,
updated_at: updated_at,
route_pattern_id: route_pattern_id,
route_name: route_name,
headsign: headsign,
nearest_intersection: nearest_intersection,
direction: direction
})
when not is_nil(headsign) and
not is_nil(direction) and
not is_nil(route_name) and
not is_nil(nearest_intersection) do
%__MODULE__{
id: id,
route: route_name,
via_variant: RoutePattern.via_variant(route_pattern_id),
direction: direction,
name: headsign,
intersection: nearest_intersection,
updated_at: timestamp_to_unix(updated_at),
author_id: author_id,
status: status
}
end

def from(_status, _attrs), do: nil

# Converts the db timestamp to unix
Expand Down
23 changes: 14 additions & 9 deletions lib/skate/detours/detours.ex
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,12 @@ defmodule Skate.Detours.Detours do
[%Detour{}, ...]
"""
def list_detours do
(detour in Skate.Detours.Db.Detour)
|> from(
preload: [:author],
order_by: [desc: detour.updated_at]
)
Repo.all(Skate.Detours.Db.Detour.Queries.select_detour_list_info())
end

def list_detours(fields) do
Skate.Detours.Db.Detour.Queries.select_fields(fields)
|> Skate.Detours.Db.Detour.Queries.sorted_by_last_updated()
|> Repo.all()
end

Expand All @@ -41,7 +42,7 @@ defmodule Skate.Detours.Detours do
def active_detours_by_route(route_id) do
list_detours()
|> Enum.filter(fn detour ->
categorize_detour(detour) == :active and get_detour_route_id(detour) == route_id
categorize_detour(detour) == :active and detour.route_id == route_id
end)
|> Enum.map(fn detour -> db_detour_to_detour(detour) end)
end
Expand Down Expand Up @@ -91,9 +92,9 @@ defmodule Skate.Detours.Detours do

def db_detour_to_detour(
:active,
%Detour{
%{
activated_at: activated_at,
state: %{"context" => %{"selectedDuration" => estimated_duration}}
estimated_duration: estimated_duration
} = db_detour
) do
details = DetailedDetour.from(:active, db_detour)
Expand Down Expand Up @@ -121,7 +122,7 @@ defmodule Skate.Detours.Detours do
nil
end

@type detour_type :: :active | :draft | :past
@type detour_type :: Skate.Detours.Db.Detour.detour_status()

@doc """
Takes a `Skate.Detours.Db.Detour` struct and a `Skate.Settings.Db.User` id
Expand All @@ -131,6 +132,10 @@ defmodule Skate.Detours.Detours do
user
"""
@spec categorize_detour(detour :: map()) :: detour_type()
def categorize_detour(%{state_value: state_value}) when not is_nil(state_value) do
categorize_detour(%{state: state_value})
end

def categorize_detour(%{state: %{"value" => %{"Detour Drawing" => %{"Active" => _}}}}),
do: :active

Expand Down
Loading