LoginSignup
7
2

【Laravel】paginate() + distinct() で総件数が重複レコード分多くなる問題を回避(おまけ:PRの議論がアツい)

Last updated at Posted at 2023-12-09

この記事は、Qiita AdventCalendar2023 ラクスパートナーズ10日目の記事です。
この記事では、私が業務で遭遇したLaravel のpaginate() と distinct() を組み合わせたコードが、「取得したレコードは重複削除してあるのに、総件数(ページ数)は重複削除されていないという結果を表示した」問題を解決する方法を紹介します。

取得したレコード28, 1ページあたり10件なのに...
1AFF45FA-A70C-4328-8457-66D821068687.png
(ちなみに重複削除しないと196レコードで計算が合う)

また、この記事を書くに当たってLaravel へ今回の方法を実装した有志のプルリクエストを読んでいたら、思っていた以上にOSSへのPR読むの面白いかも...🤔と思わせてくれた議論も最後におまけで紹介します。

想定読者

  • PHP, Laravel エンジニア
  • 既存のテーブル設計のおかげで、欲しい行がどうしても重複してしまうが、重複削除した結果が欲しい人(これが私でした)
  • プロジェクトXみたいな、誰も解決していなかった問題を解決するドラマが好きなエンジニア(おまけで紹介するPRの話を面白いと思うんじゃないかな)

バージョン

  • Laravel 6.0~(もしかしたら5.9~も可)
  • MySQL 5.7~(必須では無い)

今回の要件

例えば、次のようなテーブルにおいて
qiita_2023_12_10.drawio (7).png
(このテーブルは簡略版です。また。テーブル設計についてのご指摘は勘弁してください。これが既にあるものと思ってください。)

次の要件を満たすコードを書きます。

  1. ordersを一覧表示したい。(paginateも行う)
  2. ただし、ordered_productsのうち、ordered_products.canceled_at がNULLであるもののみを対象とします。
  3. 1つのorderに対して、複数のordered_productsが存在します。
  4. processesには、1製品につき5レコードが生成され、それぞれのレコードがprocessに'create', 'check', 'ship'など工程を表す文字列を持ち、どの工程にいるのかをprocess_statusで管理します。(重複したレコードを返す原因)

TL;DR

このように書けば、問題を起こさず実装できます。

OrderController
public function index() {
    $ordersWithoutIncludeCanceledProduct = Order::query()
        ->select('orders.*')
        ->join('processes', 'processes.order_id', 'order.order_id')
        ->join('ordered_products', 'ordered_products.orderd_product_id', 'processes.orderd_product_id')
        ->whereNull('ordered_products.canceled_at')
        ->distinct(['orders.order_id'])
        ->paginate(10);
        
    return view('orders.index', compact('ordersWithoutIncludeCanceledProduct'))
}

ポイント

ポイントは、以下の一文です。

->distinct(['orders.order_id'])

distinct()の引数に

['重複削除の検査対象とするカラム名']

を渡すことがポイントでした。

問題が発生するコード

以下のコードでは上記のコードと発行するSQLは同じですが、paginateが用意する総件数が取得するレコード数と乖離してしまい、表示するとレコードは空なのにやけにページがある状態になってしまいます。

Problem1OrderController
public function index() {
    $ordersWithoutIncludeCanceledProduct = Order::query()
        // OrderControllerと同じ
        ...
        ->distinct()
        ->paginate(10);
        
        ...
}
Problem2OrderController
public function index() {
    $ordersWithoutIncludeCanceledProduct = Order::query()
        // OrderControllerと同じ
        ...
        ->distinct()
        ->paginate(10, ['orders.order_id']);
        
        ...
}

また、distinct()ではなくgruopBy()を使うときは、DBMS側でエラーが発生する可能性があります。

Problem3OrderController
public function index() {
    $ordersWithoutIncludeCanceledProduct = Order::query()
        // OrderControllerと同じ
        ...
        ->groupBy('orders.order_id')
        ->paginate(10);
        
        ...
}
MySQL
sqlstate[42000]: syntax error or access violation: 1055 expression #1 of select list is not in group by clause and contains nonaggregated column 'orders.customer_name' which is not functionally dependent on columns in group by clause;
this is incompatible with sql_mode=only_full_group_by...

MySQL は、関数従属性の検出を実装しています。 ONLY_FULL_GROUP_BY SQL モードが有効な場合 (デフォルト)、MySQL は、選択リスト、HAVING 条件または ORDER BY リストが GROUP BY 句で名前が付けられておらず、機能的に依存していない非集計カラムを参照するクエリーを拒否します。
12.20.3 MySQL での GROUP BY の処理

ただし、これに関しては以下のようにすることでエラーを回避することができますが...。

->groupBy('selectするカラム', ...) // selectするカラムをすべてGROUP BY

selectするカラムが非常に多いときなど、書いてらんないですよね...。

ちなみに、GROUP BY と DISTINCT の違いについて

以下の記事が分かりやすかったです。
SQL: DISTINCT と GROUP BY って,何がちがうの?

また一部、「GROUP BY は実行速度が遅い!」「いや、DISTINCT の方が実行速度が遅い!」などあるようですが、実行計画や以下を見る限り、どちらを使っても性能の差は見られないようです。

ほとんどの場合、DISTINCT 句は GROUP BY の特殊な例と考えることができます。 たとえば、次の 2 つのクエリーは同等です。

SELECT DISTINCT c1, c2, c3 FROM t1
WHERE c1 > const;
SELECT c1, c2, c3 FROM t1
WHERE c1 > const GROUP BY c1, c2, c3;

この同等性のため、GROUP BY クエリーに適用できる最適化は DISTINCT 句のあるクエリーにも適用できます。 そのため、DISTINCT クエリー最適化の可能性の詳細については、セクション8.2.1.17「GROUP BY の最適化」を参照してください。
8.2.1.18 DISTINCT の最適化

このDocがversion 5.7 から存在していたので、この記事はMySQL 5.7~としました。

以上、Laravel のpaginate() と distinct() を組み合わせたコードが変な総件数を返さないようにする方法でした。

おまけ

現在、実務経験2ヶ月のペーペーですが、今回は書いてみたいドリブンでアドベントカレンダーに参加しました。(記事を書くのも初めて)
せっかく書くなら、ということで色々と情報を漁っていると、今回の方法を実装してくれた有志の方の、その時のLaravelへのPRを発見しました。
このPRを読んでかなりアツくなったのでご紹介したいと思います。
[6.0] Make LengthAwarePaginator respect distinct columns

どうやら、かなり前からこの問題は知られていたものの、有効かつフレームワークに対して非破壊的な方法を提案できたPRは無かったようです。(この状況が既にアツい)

image.png

そんな中、erikgaal という方が提出したPRにて、今回ご紹介した
distinct()の引数へカラムを与える」というLaravel の実装を考え出し、見事マージされた、というわけです。
この人のおかげで私が業務で救われたんですね。

image.png

しかし、マージまでは一筋縄では行きませんでした。
Laravel の開発者、Taylor Otwellはこう言います。
image.png
「この問題について、なぜこれが他のすべてのダメだったPRと違うのか、徹底的な説明が必要だ。」

erikgaal は答えました。
image.png
「それらと違うことを証明する、複数のテストを提供しました。
 1.現在の正常な動作を壊していない(distinct以外の)
 2.現在のget()->count()paginate()->total()の数が異なるという壊れているwww動作をまだサポートしている 
 3.私たちが見たい動作(一様なカウント)である
 問題がないことの証明として他に何が必要ですか?」(いいね2)

実際、このPRへは全体を通して多くの肯定的なコメントが見られます。

ただ実は、このPRが提出されたタイミングがなんとも微妙でした。
image.png
「このPRは5.8リリース前に提案され、5.8は昨日リリースされた。私はこのPRを5.8に取り込むことに問題はないと思う。」(賛否両論)

ここで、このPRの焦点は3つありました。

  1. このPRは本当に既存のフレームワークへ破壊的な変更を加えていないのか?
  2. この変更により、様々なDBMSに対して問題は起こらないのか?
  3. このPRは5.8に取り込むのか?

1に対しては、Taylor Otwellも良い反応をくれました。

image.png
「うーん、コードを見直すと、シグネチャの変更以外に既存のアプリケーションを壊す可能性があるとは思えない。」
(このPRでは、既存のdistinct()の引数を変えずにfunc_get_args()を使うことで、Builderクラスを拡張するときにも問題のないよう配慮しています。)

次に、2 に対してはerikgaal の調査により、MySQL, PotsgreSQL で問題のないよう調整が加えられました。
image.png

最後に、3のタイミングについて。実は途中で1ヶ月ほどこのPRが放置されていました。
image.png
「やあ、taylorotwellとチームさん、このPRの方向性は?」

そこから更に1ヶ月後、ようやく...
image.png
「5.9でマージするよう、調整してもいいかな?」
「僕は大丈夫だよ」

ということで、無事マージされました。

最後に、
image.png
「erikgaal、君がこの問題を解決した幸運な成功者だといいね!:)」
 
調子いいなコイツ


ということで、LaravelのLaravel のpaginate() と distinct() の問題の解決方法を提案し、見事承認されたerikgaalさんの熱意と、この解決方法をみんなに伝えたいと思った次第でした。

本当に長文になってしまいましたが、もしここまで読んでくれた方がいたとしたら、心から感謝いたします。

このPRの中には他にも、的外れなテクニックをコメントしてサムダウンを6人分もらっている人がいたりと、面白かったです。
image.png
「メチャ簡単だぜ。」(全然簡単でもないし、根本的な解決にもなってない)

今回は偶然読んでみる機会があったわけですが、OSSのPRを読むのも面白いなと思いました(英語ばっかりなので尻込みはしますが)。

それでは、読んでいただきありがとうございました!

7
2
0

Register as a new user and use Qiita more conveniently

  1. You get articles that match your needs
  2. You can efficiently read back useful information
  3. You can use dark theme
What you can do with signing up
7
2