fix(memory): skip non-dict rows in describe_schema - #2540
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough
ChangesSchema description guards
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
core/wren/src/wren/memory/schema_indexer.py (1)
99-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrevent a dangling "Columns:" header for entirely malformed columns.
If the
columnslist only contains malformed entries (e.g.,["bad"]),colsevaluates to true and" Columns:"is appended to the output. However, since the loop correctly skips"bad", no columns are rendered, resulting in an empty "Columns:" section.Filtering valid columns before appending the header ensures a cleaner output.
♻️ Proposed refactor to avoid empty headers
cols = model.get("columns", []) or [] - if isinstance(cols, list) and cols: - lines.append(" Columns:") - for col in cols: - if isinstance(col, dict) and col.get("name") is not None: - _describe_column(col, lines) + if isinstance(cols, list): + valid_cols = [c for c in cols if isinstance(c, dict) and c.get("name") is not None] + if valid_cols: + lines.append(" Columns:") + for col in valid_cols: + _describe_column(col, lines)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/wren/src/wren/memory/schema_indexer.py` around lines 99 - 104, Update the column-rendering logic around _describe_column to filter cols to entries that are dictionaries with a non-None "name" before appending the " Columns:" header. Only emit the header and iterate when at least one valid column remains, while preserving the existing handling of malformed entries.core/wren/tests/unit/test_schema_describe_nonduct.py (1)
16-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExpand test assertions to verify columns and relationships.
The test input includes a malformed column (
"bad") and a valid relationship ("r1"), but there are no assertions to ensure they are handled correctly. Adding these checks will strengthen the test coverage for the new guards.🧪 Proposed test enhancements
assert "orders" in text assert "not-a-model" not in text assert "v1" in text + assert "r1" in text + assert "bad" not in text🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/wren/tests/unit/test_schema_describe_nonduct.py` around lines 16 - 18, Expand the assertions in the schema description test to verify that the malformed column “bad” is excluded and the valid relationship “r1” is included in the generated text. Keep the existing model and version assertions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/wren/src/wren/memory/schema_indexer.py`:
- Around line 55-69: Update the model, relationship, and view iteration in the
manifest description flow to call _describe_model, _describe_relationship, and
_describe_view only when each dictionary’s "name" value is not None, matching
the existing column guard and preventing nameless entries from reaching the
helpers.
---
Nitpick comments:
In `@core/wren/src/wren/memory/schema_indexer.py`:
- Around line 99-104: Update the column-rendering logic around _describe_column
to filter cols to entries that are dictionaries with a non-None "name" before
appending the " Columns:" header. Only emit the header and iterate when at
least one valid column remains, while preserving the existing handling of
malformed entries.
In `@core/wren/tests/unit/test_schema_describe_nonduct.py`:
- Around line 16-18: Expand the assertions in the schema description test to
verify that the malformed column “bad” is excluded and the valid relationship
“r1” is included in the generated text. Keep the existing model and version
assertions unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: af5da57e-ff52-457d-be07-9ea68d0d7a19
📒 Files selected for processing (2)
core/wren/src/wren/memory/schema_indexer.pycore/wren/tests/unit/test_schema_describe_nonduct.py
…hema
Add .get("name") is not None guards so a malformed manifest entry
(e.g. {}) no longer raises KeyError in _describe_model/_relationship/_view.
Addresses CodeRabbit review feedback on Canner#2540.
|
Closing in favor of #2586, which supersedes this. #2586 covers the same Review effort is better spent on #2586; nothing here is lost. |
|
Makes sense — #2586 consolidates these guards through one |
2 participants
Summary
describe_schemaskips non-dict models/relationships/views and non-dict columns.License
Apache-2.0 (
core/wren/...).Verification
cd core/wren && .venv/bin/python -m pytest tests/unit/test_schema_describe_nonduct.py -qTest plan
Summary by CodeRabbit
Bug Fixes
Tests