Skip to content

feat(tableau): add is_completion_row to ddi_dashboard#3576

Open
anthonygwalters wants to merge 2 commits intomainfrom
claude/feat/ddi-dashboard-is-completion-row
Open

feat(tableau): add is_completion_row to ddi_dashboard#3576
anthonygwalters wants to merge 2 commits intomainfrom
claude/feat/ddi-dashboard-is-completion-row

Conversation

@anthonygwalters
Copy link
Copy Markdown
Member

Adds a boolean flag that is TRUE for the single deduplication row per student-assessment: response_type = 'overall' for completed assessments, or NULL response_type for assigned-but-incomplete assessments.

Filtering on is_completion_row = TRUE instead of response_type = 'overall' exposes incomplete assessments to Tableau, enabling a simple SUM(is_complete) / COUNT(is_complete) completion rate that responds naturally to any dimension filter (subgroup, school, grade, etc.) without requiring LOD expressions.

Pull Request

Summary & Motivation

"When merged, this pull request will..."

AI Assistance

If Claude Code authored or co-authored this PR, briefly note what was
AI-assisted vs. human-directed in the summary above

Self-review

Complete only the sections relevant to your changes.

General

If this is a same-day request, please flag that in the #data-team Slack

  • Update due date and assignee on the
    TEAMster Asana Project
  • Review the Claude Code Review comment posted on this PR. Address valid
    feedback; dismiss false positives with a brief reply explaining why.
    (Claude is advisory — use your judgement, but don't ignore it.)

Dagster (skip if no Dagster changes)

  • Run uv run dagster definitions validate for any modified code location
  • Run uv run pytest tests/test_dagster_definitions.py for any modified
    code location
  • New integrations follow the
    Library + Config pattern
    with the correct asset key format and IO manager

dbt (skip if no dbt changes)

  • Include a [model_name].yml properties file for all new models — see
    dbt Conventions
  • Include (or update) an
    exposure
    for all models consumed by a dashboard, analysis, or application
  • Breaking change? Renaming or removing columns in a contracted model
    (stg_, rpt_, mart) will break downstream exposures. Coordinate with
    affected teams before merging and document your rollback plan in the
    summary above.
  • If adding a new external source, run stage_external_sources before
    building — see
    SQL conventions

Docs (skip if no schedule, sensor, or integration changes)

  • If adding or changing a schedule or sensor, regenerate the automations
    catalog: uv run scripts/gen-automations-doc.py
  • If adding a new integration to a code location, update that location's
    CLAUDE.md by running /claude-md-management:revise-claude-md

CI checks

  • Trunk — passes. If it fails, run trunk check and trunk fmt
    locally, fix any issues, and push again.
  • dbt Cloud — passes. If it fails: click Details on the check →
    expand Invoke dbt build ...Debug Logs. Or tag Claude on
    the PR with the error details.
  • Dagster Cloud — passes or not triggered (only runs when relevant paths
    change and the PR is not a draft). If it fails: check the Actions tab
    for the workflow run logs. Re-run failed jobs if the failure looks
    transient; otherwise check the Dagster Cloud branch deployment for
    error details. Or tag Claude on the PR for help.

Troubleshooting

Adds a boolean flag that is TRUE for the single deduplication row per
student-assessment: response_type = 'overall' for completed assessments,
or NULL response_type for assigned-but-incomplete assessments.

Filtering on is_completion_row = TRUE instead of response_type = 'overall'
exposes incomplete assessments to Tableau, enabling a simple
SUM(is_complete) / COUNT(is_complete) completion rate that responds
naturally to any dimension filter (subgroup, school, grade, etc.) without
requiring LOD expressions.
@anthonygwalters anthonygwalters requested review from a team as code owners April 2, 2026 18:50
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 2, 2026

Claude finished @anthonygwalters's task in 1m 44s —— View job


Code Review

  • Gather context
  • Read changed files
  • Review dbt SQL and YAML changes
  • Post review feedback

Summary

This is a clean, additive change. The logic is sound and the documentation is well-written. A few observations below.


SQL — rpt_tableau__ddi_dashboard.sql

Logic — correct ✓

if(
    co.response_type = 'overall' or co.response_type is null, true, false
) as is_completion_row,

The two-branch condition correctly maps the stated semantics:

  • 'overall' → completed assessment deduplication row
  • NULL → assigned-but-incomplete assessment (no response rows exist, so the join produces NULL)

UNION ALL consistency — correct ✓

is_completion_row appears in all three branches of the UNION ALL:

  • ES/MS block: computed (line 273)
  • HS block: computed (line 381)
  • Walkthrough block: null (line 512)

Using null (not false) for walkthroughs is consistent with the existing convention for boolean fields that don't apply to walkthroughs (is_complete, is_qbl, etc.), and it correctly excludes walkthrough rows from WHERE is_completion_row = TRUE filtering.

Minor: is_complete vs is_completion_row type inconsistency (pre-existing)

is_complete returns INT64 (if(..., 1, 0)) while is_completion_row returns BOOLEAN (if(..., true, false)). This is a pre-existing pattern — not introduced here — but worth noting for Tableau consumers who might expect consistency.


YAML — rpt_tableau__ddi_dashboard.yml

Column entry — correct ✓

- name: is_completion_row
  data_type: boolean
  description: >-
    True for the single deduplication row per student-assessment: ...

data_type: boolean matches the IF() return type in BigQuery. The description is accurate and clearly explains the filter pattern for Tableau consumers.

Convention gap: missing description: on model and existing columns

Per src/dbt/CLAUDE.md: "All new or modified models require description: on the model and every column." This model is being modified, so a model-level description: and descriptions for all existing columns are technically required. This is a pre-existing gap across ~70 columns — not introduced by this PR — so I'm flagging it as an observation rather than a blocker. If there's a convention to handle this incrementally, that would apply here.


Exposure — no changes needed ✓

The ddi_suite exposure in tableau.yml already references ref("rpt_tableau__ddi_dashboard") and doesn't enumerate specific columns, so no update is needed.


Assessment

Ready to merge. The implementation is correct, consistent, and appropriately documented. The convention gap on column descriptions is pre-existing and doesn't affect correctness or the Tableau use case.

@anthonygwalters anthonygwalters marked this pull request as draft April 3, 2026 15:42
@anthonygwalters anthonygwalters marked this pull request as ready for review April 3, 2026 15:42
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 3, 2026

Claude finished @anthonygwalters's task in 3m 0s —— View job


Code Review

  • Gather context
  • Read changed files
  • Review dbt SQL and YAML changes
  • Post review feedback

Summary

Clean, purposeful change. The logic is correct and the description is excellent. One fix requested before merge.


SQL — rpt_tableau__ddi_dashboard.sql

Logic — correct ✓

if(
    co.response_type = 'overall' or co.response_type is null, true, false
) as is_completion_row,

Both conditions are semantically correct: 'overall' is the right deduplication row for completed assessments; NULL response_type correctly identifies assigned-but-incomplete assessments (no response rows exist, so the join produces NULL).

UNION ALL consistency — correct ✓

is_completion_row appears consistently in all three branches and is positioned immediately after is_qbl in both the ES/MS (line 273) and HS (line 381) blocks. The walkthrough block correctly uses null as is_completion_row (line 512), consistent with the existing convention for flags not applicable to walkthroughs (is_complete, is_qbl, etc.).


YAML — rpt_tableau__ddi_dashboard.yml — fix requested

Column description — correct ✓

data_type: boolean matches the IF() return type. The description is accurate and written from the consumer's perspective — exactly what it should be.

YAML column order does not match SQL SELECT order

is_completion_row is appended at the end of the YAML column list (after is_low_25_fl), but in the SQL SELECT it appears between is_qbl and is_passed_iready_2plus_reading_int. BigQuery contract enforcement matches columns by name, not position, so this won't cause a runtime failure — but the YAML should reflect SQL ordering for maintainability.

Please move the is_completion_row entry in the YAML to appear between is_qbl and is_passed_iready_2plus_reading_int:

      - name: is_qbl
        data_type: boolean
      - name: is_completion_row          # ← move here
        data_type: boolean
        description: >-
          True for the single deduplication row per student-assessment: the
          response_type = 'overall' row for completed assessments, or the null
          response_type row for assigned-but-incomplete assessments. Filter on
          this field instead of response_type = 'overall' when computing
          completion rates so that incomplete assessments are included in the
          denominator.
      - name: is_passed_iready_2plus_reading_int
        data_type: int64

Note: There is a broader pre-existing YAML/SQL ordering mismatch in this file (is_foundations, is_self_contained, year_in_network, is_sipps, is_low_25_fl are also out of order relative to the SQL) — that's out of scope for this PR, but this PR shouldn't deepen it by appending at the end.


Exposure — no changes needed ✓

The ddi_suite exposure already references ref("rpt_tableau__ddi_dashboard") without enumerating specific columns.


Assessment

One fix before merge — move is_completion_row in the YAML to match its SQL SELECT position. Everything else is correct.

@cbini cbini marked this pull request as draft April 3, 2026 17:15
@cbini cbini marked this pull request as ready for review April 3, 2026 17:15
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 3, 2026

Claude finished @cbini's task in 2m 0s —— View job


Code Review

  • Gather context
  • Read changed files
  • Review dbt SQL and YAML changes
  • Post review feedback

Summary

This is the fix requested in the previous review — the change itself is correct. One outstanding item remains before merge.


SQL — correct ✓

is_completion_row logic is sound and UNION ALL placement is consistent across all three branches (ES/MS line 273, HS line 381, walkthrough null line 512).


YAML — fix applied locally, push blocked

The previous review correctly flagged that is_completion_row was appended at the end of the YAML column list, but in the SQL SELECT it appears between is_qbl and is_passed_iready_2plus_reading_int. I applied the fix locally (commit 685147c) but the push was rejected (GitHub Actions bot lacks write permission to this repo).

Please apply this fix manually — move the is_completion_row entry in the YAML from the end to between is_qbl and is_passed_iready_2plus_reading_int:

      - name: is_qbl
        data_type: boolean
      - name: is_completion_row          # ← move here
        data_type: boolean
        description: >-
          True for the single deduplication row per student-assessment: the
          response_type = 'overall' row for completed assessments, or the null
          response_type row for assigned-but-incomplete assessments. Filter on
          this field instead of response_type = 'overall' when computing
          completion rates so that incomplete assessments are included in the
          denominator.
      - name: is_passed_iready_2plus_reading_int
        data_type: int64

And remove it from the end of the file (after is_low_25_fl).


Assessment

One fix before merge — YAML column ordering. Everything else is correct and the implementation logic is solid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants