fix(core,layout): Resolve review findings across core and layout#185
Merged
Conversation
import_schema only rejected empty identifiers; duplicate non-empty ids silently collapsed entries because diff/overlay key tables by stable_id (last-wins in the HashMap). Reject duplicate table/view/enum ids with a clear ImportError.
The depth field was public, so direct struct construction (in the CLI and app) skipped the MAX_FOCUS_DEPTH clamp that FocusSpec::new and serde applied. Make depth private with a depth() accessor and route every construction site through FocusSpec::new.
SchemaGraph::from_schema duplicated the graph builder that lives in relune-layout, was only exercised by relune-core's own tests, and had diverged (it never emitted view->view dependency edges and carried a dead always-true guard). Production builds go through the layout builder, so remove SchemaGraph/GraphNode/GraphEdge/GraphBuildError and the EnumIndex helper. NodeKind/EdgeKind/SqlRelation/collect_sql_relations stay (the layout crate depends on them). BREAKING CHANGE: SchemaGraph and related types are no longer exported.
count_crossings packed (rank, position) into a usize assuming each fits in 16 bits and asserted otherwise, so a rank with more than 65536 nodes panicked. Pack into a u64 (32 bits each) and drop the asserts; the limit is now unreachable for any real schema.
separate_force_groups checked connectivity with force_nodes_are_connected, which rescanned every edge per call, making the group-vs-group case O(group^2 * E) and the whole pass quadratic. Build the connected node-index pairs once into a HashSet so each check is an O(1) lookup; behaviour is unchanged (connectivity is a boolean).
Mermaid erDiagram relationship labels are quoted strings that do not honor backslash escapes, so a literal quote terminated the string and a backslash-escaped bracket rendered the backslash verbatim. Replace quotes with apostrophes and control characters with spaces for relationship labels instead of reusing the attribute-comment escaper.
Node coordinates are mirrored for RightToLeft (horizontal) and BottomToTop (vertical) before edges are routed, but self-loops were always built bulging east, so they pointed the wrong way for mirrored directions. Reflect the self-loop polyline across the node's own center to follow the mirror. Only self-loop geometry changes (regular-edge routes and node positions are unchanged); the right_to_left and bottom_to_top regression snapshots are updated accordingly.
resolve_edge_label_collisions rebuilt the full obstacle list (every node plus every other edge's route, markers and label) for each edge on every relaxation pass, giving O(E^2) behaviour that dominated layout time on large schemas. Index node and edge obstacle footprints in a uniform grid and query only those near each edge's reachable label region. place_label_on_route's overlap-area sum is order-sensitive (f32 is not associative), so the grid returns candidates in ascending index order and rebuilds the original obstacle ordering (nodes, this edge's endpoint markers, then other edges by index). Excluded obstacles lie outside the reachable region and contribute exactly zero, so output is byte-for-byte identical.
FK column names were already lowercased when diffing, but the target table/schema were compared verbatim, so a case-only difference surfaced as a spurious modification (or, for unnamed FKs, an add/remove pair). The layout graph builder likewise matched FK source columns to table columns case-sensitively, diverging from schema validation. Fold both to case-insensitive comparison.
build_prefix_groups compared every pair of table names before the graph was built. A pair can only share a group prefix when their common prefix has at least two alphanumeric characters, and for sorted names the common prefix shrinks monotonically, so each name only needs comparing against the surrounding run that still meets that bound. The adjacency edge set (and thus the resulting groups) is identical to the exhaustive scan, including the non-contiguous component case now covered by a test.
Code Metrics Report
Details | | main (3a4c823) | #185 (6a8b0ed) | +/- |
|---------------------|----------------|----------------|-------|
+ | Coverage | 94.4% | 94.4% | +0.0% |
| Files | 100 | 100 | 0 |
| Lines | 37180 | 36683 | -497 |
- | Covered | 35123 | 34657 | -466 |
- | Test Execution Time | 1m37s | 1m49s | +12s |Code coverage of files in pull request scope (96.1% → 96.2%)Reported by octocov |
Schema reviewTip ✅ No risk findings — schema changes look safe to merge. |
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
stable_idrejection on import, and an un-bypassableFocusSpecdepth clamp.SchemaGraphbuilder fromrelune-core(production builds use the layout builder).Changes
import_schemanow rejects duplicate (not just empty) table/view/enum identifiers, preventing silent last-wins collapse in diff/overlay which key bystable_id.FocusSpec::depthprivate with adepth()accessor and route every construction site throughFocusSpec::new, so theMAX_FOCUS_DEPTHclamp can no longer be skipped by direct struct construction.SchemaGraph/GraphNode/GraphEdge/GraphBuildErrorand theEnumIndexhelper; production graph building lives inrelune-layoutand the removed builder had diverged (no view→view edges, dead guard).NodeKind/EdgeKind/SqlRelation/collect_sql_relationssince the layout crate depends on them.u64(32 bits each) instead of a 16-bit-assertedusize, removing the panic for ranks with more than 65536 nodes.HashSetso group-packing connectivity checks are O(1) lookups instead of a full edge rescan per call.RightToLeft/BottomToTopso loops follow the mirrored node coordinates instead of always bulging east; only self-loop geometry changes, with regression snapshots updated.