fix(osi): skip empty expressions when picking a dialect expression#2413
Conversation
_pick_expression returned an empty string when the preferred dialect's expression entry was present but blank, even though a later ANSI_SQL or other dialect carried a real expression. An empty string carries no SQL, so a calculated OSI field would be emitted as an empty/identity column. Skip blank entries at each resolution tier (preferred -> ANSI_SQL -> first non-empty).
|
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 (2)
Walkthrough
ChangesEmpty expression fallback fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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 |
What
_pick_expression(OSI → MDL importer) resolves a SQL expression for a field/metric in priority order: preferred dialect →ANSI_SQL→ first available. The membership checks usedin by_dialect, so a present-but-empty expression for the preferred dialect (or forANSI_SQL) counted as "found" and was returned verbatim — shadowing a real expression sitting in another dialect entry.This skips blank expressions at every resolution tier and falls through to the first non-empty expression.
Why it's a bug
An empty string carries no SQL. When the preferred dialect entry is blank, the field is genuinely calculated elsewhere in the
dialectslist, but the importer would emit it as an empty/identity column — silently dropping the calculation.Evidence
Before (unpatched) — new tests fail:
After (patched) — whole OSI suite green:
Scope
core/wren/src/wren/osi.py—_pick_expressionskips empty entries per tier (1 change)core/wren/tests/unit/test_osi.py— 2 new cases (empty-preferred→ANSI, empty-preferred+empty-ANSI→first non-empty)Summary by CodeRabbit