Repository navigation
Conversation
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| if column.get("column_name") == mapped_column: | ||
| return column.get("partition_value_transform") | ||
| if column.get("column_name") == column_name: | ||
| return column.get(attribute) |
There was a problem hiding this comment.
Suggestion: A partial column entry without expression returns None here, while update_columns preserves the stored expression; a Jinja-templated partition column can therefore escape validation.
Assessment: 🟠 Major · 🔁 Occurrence: Sometimes · 🏷️ Api mismatch
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/commands/dataset/update.py
**Line:** 635:635
**Comment:**
*Api Mismatch: A partial column entry without `expression` returns `None` here, while `update_columns` preserves the stored expression; a Jinja-templated partition column can therefore escape validation.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
There was a problem hiding this comment.
Agreed—a PUT whose column entry carries an id but omits expression leaves the stored Jinja expression untouched in _upsert_columns, yet _effective_column_attribute reads None from the payload entry, so the new blocking check is skipped and the save is accepted. The mapping then silently stays inactive at query time instead of the owner seeing the error. Should the payload branch fall back to the persisted column's value when the key is absent?
There was a problem hiding this comment.
Yes. The validator should inspect the value the update will leave in storage: if the payload omits expression, fall back to the existing column’s expression; if it explicitly supplies None, treat that as a clear.
For example, a partial entry that omits expression must still validate the stored {{ ... }} expression and block the save. Since column updates can also identify a column by id, the fallback should locate the stored column by ID when the payload does not provide a matching column_name. This closes the validation gap while preserving intentional expression clears.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of d2617fa.
When a matching payload column has an id but omits the requested attribute, the helper now looks up that stored column and returns its persisted attribute value. Explicitly supplied attributes, including None, still come from the payload.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 1360f15.
When a column entry omits the attribute, the helper now looks up the stored value using the same key as the write path: column_name for overrides and id for existing upserts. Entries treated as new still return None.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
Note
🤖 Claude Opus 5.5 responding on behalf of hughhhh
Fixed: a partial column entry can no longer skip the Jinja check. _effective_column_attribute (superset/commands/dataset/update.py) now reads an omitted attribute from the row the write will actually touch, and an explicit None still counts as a clear.
idwith noexpression(4d0e16b): this falls back to the column stored under thatid, as you suggested.test_a_partial_entry_keeps_the_stored_templated_expressioncovers the block, andtest_an_explicit_null_expression_clears_the_stored_templatecovers the clear.override_columns=true(1360f15): the fallback above still missed this path, because_override_columnsmatches bycolumn_nameand ignoresid. An entry with neitheridnorexpressionkept the stored Jinja and passed validation, and a renamed entry with a staleidwas refused over an expression its new row wouldn't get. Both now follow the name.test_an_override_entry_keeps_the_stored_template_of_its_nameandtest_an_override_entry_ignores_a_stale_idfailed before the fix and pass now.
The value transform goes through the same helper. The PUT schema requires column_name, so the id-only entry CodeAnt mentions can't reach this code. I've left this thread open for you to check.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 7685947.
When a column entry omits the attribute, the helper now looks up the stored value using the same key as the write path: column_name for overrides and id for existing upserts. Entries treated as new still return None.
If that's not right, unresolve this thread and CodeAnt will leave it open.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #45546 +/- ##
=======================================
Coverage 84.93% 84.93%
=======================================
Files 3077 3077
Lines 208205 208247 +42
Branches 49105 49113 +8
=======================================
+ Hits 176834 176881 +47
+ Misses 28002 27998 -4
+ Partials 3369 3368 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
github-actions
Bot
added
the
requires:rebase
|
2 similar comments
|
|
cf34d40 to
d2617fa
Compare
github-actions
Bot
added
i18n
The mirrored predicate is built with `convert_tbl_column_to_sqla_col` outside
the chart query's template processor, so a calculated partition column whose
expression carried Jinja reached the SQL as raw `{{ ... }}`. Preview reported
the mapping valid with the raw template in `emitted_predicate`, and every chart
filtered on the mapped column then failed at the engine.
Refuse it rather than render it: preview has no chart template context to
render with, and a key computed per request is not a partition the engine can
prune on anyway.
- `validate_partition_mapping` takes the partition column's expression and
reports a blocking issue on `partition_column` when it holds Jinja; the REST
update command, the legacy datasource save and preview all pass it.
- `resolve_partition_mapping` declines such a column at query time (imports,
older rows), and the Explore summary reports it inactive.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…mn entry keeps A column entry with an `id` but no `expression` leaves the stored expression in place, because `DatasetDAO._upsert_columns` only sets the keys it is given. `_effective_column_attribute` read `None` from such an entry, so a stored Jinja partition key passed validation and the mapping silently stayed inactive. Fall back to the stored column by id when the key is absent. An explicit `None` is still a clear. The value transform gets the same fallback. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…e catalogs The new message goes into messages.pot and into every .po catalog as an untranslated entry. Nothing else in the catalogs changes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ite hits `_effective_column_attribute` assumed `_upsert_columns`' matching, by `id`. With `override_columns`, `_override_columns` matches by name and ignores the `id`, so an entry with neither `id` nor `expression` kept the stored Jinja while validation saw nothing, and a renamed entry with a stale `id` was refused over an expression its new row would not get. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
d2617fa to
1360f15
Compare
| command._properties = { | ||
| "columns": [ | ||
| {"id": 1, "column_name": "event_time"}, | ||
| {"id": 2, "column_name": "dt_epoch", "description": "partition key"}, |
There was a problem hiding this comment.
This only exercises the id-keyed fallback with the name unchanged, so a lookup keyed on column_name instead of id would pass it too. A rename in the same PUT is the case that separates them: stored column id=2 named dt_epoch holds the Jinja, the payload sends {"id": 2, "column_name": "dt_renamed"} with partition_column="dt_renamed" and no expression, and a name-based lookup finds no stored row and returns None, so the templated column is saved. Could you add a case like that asserting a partition_column error mentioning Jinja?
There was a problem hiding this comment.
Note
🤖 Claude Opus 5.5 responding on behalf of hughhhh
Added in 7685947. test_a_renamed_partial_entry_keeps_the_stored_template_of_its_id is your case: stored id=2 named dt_epoch holds the Jinja, the payload sends {"id": 2, "column_name": "dt_renamed"} with partition_column="dt_renamed", and the test asserts a partition_column error mentioning Jinja.
It passes against the current lookup, so _effective_column_attribute didn't need changing. To check that it separates the two, I switched the lookup to column_name, and the test failed while the existing partial-entry test still passed. I've left this thread open for you to check.
github-actions
Bot
removed
the
requires:rebase
…e PUT A name-based lookup finds no stored row for a renamed partial entry, so the stored Jinja would be saved under the new name. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
SUMMARY
Stacked on #43870. Addresses the review note there that calculated partition expressions bypass template processing.
The bug.
build_mirrored_predicatesbuilds the partition column withconvert_tbl_column_to_sqla_col(partition_column)and no template processor. When the partition column is a calculated column whose expression holds Jinja, that expression goes into the predicate unrendered: the query-time validator can't parse{{ ... }}, so it skips the check and returns the text as-is. Reproduced withdt_calc = {{ 'dt_' ~ 'epoch' }}:valid: truewithemitted_predicateset to({{ 'dt_' ~ 'epoch' }}) = 1768435200 OR ({{ 'dt_' ~ 'epoch' }}) IS NULL.(dt_epoch)inSELECTandGROUP BY, but leaves it raw in the mirroredWHERE, so the query fails at the engine.Before this change, only the value transform was checked for Jinja.
The fix: reject it rather than render it. Rendering with the query's template processor would not help:
url_params,filter_values, current user), so it couldn't show what a chart actually emits.Changes:
validate_partition_mappingtakes a newpartition_expressionargument and reports a blocking issue onpartition_columnwhen it contains Jinja./datasource/save/path, and preview all pass the expression in._effective_transformis generalized to_effective_column_attributefor this.resolve_partition_mappingdeclines such a column at query time, to cover imports and rows saved before this check. The Explore summary reports the mapping as inactive.A calculated partition column without Jinja still mirrors as before.
TESTING INSTRUCTIONS
Run the partition-mapping, dataset command, datasource view, model and MCP dataset unit suites. Locally: 2,523 passed, ruff is clean, and mypy reproducing the CI hook (
--check-untyped-defs) is clean on the touched files.New tests cover each layer:
resolve_partition_mapping: templated vs. untemplated partition columnADDITIONAL INFORMATION
PARTITION_FILTER_MAPPING🤖 Generated with Claude Code