From 7b9685a6ea831330e76c3da697d42bd0f0916044 Mon Sep 17 00:00:00 2001 From: angelsl Date: Fri, 5 Jun 2020 02:22:12 +0800 Subject: [PATCH] Optimise SQL queries for /user and /assessments --- lib/cadet/assessments/assessments.ex | 100 ++++++++-------- lib/cadet/assessments/query.ex | 117 +++---------------- lib/cadet_web/controllers/user_controller.ex | 3 +- test/cadet/assessments/query_test.exs | 38 ------ 4 files changed, 65 insertions(+), 193 deletions(-) diff --git a/lib/cadet/assessments/assessments.ex b/lib/cadet/assessments/assessments.ex index 9ce50fb8a..edbd95cf2 100644 --- a/lib/cadet/assessments/assessments.ex +++ b/lib/cadet/assessments/assessments.ex @@ -73,28 +73,6 @@ defmodule Cadet.Assessments do Repo.delete_all(submissions) end - @spec user_total_xp(%User{}) :: integer() - def user_total_xp(%User{id: user_id}) when is_ecto_id(user_id) do - total_xp_bonus = - Submission - |> where(student_id: ^user_id) - |> Repo.aggregate(:sum, :xp_bonus) - |> case do - nil -> 0 - xp when is_integer(xp) -> xp - end - - total_xp = - Query.all_submissions_with_xp() - |> subquery() - |> where(student_id: ^user_id) - |> select([q], fragment("? + ?", sum(q.xp), sum(q.xp_adjustment))) - |> Repo.one() - |> decimal_to_integer() - - total_xp_bonus + total_xp - end - @spec user_max_grade(%User{}) :: integer() def user_max_grade(%User{id: user_id}) when is_ecto_id(user_id) do Submission @@ -111,13 +89,29 @@ defmodule Cadet.Assessments do |> decimal_to_integer() end - def user_total_grade(%User{id: user_id}) do - Query.all_submissions_with_grade() - |> subquery() - |> where(student_id: ^user_id) - |> select([q], fragment("? + ?", sum(q.grade), sum(q.adjustment))) - |> Repo.one() - |> decimal_to_integer() + def user_total_grade_xp(%User{id: user_id}) do + submission_grade_xp = + Submission + |> where(student_id: ^user_id) + |> join(:inner, [s], a in Answer, on: s.id == a.submission_id) + |> group_by([s], s.id) + |> select([s, a], %{ + total_grade: sum(a.grade) + sum(a.adjustment), + # grouping by submission, so s.xp_bonus will be the same, but we need an + # aggregate function + total_xp: sum(a.xp) + sum(a.xp_adjustment) + max(s.xp_bonus) + }) + + total = + submission_grade_xp + |> subquery + |> select([s], %{ + total_grade: sum(s.total_grade), + total_xp: sum(s.total_xp) + }) + |> Repo.one() + + for {key, val} <- total, into: %{}, do: {key, decimal_to_integer(val)} end defp decimal_to_integer(decimal) do @@ -277,34 +271,40 @@ defmodule Cadet.Assessments do by the supplied user """ def all_assessments(user = %User{}) do + submission_aggregates = + Submission + |> join(:left, [s], ans in Answer, on: ans.submission_id == s.id) + |> where([s], s.student_id == ^user.id) + |> group_by([s], s.assessment_id) + |> select([s, ans], %{ + assessment_id: s.assessment_id, + grade: fragment("? + ?", sum(ans.grade), sum(ans.adjustment)), + # s.xp_bonus should be the same across the group, but we need an aggregate function here + xp: fragment("? + ? + ?", sum(ans.xp), sum(ans.xp_adjustment), max(s.xp_bonus)), + graded_count: ans.id |> count() |> filter(not is_nil(ans.grader_id)) + }) + + submission_status = + Submission + |> where([s], s.student_id == ^user.id) + |> select([s], [:assessment_id, :status]) + assessments = - Query.all_assessments_with_max_xp_and_grade() + Query.all_assessments_with_aggregates() |> subquery() |> join( :left, [a], - s in subquery(Query.all_submissions_with_xp_and_grade()), - on: a.id == s.assessment_id and s.student_id == ^user.id - ) - |> join( - :left, - [a, _], - q_count in subquery(Query.assessments_question_count()), - on: a.id == q_count.assessment_id - ) - |> join( - :left, - [_, s, _], - a_count in subquery(Query.submissions_graded_count()), - on: s.id == a_count.submission_id + sa in subquery(submission_aggregates), + on: a.id == sa.assessment_id ) - |> select([a, s, q_count, a_count], %{ + |> join(:left, [a, _], s in subquery(submission_status), on: a.id == s.assessment_id) + |> select([a, sa, s], %{ a - | xp: fragment("? + ? + ?", s.xp, s.xp_adjustment, s.xp_bonus), - grade: fragment("? + ?", s.grade, s.adjustment), - user_status: s.status, - question_count: q_count.count, - graded_count: a_count.count + | xp: sa.xp, + grade: sa.grade, + graded_count: sa.graded_count, + user_status: s.status }) |> filter_published_assessments(user) |> order_by(:open_at) diff --git a/lib/cadet/assessments/query.ex b/lib/cadet/assessments/query.ex index 05a32f0da..34ced60e1 100644 --- a/lib/cadet/assessments/query.ex +++ b/lib/cadet/assessments/query.ex @@ -4,58 +4,22 @@ defmodule Cadet.Assessments.Query do """ import Ecto.Query - alias Cadet.Assessments.{Answer, Assessment, Question, Submission} - - @doc """ - Returns a query with the following bindings: - [submissions_with_xp_and_grade, answers] - """ - @spec all_submissions_with_xp_and_grade :: Ecto.Query.t() - def all_submissions_with_xp_and_grade do - Submission - |> join(:inner, [s], q in subquery(submissions_xp_and_grade()), on: s.id == q.submission_id) - |> select([s, q], %Submission{ - s - | xp: q.xp, - xp_adjustment: q.xp_adjustment, - grade: q.grade, - adjustment: q.adjustment - }) - end - - @doc """ - Returns a query with the following bindings: - [submissions_with_xp, answers] - """ - @spec all_submissions_with_xp :: Ecto.Query.t() - def all_submissions_with_xp do - Submission - |> join(:inner, [s], q in subquery(submissions_xp()), on: s.id == q.submission_id) - |> select([s, q], %Submission{s | xp: q.xp, xp_adjustment: q.xp_adjustment}) - end - - @doc """ - Returns a query with the following bindings: - [submissions_with_grade, answers] - """ - @spec all_submissions_with_grade :: Ecto.Query.t() - def all_submissions_with_grade do - Submission - |> join(:inner, [s], q in subquery(submissions_grade()), on: s.id == q.submission_id) - |> select([s, q], %Submission{s | grade: q.grade, adjustment: q.adjustment}) - end + alias Cadet.Assessments.{Assessment, Question} @doc """ Returns a query with the following bindings: [assessments_with_xp_and_grade, questions] """ - @spec all_assessments_with_max_xp_and_grade :: Ecto.Query.t() - def all_assessments_with_max_xp_and_grade do + @spec all_assessments_with_aggregates :: Ecto.Query.t() + def all_assessments_with_aggregates do Assessment - |> join(:inner, [a], q in subquery(assessments_max_xp_and_grade()), - on: a.id == q.assessment_id - ) - |> select([a, q], %Assessment{a | max_grade: q.max_grade, max_xp: q.max_xp}) + |> join(:inner, [a], q in subquery(assessments_aggregates()), on: a.id == q.assessment_id) + |> select([a, q], %Assessment{ + a + | max_grade: q.max_grade, + max_xp: q.max_xp, + question_count: q.question_count + }) end @doc """ @@ -69,41 +33,6 @@ defmodule Cadet.Assessments.Query do |> select([a, q], %Assessment{a | max_grade: q.max_grade}) end - @spec submissions_xp_and_grade :: Ecto.Query.t() - def submissions_xp_and_grade do - Answer - |> group_by(:submission_id) - |> select([a], %{ - submission_id: a.submission_id, - grade: sum(a.grade), - adjustment: sum(a.adjustment), - xp: sum(a.xp), - xp_adjustment: sum(a.xp_adjustment) - }) - end - - @spec submissions_grade :: Ecto.Query.t() - def submissions_grade do - Answer - |> group_by(:submission_id) - |> select([a], %{ - submission_id: a.submission_id, - grade: sum(a.grade), - adjustment: sum(a.adjustment) - }) - end - - @spec submissions_xp :: Ecto.Query.t() - def submissions_xp do - Answer - |> group_by(:submission_id) - |> select([a], %{ - submission_id: a.submission_id, - xp: sum(a.xp), - xp_adjustment: sum(a.xp_adjustment) - }) - end - @spec assessments_max_grade :: Ecto.Query.t() def assessments_max_grade do Question @@ -111,33 +40,15 @@ defmodule Cadet.Assessments.Query do |> select([q], %{assessment_id: q.assessment_id, max_grade: sum(q.max_grade)}) end - @spec assessments_max_xp_and_grade :: Ecto.Query.t() - def assessments_max_xp_and_grade do + @spec assessments_aggregates :: Ecto.Query.t() + def assessments_aggregates do Question |> group_by(:assessment_id) |> select([q], %{ assessment_id: q.assessment_id, max_grade: sum(q.max_grade), - max_xp: sum(q.max_xp) - }) - end - - def assessments_question_count do - Question - |> group_by(:assessment_id) - |> select([q], %{ - assessment_id: q.assessment_id, - count: count(q.id) - }) - end - - def submissions_graded_count do - Answer - |> where([a], not is_nil(a.grader_id)) - |> group_by(:submission_id) - |> select([a], %{ - submission_id: a.submission_id, - count: count(a.id) + max_xp: sum(q.max_xp), + question_count: count(q.id) }) end end diff --git a/lib/cadet_web/controllers/user_controller.ex b/lib/cadet_web/controllers/user_controller.ex index 575409784..e16e1f064 100644 --- a/lib/cadet_web/controllers/user_controller.ex +++ b/lib/cadet_web/controllers/user_controller.ex @@ -10,10 +10,9 @@ defmodule CadetWeb.UserController do def index(conn, _) do user = user_with_group(conn.assigns.current_user) - grade = user_total_grade(user) + %{total_grade: grade, total_xp: xp} = user_total_grade_xp(user) max_grade = user_max_grade(user) story = user_current_story(user) - xp = user_total_xp(user) game_states = user_game_states(user) render( diff --git a/test/cadet/assessments/query_test.exs b/test/cadet/assessments/query_test.exs index b6976f70d..ab314dc06 100644 --- a/test/cadet/assessments/query_test.exs +++ b/test/cadet/assessments/query_test.exs @@ -3,26 +3,6 @@ defmodule Cadet.Assessments.QueryTest do alias Cadet.Assessments.Query - test "all_submissions_with_grade/1" do - submission = insert(:submission) - assessment = insert(:assessment) - questions = insert_list(5, :question, assessment: assessment) - - Enum.each( - questions, - &insert(:answer, submission: submission, grade: 200, adjustment: -100, question: &1) - ) - - result = - Query.all_submissions_with_grade() - |> where(id: ^submission.id) - |> Repo.one() - - submission_id = submission.id - - assert %{grade: 1000, adjustment: -500, id: ^submission_id} = result - end - test "all_assessments_with_max_grade" do assessment = insert(:assessment) insert_list(5, :question, assessment: assessment, max_grade: 200) @@ -37,24 +17,6 @@ defmodule Cadet.Assessments.QueryTest do assert %{max_grade: 1000, id: ^assessment_id} = result end - test "submissions_grade" do - submission = insert(:submission) - assessment = insert(:assessment) - questions = insert_list(5, :question, assessment: assessment) - - Enum.each( - questions, - &insert(:answer, submission: submission, grade: 200, adjustment: -100, question: &1) - ) - - result = - Query.submissions_grade() - |> Repo.all() - |> Enum.find(&(&1.submission_id == submission.id)) - - assert %{grade: 1000, adjustment: -500, submission_id: submission.id} == result - end - test "assessments_max_grade" do assessment = insert(:assessment) insert_list(5, :question, assessment: assessment, max_grade: 200)