Feature/chapter15#14
Conversation
- benchmark/ディレクトリに独立したベンチマーク環境を構築 - 軽量化されたテストデータ（学生100件、履修300件など） - ASCII表形式の結果表示 - JSON出力サポート（CI用） - 既存テストとの完全分離
…- HeuristicQueryPlanner/TablePlannerを教科書ロジックに忠実に再実装\n- テストを単体・機能・ベンチマークに分割し、拡張性・可読性を向上\n- ベンチマーク結果をシンプルな表形式で出力し、複数プランナー・複数クエリに対応\n- 不要な旧テストファイルを削除 (benchmark_test.go, heuristic_validation_test.go)\n- その他細部のリファクタ・コメント整理
There was a problem hiding this comment.
Pull Request Overview
This PR implements Chapter 15 features including query optimization with multi-buffer algorithms, heuristic query planning, and enhanced plan visualization. The implementation adds sophisticated join optimization strategies and improves buffer management for better database performance.
- Implements multi-buffer product scan and chunk scan for optimized large table operations
- Adds heuristic query planner with intelligent join ordering and selection pushdown
- Enhances existing plan classes with getter methods and improved error handling
Reviewed Changes
Copilot reviewed 27 out of 29 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tx/buffer_list.go | Adds null check for buffer existence before unpinning |
| server/server.go | Refactors constants and adds custom buffer size initialization |
| scan/chunk_scan.go | Implements new chunk scan for multi-buffer operations |
| query/multi_buffer_product_scan.go | Adds multi-buffer product scan implementation |
| query/index_select_scan.go | Updates to match scan.Scan interface requirements |
| query/index_join_scan.go | Updates BeforeFirst method signature for interface compliance |
| plan/table_planner*.go | Implements table-level query planning with optimization strategies |
| plan/*_plan.go | Adds getter methods and improves error handling across plan types |
| plan/plan_visualization.go | Adds plan tree visualization utilities |
| plan/heuristic_query_planner.go | Implements heuristic-based query optimization |
| metadata/table_manager.go | Adds error handling for non-existent tables |
| buffer/buffer_needs.go | Implements buffer allocation optimization functions |
| benchmark/ | Adds comprehensive benchmark framework for query optimization |
| .github/workflows/ | Updates CI configuration with benchmark workflow |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| var i int32 | ||
| for i = range int32(len(c.buffs)) { |
There was a problem hiding this comment.
The loop variable declaration and range usage is unconventional. Use a standard for loop: for i := int32(0); i < int32(len(c.buffs)); i++ {
| var i int32 | |
| for i = range int32(len(c.buffs)) { | |
| for i = 0; i < int32(len(c.buffs)); i++ { |
| scan.BeforeFirst() | ||
|
|
||
| return scan, nil | ||
| } | ||
|
|
||
| func (m *MultiBufferProductScan) BeforeFirst() { | ||
| m.nextBlockNum = 0 | ||
| // FIXME: エラー握りつぶし | ||
| _, err := m.useNextChunk() | ||
| if err != nil { | ||
| log.Printf("failed to invoke MultiBufferProductScan#BeforeFirst(): %v", err) | ||
| } |
There was a problem hiding this comment.
Error suppression with FIXME comment indicates technical debt. Either handle the error properly or document why it's safe to ignore.
| scan.BeforeFirst() | |
| return scan, nil | |
| } | |
| func (m *MultiBufferProductScan) BeforeFirst() { | |
| m.nextBlockNum = 0 | |
| // FIXME: エラー握りつぶし | |
| _, err := m.useNextChunk() | |
| if err != nil { | |
| log.Printf("failed to invoke MultiBufferProductScan#BeforeFirst(): %v", err) | |
| } | |
| if err := scan.BeforeFirst(); err != nil { | |
| return nil, fmt.Errorf("failed to initialize MultiBufferProductScan: %w", err) | |
| } | |
| return scan, nil | |
| } | |
| func (m *MultiBufferProductScan) BeforeFirst() error { | |
| m.nextBlockNum = 0 | |
| _, err := m.useNextChunk() | |
| if err != nil { | |
| return fmt.Errorf("failed to invoke MultiBufferProductScan#BeforeFirst(): %w", err) | |
| } | |
| return nil |
| // BeforeFirst はscan.Scanインターフェースに準拠（戻り値なし） | ||
| func (iss *IndexSelectScan) BeforeFirst() { | ||
| // エラーが発生した場合はパニックせず、ログに記録するか無視する | ||
| // Chapter 15では簡単のためエラーハンドリングを簡略化 | ||
| iss.idx.BeforeFirst(iss.val) | ||
| } |
There was a problem hiding this comment.
Silent error suppression without any logging or handling mechanism makes debugging difficult. At minimum, log errors when they occur.
| // これは結合条件がない場合の最後の手段として使用される | ||
| func (tp *TablePlanner) MakeProductPlan(current Plan) Plan { | ||
| selectPlan := tp.addSelectPred(tp.myPlan) | ||
| return NewMultiBufferProductPcan(tp.tx, current, selectPlan) |
There was a problem hiding this comment.
Function name has a typo: 'NewMultiBufferProductPcan' should be 'NewMultiBufferProductPlan'
| return NewMultiBufferProductPcan(tp.tx, current, selectPlan) | |
| return NewMultiBufferProductPlan(tp.tx, current, selectPlan) |
| if val, ok := sp.pred.EquatesWithConstant(fldName); ok { | ||
| _ = val // val を使う処理があればここに。なければ無視してokだけで判断 |
There was a problem hiding this comment.
The blank identifier assignment with Japanese comment suggests unused variable. Remove the assignment or use the value: if _, ok := sp.pred.EquatesWithConstant(fldName); ok {
| if val, ok := sp.pred.EquatesWithConstant(fldName); ok { | |
| _ = val // val を使う処理があればここに。なければ無視してokだけで判断 | |
| if _, ok := sp.pred.EquatesWithConstant(fldName); ok { |
| for table := range tables { | ||
| tableName := table | ||
| tp := NewTablePlanner(tableName, data.Pred(), tx, hqp.mdm) |
There was a problem hiding this comment.
Unnecessary variable assignment. Use the range variable directly: for tableName := range tables {
| for table := range tables { | |
| tableName := table | |
| tp := NewTablePlanner(tableName, data.Pred(), tx, hqp.mdm) | |
| for tableName := range tables { | |
| tp := NewTablePlanner(tableName, data.Pred(), tx, hqp.mdm) |
| if actualRecords > 30000 { // 無限ループ防止カウンターを調整 | ||
| break | ||
| } |
There was a problem hiding this comment.
Magic number 30000 should be defined as a constant: const MaxBenchmarkRecords = 30000
No description provided.