Optimize GetStationsByLineIdList performance#1441
Merged
Merged
Conversation
…t query - Pre-index lines by station_g_cd into a HashMap for O(1) lookup, replacing the O(n×m) linear scan in update_station_vec_with_attributes - Add get_by_station_group_id_vec_no_types that skips station_station_types and types JOINs when train type data is unused (line_group_id is None) - Add quality/performance-first guideline to AGENTS.md Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughユースケースに skip_types_join フラグを追加し、ドメイン／インフラ層に「型情報を結合しない」駅取得経路を追加。駅→線の紐付け処理をインデックス化して再構築。ドキュメント（AGENTS.md）に性能重視の貢献指針を追記。 Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant UseCase as QueryUseCase
participant Repo as StationRepository
participant DB as Database
Client->>UseCase: リクエスト (station_group_ids, skip_types_join=true)
UseCase->>Repo: get_by_station_group_id_vec_no_types(ids)
Repo->>DB: SELECT from stations, lines, line_aliases, aliases (type列はNULL)
DB-->>Repo: rows
Repo-->>UseCase: Vec<Station>
UseCase->>UseCase: lines を HashMap にインデックス化、各 station に対して station_lines を構築
UseCase-->>Client: 整形済みの駅リスト
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@stationapi/src/use_case/interactor/query.rs`:
- Around line 1910-1915: The mock implementation of
get_by_station_group_id_vec_no_types currently just forwards to
get_by_station_group_id_vec, so tests won't exercise the "no-types" branch;
change get_by_station_group_id_vec_no_types to return fixtures that mirror
production get_by_station_group_id_vec_no_types behavior by filtering out
line_group_cd, pass, and any type_* fields from each Station (i.e., call
get_by_station_group_id_vec to get base fixtures then map them through the same
trimming logic used by update_station_vec_with_attributes or the real no-types
code), so tests validate the no-types path and catch regressions when
update_station_vec_with_attributes changes.
- Around line 261-273: The current branch uses line_group_id.is_some() to choose
get_stations_by_group_id_vec_no_types (via
get_stations_by_group_id_vec_no_types) which is too broad and strips line
attributes for other callers like update_station_vec_with_attributes; change the
branching to depend on an explicit flag/parameter (e.g., use_no_types or
optimize_for_line_id_list) instead of line_group_id, wire that flag only from
the specific GetStationsByLineIdList call site, and keep all other callers using
get_stations_by_group_id_vec so update_station_vec_with_attributes still
receives full data (references: line_group_id,
get_stations_by_group_id_vec_no_types, get_stations_by_group_id_vec,
get_lines_by_station_group_id_vec, update_station_vec_with_attributes). Also
add/adjust tests in QueryInteractor to assert stations/lines/station
numbers/train types enrichment remains intact when line_group_id is None for
other code paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a728ccfb-9cf2-4191-b098-549a8d3c97aa
📒 Files selected for processing (4)
AGENTS.mdstationapi/src/domain/repository/station_repository.rsstationapi/src/infrastructure/station_repository.rsstationapi/src/use_case/interactor/query.rs
…es optimization - Add skip_types_join parameter to update_station_vec_with_attributes to decouple the optimization from line_group_id semantics - Only pass true from get_stations_by_line_id_vec (GetStationsByLineIdList), keeping all other callers on the full query to preserve station type data - Update ConfigurableMock get_by_station_group_id_vec_no_types to strip type fields (line_group_cd, pass, sst_id, type_cd, type_id, type_name, color, direction, kind) mirroring production behavior Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
stationapi/src/use_case/interactor/query.rs (1)
1922-1948:⚠️ Potential issue | 🟡 Minorno-types モックが派生フィールドを落とし切れていません。
ここは
pass/type_id系をNoneにしていますが、has_train_types・stop_condition・train_typeは元の fixture のまま残ります。get_by_station_group_id_vec_no_types()の本番戻り値より情報が多いので、skip_types_join経路のテストが実挙動より甘くなります。✂️ テストダブルを本番寄りにする差分例
async fn get_by_station_group_id_vec_no_types( &self, _: &[u32], ) -> Result<Vec<Station>, DomainError> { let stations = self .stations_by_group .iter() .map(|s| { let mut s = s.clone(); s.line_group_cd = None; s.pass = None; s.sst_id = None; s.type_cd = None; s.type_id = None; s.type_name = None; s.type_name_k = None; s.type_name_r = None; s.type_name_zh = None; s.type_name_ko = None; s.color = None; s.direction = None; s.kind = None; + s.has_train_types = false; + s.stop_condition = crate::proto::StopCondition::All; + s.train_type = None; s }) .collect(); Ok(stations) }Based on learnings:
Verify that helper methods such as update_station_vec_with_attributes and build_route_tree_map behave correctly when modifying QueryInteractor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stationapi/src/use_case/interactor/query.rs` around lines 1922 - 1948, The mock implementation of get_by_station_group_id_vec_no_types leaves derived fields set (e.g., has_train_types, stop_condition, train_type) causing tests to be too permissive; update the function to explicitly set those fields to None/false as appropriate on each cloned Station (in addition to the existing pass/type_id/type_* nulling) so the mock matches the production "no types / skip_types_join" response; ensure changes align with how update_station_vec_with_attributes and build_route_tree_map expect stations to look when QueryInteractor modifies stations.
🧹 Nitpick comments (2)
stationapi/src/use_case/traits/query.rs (1)
63-69:QueryUseCaseに最適化用のboolを露出しない方がよいです。
skip_types_joinは interactor 内部の join 戦略そのもので、契約に乗ると呼び出し側まで未使用の最適化知識を持つことになります。今回もstationapi/src/presentation/controller/grpc.rsのモックまで引数が波及しているので、enumで意味を型にするか、この helper 自体をQueryInteractor内に閉じた方が API を保ちやすいです。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stationapi/src/use_case/traits/query.rs` around lines 63 - 69, The trait QueryUseCase currently exposes an optimization flag skip_types_join on the method update_station_vec_with_attributes; remove this bool from the public trait signature and either (A) introduce a small enum (e.g., TypesJoinStrategy with variants Default and Skip) and use that enum in update_station_vec_with_attributes to encode meaningfully the strategy, or (B) move the helper method update_station_vec_with_attributes entirely into the QueryInteractor impl so optimization details stay private; update all implementations (QueryInteractor::update_station_vec_with_attributes) and the grpc controller mock to match the new signature (or call the interactor helper instead), and adjust callers to use the enum variant or rely on the interactor behavior so no raw bool leaks through the public API.stationapi/src/use_case/interactor/query.rs (1)
201-215:skip_types_join=trueを通る回帰テストを 1 本追加したいです。
trueを渡す呼び出しはここだけですが、今回の追加テストはほぼfalse側のシグネチャ追従に留まっています。get_stations_by_line_id_vec()経由で、no-types 取得に切り替わってもlines/company/station_numbersの enrich が維持されることを固定しておくと、この最適化を安心して触れます。 Based on learnings:Verify that helper methods such as update_station_vec_with_attributes and build_route_tree_map behave correctly when modifying QueryInteractor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stationapi/src/use_case/interactor/query.rs` around lines 201 - 215, Add a regression test that exercises the skip_types_join=true path by calling QueryInteractor::get_stations_by_line_id_vec with representative line_ids and transport filter, stubbing the station_repository to return base station rows (simulate no-type join) and then asserting that the returned Station objects still have their lines, company, and station_numbers enriched; ensure the test triggers the call path that passes true into update_station_vec_with_attributes and also exercises build_route_tree_map/helper enrichment logic so any change to QueryInteractor that switches to no-types retrieval is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@stationapi/src/use_case/interactor/query.rs`:
- Around line 1922-1948: The mock implementation of
get_by_station_group_id_vec_no_types leaves derived fields set (e.g.,
has_train_types, stop_condition, train_type) causing tests to be too permissive;
update the function to explicitly set those fields to None/false as appropriate
on each cloned Station (in addition to the existing pass/type_id/type_* nulling)
so the mock matches the production "no types / skip_types_join" response; ensure
changes align with how update_station_vec_with_attributes and
build_route_tree_map expect stations to look when QueryInteractor modifies
stations.
---
Nitpick comments:
In `@stationapi/src/use_case/interactor/query.rs`:
- Around line 201-215: Add a regression test that exercises the
skip_types_join=true path by calling
QueryInteractor::get_stations_by_line_id_vec with representative line_ids and
transport filter, stubbing the station_repository to return base station rows
(simulate no-type join) and then asserting that the returned Station objects
still have their lines, company, and station_numbers enriched; ensure the test
triggers the call path that passes true into update_station_vec_with_attributes
and also exercises build_route_tree_map/helper enrichment logic so any change to
QueryInteractor that switches to no-types retrieval is covered.
In `@stationapi/src/use_case/traits/query.rs`:
- Around line 63-69: The trait QueryUseCase currently exposes an optimization
flag skip_types_join on the method update_station_vec_with_attributes; remove
this bool from the public trait signature and either (A) introduce a small enum
(e.g., TypesJoinStrategy with variants Default and Skip) and use that enum in
update_station_vec_with_attributes to encode meaningfully the strategy, or (B)
move the helper method update_station_vec_with_attributes entirely into the
QueryInteractor impl so optimization details stay private; update all
implementations (QueryInteractor::update_station_vec_with_attributes) and the
grpc controller mock to match the new signature (or call the interactor helper
instead), and adjust callers to use the enum variant or rely on the interactor
behavior so no raw bool leaks through the public API.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3610097a-9322-4fab-83c2-cdebd74e9370
📒 Files selected for processing (3)
stationapi/src/presentation/controller/grpc.rsstationapi/src/use_case/interactor/query.rsstationapi/src/use_case/traits/query.rs
…rent impl - Remove update_station_vec_with_attributes from QueryUseCase trait to keep the skip_types_join optimization detail private to QueryInteractor - Remove the method from the gRPC controller mock (no longer needed) - Add has_train_types and train_type clearing to the ConfigurableMock get_by_station_group_id_vec_no_types to match production behavior where line_group_cd=NULL makes has_train_types=false - Add regression test for skip_types_join=true path verifying that lines, company, and station_numbers are still enriched correctly Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
stationapi/src/use_case/interactor/query.rs (1)
953-959: このフラグはモード型にした方が安全です。今は各呼び出し側に
true/falseだけが残るので意味を追いづらく、将来skip_types_join = trueとline_group_id = Some(_)を誤って組み合わせやすいです。enum化するか no-types 専用 helper に分けて、不変条件を型で表した方が保守しやすいです。Based on learnings: Verify that helper methods such as
update_station_vec_with_attributesandbuild_route_tree_mapbehave correctly when modifyingQueryInteractor🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stationapi/src/use_case/interactor/query.rs` around lines 953 - 959, The boolean flag skip_types_join is ambiguous and error-prone; replace it with a descriptive enum (e.g., AttributesMode { IncludeTypes, SkipTypes }) used by update_station_vec_with_attributes and related helpers (such as build_route_tree_map) so callers pass intent not a raw bool; update the signature of update_station_vec_with_attributes, adjust all callers to use the new enum, and enforce incompatible states (e.g., prevent AttributesMode::SkipTypes being used with a Some(line_group_id)) either by constructor validation or by pattern-matching early in QueryInteractor so the invariant is encoded in types/control flow rather than scattered true/false values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@stationapi/src/use_case/interactor/query.rs`:
- Around line 1033-1048: The cloned station used to populate line.station is
taken before the station is fully enriched, causing station.line.station to
reference an outdated copy; move the clone so it happens after you set
station.station_numbers and station.train_type and just before assigning
station.line. Specifically, in the loop that calls extract_line_from_station and
get_line_symbols, apply get_station_numbers and the train_type_map lookup
(get_station_numbers, station.station_numbers, train_type_map,
station.train_type) on the original station first, then create the cloned
station and assign it to the Line's station field before setting station.line =
Some(Box::new(line)); this ensures line.station references the fully enriched
station and keeps company_map/company_cd logic unchanged.
---
Nitpick comments:
In `@stationapi/src/use_case/interactor/query.rs`:
- Around line 953-959: The boolean flag skip_types_join is ambiguous and
error-prone; replace it with a descriptive enum (e.g., AttributesMode {
IncludeTypes, SkipTypes }) used by update_station_vec_with_attributes and
related helpers (such as build_route_tree_map) so callers pass intent not a raw
bool; update the signature of update_station_vec_with_attributes, adjust all
callers to use the new enum, and enforce incompatible states (e.g., prevent
AttributesMode::SkipTypes being used with a Some(line_group_id)) either by
constructor validation or by pattern-matching early in QueryInteractor so the
invariant is encoded in types/control flow rather than scattered true/false
values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 177bdaf1-a69e-4804-940d-5866cfdf34cb
📒 Files selected for processing (3)
stationapi/src/presentation/controller/grpc.rsstationapi/src/use_case/interactor/query.rsstationapi/src/use_case/traits/query.rs
💤 Files with no reviewable changes (2)
- stationapi/src/presentation/controller/grpc.rs
- stationapi/src/use_case/traits/query.rs
Move line.station assignment after station_numbers and train_type are set so the cloned station embedded in the line is fully populated. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
stationapi/src/use_case/interactor/query.rs (2)
2809-2849:⚠️ Potential issue | 🟡 Minorこのテストだと
skip_types_join分岐を取り違えても通ってしまいます。
stations_by_groupの fixture が最初からline_group_cd/pass/type_*を持っていないので、実装が誤ってget_by_station_group_id_vec()を呼んでも観測結果が変わりません。station_result.lines[*].stationに no-types 経路の差分が反映されていることまで assert しないと、この最適化の回帰を拾えないです。差分が見える fixture / assert にする例
async fn test_update_station_vec_with_attributes_skip_types_join_enriches_correctly() { let company = create_test_company(1, "JR東日本"); - let mut station = create_test_station(101, 1001, 100, None); + let mut station = create_test_station(101, 1001, 100, Some(1000)); station.company_cd = Some(1); + station.pass = Some(1); + station.type_id = Some(10); + station.type_cd = Some(20); + station.type_name = Some("快速".to_string()); + station.has_train_types = true; let mut line = create_test_line(100); line.station_g_cd = Some(1001); @@ assert!(!station_result.lines.is_empty()); assert!(station_result.line.is_some()); assert!(!station_result.station_numbers.is_empty()); + + let nested_station = station_result.lines[0] + .station + .as_ref() + .expect("line.station should be populated"); + assert!(nested_station.line_group_cd.is_none()); + assert!(nested_station.pass.is_none()); + assert!(nested_station.type_id.is_none()); + assert!(!nested_station.has_train_types); // train_type should be None since line_group_id is None assert!(station_result.train_type.is_none());Based on learnings: Verify that helper methods such as
update_station_vec_with_attributesandbuild_route_tree_mapbehave correctly when modifyingQueryInteractor🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stationapi/src/use_case/interactor/query.rs` around lines 2809 - 2849, The test test_update_station_vec_with_attributes_skip_types_join_enriches_correctly is masking a bug because the stations_by_group fixture lacks line_group_cd/pass/type_* so calling the wrong branch (get_by_station_group_id_vec) still passes; update the test to seed stations_by_group with a route that includes line_group_cd/pass/type_* (and a corresponding no-types route) and assert that update_station_vec_with_attributes (and indirectly build_route_tree_map) applies the no-types diff into station_result.lines[*].station (i.e., verify that station_result.lines[].station reflects the no-types modifications), ensuring the code path that skips the types join is actually exercised and will fail on regressions.
302-302:⚠️ Potential issue | 🟡 Minor
get_stations_by_line_group_id_vec()だけstation.line.station.train_typeが古いまま残ります。Line 302 の呼び出しは
update_station_vec_with_attributes(..., None, ...)なので、line.stationは train type 未設定の状態で clone されます。ところが Lines 315-329 ではstation.train_typeとstation.lines[*].station.train_typeしか更新しておらず、station.line.station.train_typeだけNoneのままです。修正イメージ
for station in stations.iter_mut() { station.train_type = train_type_map .get(&station.station_cd) .cloned() .cloned() .map(Box::new); + if let Some(ref mut primary_line) = station.line { + if let Some(ref mut nested_station) = primary_line.station { + nested_station.train_type = train_type_map + .get(&nested_station.station_cd) + .cloned() + .cloned() + .map(Box::new); + } + } for line in station.lines.iter_mut() { if let Some(ref mut nested_station) = line.station { nested_station.train_type = train_type_map .get(&nested_station.station_cd) .cloned()Based on learnings: Ensure
QueryInteractorenriches station responses with lines, companies, station numbers, and train types through proper enrichment steps🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stationapi/src/use_case/interactor/query.rs` at line 302, get_stations_by_line_group_id_vec clones station.line with train_type left None because update_station_vec_with_attributes is called with None for the lines' inner station, so station.line.station.train_type remains unset; fix by ensuring the same enrichment step that sets station.train_type and station.lines[*].station.train_type also updates station.line.station.train_type (e.g., after update_station_vec_with_attributes(stations, None, transport_type, false) run the train-type enrichment over each station.line.station or call update_station_vec_with_attributes with the correct nested data instead of None) so that get_stations_by_line_group_id_vec returns fully enriched train_type fields for station.line.station as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@stationapi/src/use_case/interactor/query.rs`:
- Around line 2809-2849: The test
test_update_station_vec_with_attributes_skip_types_join_enriches_correctly is
masking a bug because the stations_by_group fixture lacks
line_group_cd/pass/type_* so calling the wrong branch
(get_by_station_group_id_vec) still passes; update the test to seed
stations_by_group with a route that includes line_group_cd/pass/type_* (and a
corresponding no-types route) and assert that update_station_vec_with_attributes
(and indirectly build_route_tree_map) applies the no-types diff into
station_result.lines[*].station (i.e., verify that
station_result.lines[].station reflects the no-types modifications), ensuring
the code path that skips the types join is actually exercised and will fail on
regressions.
- Line 302: get_stations_by_line_group_id_vec clones station.line with
train_type left None because update_station_vec_with_attributes is called with
None for the lines' inner station, so station.line.station.train_type remains
unset; fix by ensuring the same enrichment step that sets station.train_type and
station.lines[*].station.train_type also updates station.line.station.train_type
(e.g., after update_station_vec_with_attributes(stations, None, transport_type,
false) run the train-type enrichment over each station.line.station or call
update_station_vec_with_attributes with the correct nested data instead of None)
so that get_stations_by_line_group_id_vec returns fully enriched train_type
fields for station.line.station as well.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1cf88c7a-e8ca-4284-b8d0-81edfa523a1c
📒 Files selected for processing (1)
stationapi/src/use_case/interactor/query.rs
…ine_group_id_vec The train_type enrichment loop was updating station.train_type and station.lines[*].station.train_type but missed station.line.station.train_type. Also improve skip_types_join test to seed stations_by_group with actual type fields and verify they are stripped by the no_types code path. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Member
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@stationapi/src/use_case/interactor/query.rs`:
- Around line 1047-1059: When skip_types_join=true the code clones the original
Station (station.clone()) into line.station without removing type-related
fields, so the response can contain inconsistent representations; before
assigning or cloning (the block that sets station.station_numbers, possibly
within QueryInteractor methods), normalize/trim the Station by removing type_*
fields, pass, line_group_cd, and has_train_types (reuse the existing
normalization logic used by update_station_vec_with_attributes and
build_route_tree_map) or call a shared helper to produce a no-types version,
then assign that sanitized Station into line.station and station.line to ensure
both references are consistent when skip_types_join is enabled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a640da1a-d298-42ae-9c49-3bb3bda758d6
📒 Files selected for processing (1)
stationapi/src/use_case/interactor/query.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
station_g_cdinto aHashMap<i32, Vec<usize>>before the enrichment loop, replacing O(n×m) linear scan with O(1) lookup per stationget_by_station_group_id_vec_no_typesthat skipsstation_station_typesandtypesJOINs whenline_group_idis None (i.e.,GetStationsByLineIdList), since train type data is unused in that pathComplexity improvement
Test plan
SQLX_OFFLINE=true cargo buildpassesSQLX_OFFLINE=true cargo test— all 26 tests pass🤖 Generated with Claude Code
Summary by CodeRabbit
パフォーマンス改善
ドキュメント
テスト / リファクタ