[fix] st.bar_chart fails on column names containing "."#16089
[fix] st.bar_chart fails on column names containing "."#16089sfc-gh-dbyttow wants to merge 7 commits intodevelopstreamlit/streamlit:developfrom bar-chart-dot-field-7714streamlit/streamlit:bar-chart-dot-field-7714Copy head branch name to clipboard
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
✅ PR preview is ready!
|
|
| Filename | Overview |
|---|---|
| lib/streamlit/elements/lib/built_in_chart_utils.py | Adds collision-safe aliases and carries original column names through chart encodings. |
| lib/tests/streamlit/elements/lib/built_in_chart_utils_test.py | Tests unsafe-name conversion and generated-alias collision handling. |
| lib/tests/streamlit/elements/vega_charts_test.py | Tests rendering, sorting, tooltips, legends, and duplicate special-character labels. |
| e2e_playwright/st_bar_chart.py | Adds bar chart examples with dotted and bracketed column names. |
| e2e_playwright/st_bar_chart_test.py | Adds visual checks for the new special-character column cases. |
Reviews (8): Last reviewed commit: "Address AI review nits on _remap_y" | Re-trigger Greptile
SummaryFixes blank charts when DataFrame column names contain characters that Vega-Lite treats as field syntax ( Both reviewers (claude-4.6-opus-high-thinking, gpt-5.3-codex-high) approved the PR. No expected models failed to complete their review. Code QualityConsensus: Both reviewers agree the implementation is well-structured and centralized. The aliasing logic is introduced at data-prep time in Minor observations (non-blocking, both reviewers):
Test CoverageConsensus: Both reviewers rate coverage as strong. The PR includes:
Minor gap (noted by gpt-5.3-codex-high only): No explicit high-level API tests for Backwards CompatibilityFull agreement: No breaking changes. All modified functions are module-private ( Security & RiskFull agreement: No security concerns. The change is purely in the data-preparation layer for built-in charts with no network, auth, file handling, or injection paths involved. Regression risk is low-to-medium, mitigated by the conservative approach (only columns containing special characters are renamed) and thorough test coverage. External test recommendation
AccessibilityFull agreement: No frontend component or DOM changes. The fix operates entirely in the Python backend chart-generation layer. Axis labels, tooltips, and legend text continue to display the original user-facing column names, so there is no regression in accessible labeling. ReadabilityChanged code readability:
PR metadata readability:
Recommendations
VerdictAPPROVED: Both reviewers approve. This is a clean, well-tested bug fix for a real user-facing issue (#7714). The approach is conservative and backwards-compatible, with thorough test coverage and no security or regression concerns. All suggestions above are non-blocking improvements suitable for follow-up PRs. Consolidated review by This review also includes 3 inline comment(s) on specific code lines. Inline comments (3) that could not be posted on specific lines
|
There was a problem hiding this comment.
Summary
Fixes #7714: built-in chart commands (st.bar_chart, st.line_chart, st.area_chart, st.scatter_chart) rendered blank when a DataFrame column name contained characters that Vega-Lite treats as special in field strings (., [, ], \). Vega-Lite interprets . as nested-object access, so a column named col.name was parsed as row["col"]["name"], yielding no data.
The fix renames such columns to safe internal aliases in _convert_col_names_to_str_in_place, threads an alias_to_original map through all encoding helpers (axis, color, size, tooltip, sort), and rewrites melted-color values back to original names so that axis titles, legends, tooltips, and sort references still display the user-facing column names.
Reviewer consensus: Both reviewers (claude-4.6-opus-high-thinking and gpt-5.3-codex-high) approved unanimously. No disagreements on correctness, risk, or test coverage.
Code Quality
The implementation is clean and well-structured. Both reviewers agreed on the following:
- Alias generation and collision avoidance (
_convert_col_names_to_str_in_place): Correctly builds areserved_namesset from plain columns, uses index-based alias names with a counter fallback for the rare collision case. The_remaphelper keeps column-argument remapping concise. - Alias map threading: The
alias_to_originaldict is consistently passed to all encoding functions (_get_axis_encodings,_get_color_encoding,_get_size_encoding,_get_tooltip_encoding,_update_encoding_with_sort), each of which correctly uses it for display titles while referencing the alias for field lookups. - Melted-color rewrite in
_prep_data(lines 458–468): Smart approach — rewriting the melted color column data values back to original names avoids needing a Vega-LitelabelExprremap, keeping both legend and tooltip correct. - Comments explain the "why" (Vega-Lite special characters, #7714) rather than narrating the "what". The
r"""raw docstring is correctly used for the backslash in the function docstring. - The 8-element
_prep_datareturn tuple is getting large. ANamedTupleordataclasswould improve readability, but this follows the pre-existing pattern and is a style concern for a follow-up, not a blocker.
Test Coverage
Both reviewers confirmed that test coverage is thorough and well-targeted:
Unit tests (built_in_chart_utils_test.py):
- Aliasing for
.and[]characters, with alias map round-trip verification. - Collision avoidance when a plain column literally matches the default alias form.
- Existing
_get_color_encodingand_get_size_encodingtests updated to pass the newalias_to_original={}parameter.
Integration tests (vega_charts_test.py):
- Single-column with
.and[]in name render correctly. - Tooltip and axis title show original column name for aliased columns.
- Multi-column legend and tooltip display original names (not aliases) for melted data.
- Anti-regression: plain column names are never aliased.
- Sort by a dotted column uses the alias field while preserving sort order.
E2E tests (st_bar_chart_test.py):
- Two new snapshot tests (dotted and bracketed column names) with proper
TOTAL_BAR_CHARTScount update.
One reviewer noted a minor gap: E2E additions are bar-chart-only, while the shared alias plumbing also affects st.line_chart, st.area_chart, and st.scatter_chart. The integration-level unit tests do cover the shared code paths, so this is not blocking, but a targeted non-bar E2E test would strengthen confidence.
Backwards Compatibility
No breaking changes. Both reviewers agreed:
- No public API changes — the fix is entirely internal to chart preparation.
- Columns without special characters are unaffected (verified by anti-regression test).
- Axis titles, tooltips, legends, and sort all still display the original user-facing column names.
st.altair_chartandst.vega_lite_chartpassthrough paths are untouched.
Security & Risk
No security concerns. Both reviewers agreed:
- No new dependencies, network changes, auth changes, or runtime JavaScript execution.
- Change is scoped to internal DataFrame column renaming in chart preparation.
- The alias naming pattern (
col -- streamlit-generated-<idx>) uses the existing_PROTECTION_SUFFIXconvention, minimizing collision risk with user data.
Risk level: Low. The alias map flows through many encoding paths, but each path is well-tested and the logic is straightforward.
External test recommendation
- Recommend external_test: No
- Triggered categories: None
- Evidence:
- All changes are in
lib/streamlit/elements/lib/built_in_chart_utils.py— internal chart data preparation, no routing, auth, transport, storage, or cross-origin policy logic touched. - E2E tests added use local data with no external dependencies.
- All changes are in
- Confidence: High
- Assumptions and gaps: None — pure data-preparation logic with no integration surface.
Both reviewers independently reached the same conclusion.
Accessibility
No frontend component or interaction model changes were made. The fix is entirely backend (Python chart-spec generation). Axis labels, legends, and tooltips continue to display the original user-facing column names, so there is no accessibility regression.
Readability
Overall readability is very good. Both reviewers noted strong commenting and naming practices.
lib/streamlit/elements/lib/built_in_chart_utils.py:
- Constants
_COLUMN_ALIAS_PREFIXand_VEGA_LITE_FIELD_SPECIAL_CHARSare well-named and well-commented. _needs_field_alias— name and docstring are clear and concise.- Updated
_prep_datadocstring correctly describes the new return values. - Comment on the melted-color rewrite leads with intent before explaining mechanism.
- Comment on sort aliasing includes a reference to the public GitHub issue.
lib/tests/streamlit/elements/vega_charts_test.py:
- Test names are descriptive and communicate their purpose.
- Location:
test_bar_chart_multi_dotted_columns_tooltip_shows_originals, line 2677 - Issue:
any(ch in str(value) for ch in ("streamlit-generated",))iterates a single-item tuple, which reads as more complex than needed. - Proposed rewrite:
assert "streamlit-generated" not in str(value)
PR title: [fix] st.bar_chart fails on column names containing "." — clear, follows the [type] Description format. Could mention other chart types are also fixed, but that would make it too long. Acceptable as-is.
PR description: Well-structured. Leads with the root cause, explains the fix approach, lists affected chart types, and includes a thorough testing plan. No significant improvements needed.
Recommendations
No merge-blocking issues found. Minor suggestions for potential follow-up:
-
Consider a
NamedTuplefor_prep_data's return type: The 8-element tuple is getting unwieldy. APreparedDataNamedTuplewould make call sites more readable and reduce argument-order confusion risk. (Both reviewers noted this.) -
Add a targeted non-bar chart E2E test: A single regression test for
st.line_chart,st.area_chart, orst.scatter_chartwith a dotted/bracketed column name would confirm cross-command behavior end-to-end. The integration unit tests cover the shared code paths, so this is low priority. -
Consider testing backslash column names:
_VEGA_LITE_FIELD_SPECIAL_CHARSincludes\\, but there's no dedicated test for a column name like"a\\b". The logic handles it correctly via_needs_field_alias, but an explicit test case would complete the coverage matrix. Low priority.
Verdict
APPROVED: Well-implemented bug fix with thorough test coverage, no breaking changes, and clean code. Both reviewers approved unanimously — the alias-map approach is sound and correctly handles single-column, multi-column, tooltip, legend, sort, and collision edge cases.
Consolidated from reviews by gpt-5.3-codex-high and claude-4.6-opus-high-thinking. This is an automated AI review by claude-4.6-opus-high-thinking. Please verify the feedback and use your judgment.
This review also includes 2 inline comment(s) on specific code lines.
| data_frame = convert_arrow_bytes_to_pandas_df(proto.datasets[0].data.data) | ||
| color_field = chart_spec["encoding"]["color"]["field"] | ||
| for value in data_frame[color_field].unique(): | ||
| assert not any(ch in str(value) for ch in ("streamlit-generated",)) |
There was a problem hiding this comment.
suggestion: any(ch in str(value) for ch in ("streamlit-generated",)) iterates a single-item tuple, which reads as more complex than needed. Simplify to assert "streamlit-generated" not in str(value).
| str | None, | ||
| str | None, | ||
| dict[str, str], | ||
| ]: |
There was a problem hiding this comment.
thought: The 8-element return tuple (DataFrame, str|None, str|None, list[str], str|None, str|None, str|None, dict[str, str]) is getting unwieldy. A NamedTuple (e.g. PreparedData) would make call sites more readable (result.alias_to_original vs positional unpacking) and reduce argument-order confusion risk. Not blocking — fine as a follow-up.
There was a problem hiding this comment.
Agreed the return tuple is unwieldy — leaving as a follow-up so the fix here stays scoped to the field-syntax bug. Filing a note for a later refactor.
|
Note on the failing |
## Describe your changes Automated snapshot updates for #16089 created via the snapshot autofix CI workflow. This workflow was triggered by adding the `update-snapshots` label to a PR after Playwright E2E tests failed with snapshot mismatches. **Updated snapshots:** 6 file(s)⚠️ **Please review the snapshot changes carefully** - they could mask visual bugs if accepted blindly. This PR targets a feature branch and can be merged without review approval. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Snapshot-only PRs can hide unintended visual bugs if baselines are accepted without review; risk is moderate because six images changed but no runtime code did. > > **Overview** > **Automated Playwright E2E baseline refresh** for the feature work in #16089, produced by the `snapshot-autofix` workflow after the `update-snapshots` label was applied when visual snapshot tests failed in CI. > > This PR **replaces six existing screenshot snapshots** with versions taken from the latest failed Playwright run so E2E visual checks align with the new UI from the parent branch. There is **no application or test logic change** here—only committed image assets under the E2E snapshot directories. > > Reviewers should **compare the image diffs** to confirm the pixel changes match intentional UI updates from #16089 and are not accidental regressions. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit ef707c4. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> Co-authored-by: Streamlit Bot <core+streamlitbot-github@streamlit.io>
| for idx, name in enumerate(str_column_names): | ||
| if _needs_field_alias(name) and name not in original_to_alias: | ||
| # Pick an alias that does not collide with any plain user column or a | ||
| # previously generated alias. The index disambiguates duplicates; a | ||
| # trailing counter is only needed on the extremely rare occasion that | ||
| # a user column literally matches the default form. | ||
| alias = f"{_COLUMN_ALIAS_PREFIX}{idx}" | ||
| counter = 0 | ||
| while alias in reserved_names or alias in alias_to_original: | ||
| counter += 1 | ||
| alias = f"{_COLUMN_ALIAS_PREFIX}{idx}-{counter}" | ||
| original_to_alias[name] = alias | ||
| alias_to_original[alias] = name | ||
| final_column_names.append(alias) | ||
| else: | ||
| final_column_names.append(original_to_alias.get(name, name)) |
There was a problem hiding this comment.
Critical bug: Duplicate column names after stringification will map to the same alias, creating duplicate columns in the DataFrame.
If a DataFrame has columns that stringify to the same name and both contain special characters (e.g., tuple columns ('a.b', 0) and ('a.b', 1) both become "a.b"), the current logic will:
- Create an alias for the first occurrence:
original_to_alias["a.b"] = "col--streamlit-generated-0" - For the second occurrence, skip alias creation (line 692 check fails because
"a.b" in original_to_alias) and reuse the same alias (line 706) - Result: Both columns renamed to the same alias, creating duplicate column names
This breaks pandas operations and Vega-Lite field selection.
# Fix: Track idx separately for each unique name needing an alias
for idx, name in enumerate(str_column_names):
if _needs_field_alias(name):
if name not in original_to_alias:
# Create new alias for this unique name
alias = f"{_COLUMN_ALIAS_PREFIX}{idx}"
counter = 0
while alias in reserved_names or alias in alias_to_original:
counter += 1
alias = f"{_COLUMN_ALIAS_PREFIX}{idx}-{counter}"
original_to_alias[name] = alias
alias_to_original[alias] = name
# Use the alias (whether just created or existing for this name)
final_column_names.append(original_to_alias[name])
else:
final_column_names.append(name)Alternatively, use a counter per position rather than per unique name to ensure each column gets a unique alias even if names collide after stringification.
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
Good catch — fixed in 43dcb8d. Every column that needs aliasing now gets a fresh alias keyed on its column index, and the original→alias map uses setdefault so it only remembers the first-occurrence alias for user-argument remapping. Added a regression test (test_bar_chart_columns_with_colliding_stringified_names_get_distinct_aliases) covering duplicate literal "a.b" column labels.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 43dcb8d. Configure here.
There was a problem hiding this comment.
Summary
This PR fixes #7714 where st.bar_chart (and other built-in chart commands: st.line_chart, st.area_chart, st.scatter_chart) rendered blank charts when a DataFrame column name contained characters that Vega-Lite treats as special in field strings (., [, ], \). Vega-Lite interprets col.name as nested access (row["col"]["name"]) rather than a literal field name.
The fix introduces an aliasing layer in _convert_col_names_to_str_in_place that renames problematic columns to safe internal aliases (col -- streamlit-generated-<idx>) while threading an alias_to_original map through all encoding helpers so that user-facing labels (axis titles, legends, tooltips, sort, size, color) still display the original column names.
Code Quality
Both reviewers agree the implementation is clean and well-structured:
- Follows existing patterns: The aliasing approach is consistent with the existing
_PROTECTION_SUFFIXnaming convention (_SEPARATED_INDEX_COLUMN_NAME,_MELTED_Y_COLUMN_NAME, etc.). - Collision avoidance is robust: The
while alias in reserved_names or alias in alias_to_originalloop with a counter ensures generated aliases never shadow user columns. - Melted-color rewrite is correct: The code rewrites the melted-color column data values back to original names, fixing both legend and tooltip display.
- Module conventions followed: All new module-level symbols are prefixed with
_, type annotations are present on all new/modified functions.
One reviewer raised a concern about setdefault at line 707 potentially collapsing duplicate column names. After verification, this is not a blocking issue: the DataFrame itself correctly receives distinct aliases (keyed on column index), as demonstrated by test_bar_chart_columns_with_colliding_stringified_names_get_distinct_aliases. The setdefault only affects spec-level _remap lookups, and duplicate column names are inherently ambiguous regardless of this PR's changes. See inline comment for details.
Test Coverage
Both reviewers agree test coverage is thorough:
Unit tests (built_in_chart_utils_test.py):
- Aliasing for
.and[]characters, with plain columns left untouched. - Collision avoidance when a user column matches the alias pattern.
- Existing tests updated to pass the new
alias_to_originalparameter.
Integration tests (vega_charts_test.py, 8 new tests):
- Single-column and multi-column dotted/bracketed names render correctly.
- Tooltip and axis title show original column names.
- Legend shows originals, no
labelExprleakage, melted-color tooltip values use original names. - Sort by dotted column uses the alias in the sort field.
- Duplicate column labels get distinct aliases.
E2E tests (st_bar_chart_test.py):
- Two new snapshot tests (dotted and bracketed column names).
TOTAL_BAR_CHARTSupdated from 28 to 30.
One reviewer noted the absence of explicit regression tests for st.line_chart, st.area_chart, and st.scatter_chart. Since these share the same _convert_col_names_to_str_in_place utility path that is thoroughly tested at the unit level, this is a nice-to-have rather than a requirement.
Backwards Compatibility
Both reviewers agree — no breaking changes:
- No public API changes: Function signatures are unchanged.
- Plain columns unaffected:
_needs_field_aliasreturnsFalsefor column names without special characters, bypassing the aliasing path entirely. An explicit test verifies this. - Existing chart behavior preserved: Legends, tooltips, and sort all display user-facing names, matching pre-existing behavior.
Security & Risk
Both reviewers agree — no security concerns:
- The change is purely in backend chart-preparation logic (DataFrame column renaming and Vega-Lite spec generation).
- No network, authentication, file serving, CORS, CSP, session handling, or dependency changes.
- No HTML/Markdown rendering,
eval, or injection vectors.
Regression risk is low-to-medium: the aliasing logic touches shared chart-prep paths for all built-in Altair charts, but is scoped to columns with special characters. The comprehensive test suite mitigates this risk well.
External test recommendation
- Recommend external_test: No
- Triggered categories: None
- Evidence: All changes are in backend chart-preparation logic (
built_in_chart_utils.py) and corresponding tests. No routing, auth, WebSocket, embedding, asset serving, cross-origin, storage, or security header changes. - Confidence: High
- Assumptions and gaps: None — this is a pure data-processing fix with no external-facing behavior changes.
Accessibility
No frontend changes are included in this PR. The fix is entirely in Python backend chart-preparation code. Vega-Lite's accessibility features (ARIA roles, keyboard navigation) are unaffected since only field names and data values in the generated spec change, not the chart structure or rendering behavior.
Readability
lib/streamlit/elements/lib/built_in_chart_utils.py
-
Lines 1136–1139 (comment in
_get_color_encoding, color-value-list path): The 5-line comment leads with internal mechanics. Proposed rewrite:# ``y_column_list`` and the melted color data already hold user-facing # names, so the scale domain and legend align without a ``labelExpr``.
-
Lines 1159–1162 (comment in
_get_color_encoding, melted-color path): Similar verbosity. Proposed rewrite:# The melted color values are already the original user-facing names # (rewritten in ``_prep_data``), so legend labels are correct. See #7714.
lib/tests/streamlit/elements/vega_charts_test.py
- Line ~2663 (
test_bar_chart_multi_dotted_columns_tooltip_shows_originalsdocstring): Contains process commentary ("Reported by Cursor Bugbot on #16089") that doesn't aid comprehension. See inline comment for proposed trim.
e2e_playwright/st_bar_chart_test.py
-
Lines 86–90 (comment before snapshot assertions): Could be more concise. Proposed rewrite:
# Regression tests for #7714: column names with Vega-Lite special chars # ('.', '[', ']') used to produce blank charts. The count assertion above # already verifies rendering; snapshots catch visual regressions.
PR Title & Description
- Title is accurate but narrower than implementation scope (also covers
[,],\and applies to all built-in charts). Acceptable for a bug-fix PR focused on the reported symptom. - Description is well-structured with clear root cause, fix, and testing plan.
Recommendations
- Trim verbose comments (lines 1136–1139, 1159–1162 in
built_in_chart_utils.py; lines 86–90 inst_bar_chart_test.py): The proposed rewrites above improve scan-ability without losing meaning. - Trim process commentary from the
test_bar_chart_multi_dotted_columns_tooltip_shows_originalsdocstring. - Consider a
NamedTuplefor_prep_data's return in a follow-up: The 8-element tuple is getting large. ANamedTuplelikePreparedDatawould make call sites more readable. Out of scope for this bug fix. - Consider adding a focused regression test for
st.line_chart(or another non-bar chart) in a follow-up PR to confirm the shared path works end-to-end for those commands too.
Verdict
APPROVED: A well-implemented, thoroughly tested bug fix for a real user-facing issue. No bugs, security concerns, or breaking changes. The aliasing approach is robust with proper collision avoidance, and the test coverage (unit, integration, e2e) is comprehensive. The setdefault concern raised by one reviewer is not a blocking issue — duplicate column names are inherently ambiguous and the PR handles them gracefully. Minor readability suggestions are non-blocking and can be addressed in follow-up.
Reviewer Agreement
- claude-4.6-opus-high-thinking: APPROVED — no blocking issues.
- gpt-5.3-codex-high: CHANGES REQUESTED — cited a
setdefaultconcern as blocking.
Resolution: After verification, the setdefault behavior is intentional and correct for the supported use cases. The PR explicitly tests duplicate-column handling (test_bar_chart_columns_with_colliding_stringified_names_get_distinct_aliases) and confirms distinct aliases are generated at the DataFrame level. The spec-level ambiguity for duplicate column names is a pre-existing limitation, not a regression. The concern is valid as a future improvement note but does not warrant blocking this PR.
This is a consolidated AI review by claude-4.6-opus-high-thinking.
This review also includes 3 inline comment(s) on specific code lines.
| counter += 1 | ||
| alias = f"{_COLUMN_ALIAS_PREFIX}{idx}-{counter}" | ||
| alias_to_original[alias] = name | ||
| original_to_alias.setdefault(name, alias) |
There was a problem hiding this comment.
thought: setdefault means that when two columns stringify to the same name (e.g. duplicate pandas labels like ["a.b", "a.b"]), the _remap function will map both occurrences to the first alias. The DataFrame itself correctly gets distinct aliases (keyed on index), but spec-level references (y_column_list, sort) will point to only the first. This is acceptable for a bug-fix PR since duplicate column names are inherently ambiguous — but worth noting for a future improvement.
There was a problem hiding this comment.
Fixed in 856bda2 — _remap now consumes aliases from a per-name FIFO queue so duplicate y entries get distinct aliases.
| """Regression test: the melted-color tooltip must display the original | ||
| column names, not the internal aliases. Reported by Cursor Bugbot on | ||
| #16089 — the earlier fix only remapped the legend via ``labelExpr``. | ||
| """ |
There was a problem hiding this comment.
nitpick: The "Reported by Cursor Bugbot on #16089" clause is process commentary that won't help a future reader understand the test. Consider trimming to:
"""The melted-color tooltip must display the original column names,
not the internal aliases.
"""There was a problem hiding this comment.
Trimmed the process commentary in 856bda2.
| # contained ".", "[", "]", or "\"), use the alias here so Vega-Lite finds | ||
| # the actual field. See #7714. | ||
| original_to_alias = {v: k for k, v in alias_to_original.items()} | ||
| sort_field = original_to_alias.get(sort_field, sort_field) |
There was a problem hiding this comment.
thought: This builds a reverse map on every call. Since _update_encoding_with_sort is called at most once per chart the cost is negligible, but if you ever refactor _prep_data to return a NamedTuple you could precompute original_to_alias there and pass it through directly.
There was a problem hiding this comment.
Agreed — _update_encoding_with_sort runs at most once per chart so the linear scan is fine. Switched the inline inversion in 856bda2 to a first-alias-wins scan for consistency with _remap; a precomputed map can come with the NamedTuple refactor already noted above.
## Describe your changes Automated snapshot updates for #16089 created via the snapshot autofix CI workflow. This workflow was triggered by adding the `update-snapshots` label to a PR after Playwright E2E tests failed with snapshot mismatches. **Updated snapshots:** 6 file(s)⚠️ **Please review the snapshot changes carefully** - they could mask visual bugs if accepted blindly. This PR targets a feature branch and can be merged without review approval. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Snapshot-only PRs can hide unintended visual bugs if baselines are accepted without review; risk is moderate because six images changed but no runtime code did. > > **Overview** > **Automated Playwright E2E baseline refresh** for the feature work in #16089, produced by the `snapshot-autofix` workflow after the `update-snapshots` label was applied when visual snapshot tests failed in CI. > > This PR **replaces six existing screenshot snapshots** with versions taken from the latest failed Playwright run so E2E visual checks align with the new UI from the parent branch. There is **no application or test logic change** here—only committed image assets under the E2E snapshot directories. > > Reviewers should **compare the image diffs** to confirm the pixel changes match intentional UI updates from #16089 and are not accidental regressions. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit ef707c4. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> Co-authored-by: Streamlit Bot <core+streamlitbot-github@streamlit.io>
43dcb8d to
856bda2
Compare
There was a problem hiding this comment.
Summary
This PR fixes #7714 where st.bar_chart (and other built-in charts: st.line_chart, st.area_chart, st.scatter_chart) rendered blank when a DataFrame column name contained characters that Vega-Lite treats as special in field strings (., [, ], \).
The fix introduces an internal aliasing layer in _convert_col_names_to_str_in_place that renames problematic columns to safe aliases (e.g. col -- streamlit-generated-0) and returns an alias_to_original mapping. This mapping is threaded through all encoding helpers (x, y, color, size, tooltip, sort) so that axis titles, legends, tooltips, and sort references still display the original user-facing column names.
Reviewer consensus: Both reviewers (claude-4.6-opus-high-thinking, gpt-5.3-codex-high) agreed on the core approach, test quality, backwards compatibility, and absence of security concerns. They diverged on one correctness edge case (discussed below).
Code Quality
The implementation is clean and well-structured. Both reviewers praised:
- Per-column aliasing keyed on index prevents collisions when multiple columns stringify to the same name.
- Collision avoidance with a counter handles the rare case where a user column literally matches the alias form.
- Position-aware y remapping via
per_name_aliasesFIFO queues correctly handles DataFrames with duplicate column labels. - Melted-color rewrite converts alias values back to originals in the data itself, preserving legend/tooltip display.
- The
alias_to_originalparameter is consistently threaded through all affected private functions with appropriate type annotations.
Disagreement resolved: GPT-5.3-codex-high raised a "merge-blocking" correctness issue where _remap_y could return the raw (now-renamed) column name if y_column_list contains more occurrences of an aliased name than entries in per_name_aliases. After verification, this scenario requires the user to explicitly pass the same column name multiple times (e.g. y=['a.b', 'a.b']) for a DataFrame with only one such column. This is nonsensical input that was already broken pre-PR (pandas melt with duplicate value_vars is undefined). The legitimate case — duplicate column labels in the DataFrame itself — is handled correctly because the FIFO queue has one entry per actual column. This is therefore not a regression and not merge-blocking.
Test Coverage
Excellent coverage across multiple layers (both reviewers agreed):
- Unit tests (
built_in_chart_utils_test.py): Aliasing for special characters, collision avoidance, updated existing tests for newalias_to_originalparameter. - Integration-level unit tests (
vega_charts_test.py): Single-column dotted/bracketed names, tooltip/title preservation, multi-dotted-column legend, anti-regression for plain columns, sort by dotted column, duplicate column labels. - E2E tests (
st_bar_chart_test.py): Two new snapshot assertions for dotted and bracketed column names, withTOTAL_BAR_CHARTScount updated from 28 to 30.
Backwards Compatibility
No public API changes. Both reviewers confirmed:
- Columns without special characters are never aliased (verified by anti-regression test).
- Aliasing is purely internal — user-facing surfaces (axis titles, legends, tooltips) display original names.
- The
alias_to_originaldict is empty when no aliasing occurs, so existing codepaths pass through unaffected. - Modified function signatures are all private (
_-prefixed), not exposed to users.
Security & Risk
No security concerns (unanimous). No new dependencies, external requests, network operations, HTML/JS rendering changes, or injection risks. The change is entirely within backend chart-spec generation logic using a deterministic, collision-safe naming scheme.
Regression risk is medium: the aliasing layer touches all built-in chart types' data preparation and encoding paths. However, comprehensive test coverage and the "no-op when no special chars" design effectively mitigate this.
External test recommendation
- Recommend external_test: No
- Triggered categories: None
- Evidence: Pure internal chart-spec generation logic; no routing, auth, embedding, asset serving, WebSocket, or cross-origin changes.
- Suggested external_test focus areas: None applicable.
- Confidence: High
- Assumptions and gaps: None. The change is entirely within chart data preparation with no interaction with server, networking, or embedding layers.
Accessibility
No accessibility impact (unanimous). This PR does not modify any frontend components, DOM structure, or ARIA attributes. The change is purely in the Python backend's Vega-Lite spec generation.
Readability
lib/streamlit/elements/lib/built_in_chart_utils.py
-
Line 692-695 (
per_name_aliasescomment): Dense and buries purpose under mechanism.- Proposed:
# FIFO queue of aliases per original column name. When y_column_list has duplicate labels (e.g. two columns both named "a.b"), each entry consumes a distinct alias in column order.
- Proposed:
-
Lines 452-457 (melt rewrite comment): Opens with mechanism rather than intent.
- Proposed:
# Restore original column names in the melted-color data so legends and tooltips show user-facing labels instead of internal aliases.
- Proposed:
lib/tests/streamlit/elements/vega_charts_test.py
- Line 2711: Test name
test_bar_chart_columns_with_colliding_stringified_names_get_distinct_aliasesis overly long (77 chars). Shorter alternative:test_bar_chart_duplicate_dotted_column_names_get_distinct_aliases.
e2e_playwright/st_bar_chart_test.py
- Comment block above new snapshots is correct but verbose; key intent is slightly buried.
- Proposed:
# Regression for #7714: fields containing '.', '[' or ']' must still render.
- Proposed:
PR title/description
- Title is accurate for the reported symptom but narrower than the implemented scope (all built-in chart types share the fix path).
- Proposed title:
[fix] Handle Vega-Lite special characters in built-in chart field names
Recommendations
- Consider the readability rewrites above for comment clarity — these are optional and non-blocking.
- The
_remap_yclosure mutatesper_name_aliasesby popping from lists. While safe in the current single-call context, a brief comment noting this is intentionally destructive would help future readers. - (Optional follow-up) If desired, add a guard in
_remap_ythat falls back tooriginal_to_alias.get(name, name)when the FIFO queue is exhausted, to handle the degenerate case of user-supplied duplicate y values gracefully. This is not a regression and can be addressed separately.
Verdict
APPROVED: Well-implemented bugfix with thorough test coverage, clean backwards compatibility, and no security concerns. The aliasing approach is sound and handles real-world edge cases robustly. The one correctness concern raised (duplicate explicit y values for a single column) is a pre-existing limitation with nonsensical input, not a regression introduced by this PR.
This is a consolidated AI review by claude-4.6-opus-high-thinking, synthesizing reviews from claude-4.6-opus-high-thinking and gpt-5.3-codex-high.
This review also includes 3 inline comment(s) on specific code lines.
| aliases = per_name_aliases.get(name) | ||
| if aliases: | ||
| return aliases.pop(0) | ||
| return name |
There was a problem hiding this comment.
thought: When the FIFO queue in per_name_aliases is exhausted (empty list is falsy), this falls back to returning the raw name. This is safe for the designed use case (duplicate DataFrame column labels produce matching queue lengths), but if a user explicitly passes duplicate y values for a single column (e.g. y=['a.b', 'a.b'] with one a.b column), the fallback would reference the now-renamed column. Consider falling back to original_to_alias.get(name, name) for robustness, or adding a comment noting this is only valid when y_column_list mirrors actual column occurrences.
There was a problem hiding this comment.
Good call — swapped the fallback to original_to_alias.get(name, name). Now if a caller passes more duplicates than the DataFrame actually has (a degenerate case), we still point at an aliased column rather than the pre-rename original. Also expanded the comment to explain the fallback intent.
| name = str(name) | ||
| aliases = per_name_aliases.get(name) | ||
| if aliases: | ||
| return aliases.pop(0) |
There was a problem hiding this comment.
suggestion: aliases.pop(0) is intentionally destructive — it relies on being called exactly once per entry during the list comprehension below. A brief comment like # Intentionally consumes the first alias (single-use per column) would signal to future readers that this side effect is by design.
There was a problem hiding this comment.
Added an inline note explaining that aliases.pop(0) is intentionally destructive — each queue entry is consumed once per DataFrame column occurrence.
| # y remapping so a ``y_column_list`` derived from ``list(df.columns)`` with | ||
| # duplicate labels consumes distinct aliases in order (see #7714 follow-up). | ||
| per_name_aliases: dict[str, list[str]] = {} | ||
| final_column_names: list[str] = [] |
There was a problem hiding this comment.
nitpick: This comment is dense and buries the purpose under the mechanism. Consider leading with intent:
| final_column_names: list[str] = [] | |
| # FIFO queue of aliases per original column name. When y_column_list has | |
| # duplicate labels (e.g. two columns both named "a.b"), each entry consumes | |
| # a distinct alias in column order. |
There was a problem hiding this comment.
Applied — comment now leads with intent: it's a FIFO queue per original column name that hands out one alias per duplicate label in column order. Kept the #7714 reference for future readers grepping the fix.
There was a problem hiding this comment.
Summary
This PR fixes a long-standing bug (#7714) where st.bar_chart (and the other built-in chart commands: st.line_chart, st.area_chart, st.scatter_chart) rendered blank charts when a DataFrame column name contained characters that Vega-Lite treats as special in field strings (., [, ], \).
The fix introduces an aliasing layer in _convert_col_names_to_str_in_place that detects unsafe column names and renames them to internal aliases (e.g., col -- streamlit-generated-0). An alias_to_original mapping is threaded through all encoding helpers (x, y, color, size, tooltip, sort) so user-facing labels, legends, tooltips, and sort references continue to display the original column names.
Reviewed by: claude-4.6-opus-high-thinking, gpt-5.3-codex-high — both completed successfully.
Code Quality
Both reviewers agree the implementation is well-structured and cohesive:
- Aliasing design is clean:
_convert_col_names_to_str_in_placeowns alias generation and collision handling, while downstream encoding helpers consume an explicitalias_to_originalmap. - Collision avoidance is robust: the alias generation uses a counter-bump loop to avoid conflicts with existing user column names, including pathological cases.
- Duplicate column handling: the position-aware
_remap_ycorrectly consumes aliases in column order so DataFrames with duplicate labels each get their own alias. - Melted-color rewrite: rather than using a Vega-Lite
labelExprhack, the PR rewrites melted-color values back to original names in the DataFrame itself — simpler and more robust.
No merge-blocking implementation defects were identified by either reviewer.
Test Coverage
Both reviewers found test coverage comprehensive across all layers:
- Unit tests (
built_in_chart_utils_test.py): verify dotted/bracketed column aliasing with round-trip assertions, collision avoidance, and updated callers for the newalias_to_originalparameter. - Integration tests (
vega_charts_test.py): 8 new test cases covering single-column dot, brackets, tooltip original name, multi-column legend, multi-column tooltip, anti-regression for plain columns, sort by dotted column, and duplicate-label collision handling. Each test includes targeted negative assertions. - E2E tests (
st_bar_chart_test.py): 2 new visual snapshot tests for dotted and bracketed column names, withTOTAL_BAR_CHARTScount correctly updated from 28 to 30.
One reviewer suggested adding a command-level regression test for backslash (\) column names to complete coverage of all four special characters — currently only . and [] are exercised at the integration test layer. This is a minor gap and non-blocking.
Backwards Compatibility
Both reviewers agree there are no breaking changes:
- The public API signatures (
st.bar_chart,st.line_chart,st.area_chart,st.scatter_chart) are unchanged. - Aliasing only activates when column names contain special characters — plain column names pass through untouched (verified by
test_bar_chart_plain_column_names_are_not_aliased). - The
_prep_datareturn type change only affects the internal (underscore-prefixed) function, not any public interface.
Security & Risk
Both reviewers found no security concerns:
- No changes to network handling, authentication, file I/O, or session management.
- No new dependencies, no dynamic code execution, no subprocess or eval usage.
- The aliased column names use the existing
_PROTECTION_SUFFIXpattern already established in the codebase.
Regression risk is medium-low: the change touches shared chart data-prep code used by all built-in chart types, but the fix only activates for columns containing special characters, and the expanded test suite (unit + integration + E2E snapshots) provides strong regression protection.
External test recommendation
- Recommend external_test: No
- Triggered categories: None
- Evidence:
lib/streamlit/elements/lib/built_in_chart_utils.py: Pure data-transformation logic — no routing, auth, embedding, asset serving, or networking changes.- E2E additions are display-only with no external dependencies.
- Suggested external_test focus areas: N/A
- Confidence: High (both reviewers agree)
- Assumptions and gaps: None — the change is entirely within the Vega-Lite data-prep layer and does not touch any external-test risk categories.
Accessibility
No accessibility concerns. Both reviewers confirm that no frontend components, HTML structure, ARIA attributes, or keyboard interaction patterns are modified. The changes are entirely in the Python backend data-preparation layer.
Readability
PR title and description:
Both reviewers noted the PR title [fix] st.bar_chart fails on column names containing "." slightly understates the scope, since the fix also covers [, ], and \ across all built-in chart types. One reviewer proposed: [fix] Handle Vega-Lite special characters in built-in chart column names. This is a minor observation and non-blocking, since the dot is the most common trigger and the linked issue (#7714) specifically references it.
Code comments and naming:
Overall naming is clear and self-documenting (alias_to_original, reserved_names, _needs_field_alias, _VEGA_LITE_FIELD_SPECIAL_CHARS). Comments explain "why" rather than "what."
Minor findings (covered by inline comments):
built_in_chart_utils.py, line 695: "FIFO queue" comment could be clarified to "ordered list consumed left-to-right."built_in_chart_utils.py, line 737:list.pop(0)is O(n) but negligible for the expected workload of 1-2 aliases.
Recommendations
- Consider adding a command-level regression test for backslash (
\) column names to complete coverage of all four special characters at the integration test layer. - Consider updating the PR title to reflect the broader scope (all special characters, all built-in chart types) if desired.
- The minor comment/naming suggestions in inline comments are optional stylistic improvements that can be addressed in a follow-up.
Verdict
APPROVED: Both reviewers unanimously approve. This is a well-crafted bugfix with comprehensive test coverage across all layers, correct edge-case handling, no breaking changes, and no security concerns. All suggestions are minor and non-blocking.
This is a consolidated AI review by claude-4.6-opus-high-thinking, synthesizing reviews from claude-4.6-opus-high-thinking and gpt-5.3-codex-high. Please verify the feedback and use your judgment.
This review also includes 3 inline comment(s) on specific code lines.
| """Regression test for #7714 companion report: column names containing | ||
| square brackets (e.g. 'CO2 Storage [t]') must also render. | ||
| """ | ||
| df = pd.DataFrame({"CO2 Storage [t]": [1, 2, 3, 4]}) |
There was a problem hiding this comment.
suggestion: Add a sibling regression case with a column name containing \\ so command-level tests explicitly cover all four special characters handled by the aliasing path (., [, ], \\). Currently only . and [] are exercised at this layer.
| # FIFO queue of aliases per original column name. When ``y_column_list`` has | ||
| # duplicate labels (e.g. two columns both named ``"a.b"``), each entry | ||
| # consumes a distinct alias in column order (see #7714 follow-up). | ||
| per_name_aliases: dict[str, list[str]] = {} |
There was a problem hiding this comment.
nitpick: The comment says "FIFO queue" but this is a plain list consumed via pop(0). Consider rewording to "Ordered list of aliases per original column name, consumed left-to-right" to avoid momentarily misleading a reader into expecting a collections.deque.
| if aliases: | ||
| # Intentionally destructive: each queue entry is consumed once, | ||
| # matching one DataFrame column occurrence. | ||
| return aliases.pop(0) |
There was a problem hiding this comment.
thought: list.pop(0) is O(n) per call. With typically 1-2 aliases per name this is negligible, but if this ever scaled, a collections.deque with popleft() would be O(1). Fine as-is for the expected workload.
) Vega-Lite treats "." in a field string as nested-object access and "["/"]" as property access. When a user passed a DataFrame whose only column name contained one of these characters (e.g. "col.name"), the built-in chart commands (bar/line/area/scatter_chart) emitted that name verbatim as the encoding field. Vega-Lite then looked up a non-existent nested field and rendered a blank chart. The multi-column path already avoided this by melting the frame and using an internal name. This commit extends that idea to all cases: during data prep we rename any column whose name contains ".", "[", "]", or "\" to a safe internal alias, and remember the original name. Axis titles, tooltips, size legends, and the melted color legend all consult that alias-to-original map so users still see their column names in the UI. The altair_chart / vega_lite_chart passthrough paths are untouched.
When a DataFrame contained both a column with Vega-Lite-special characters (e.g. "a.b") and a plain column literally named like the default alias (e.g. "col -- streamlit-generated-0"), the aliasing step produced duplicate column labels. Bump the alias with a trailing counter until it does not collide with any plain user column or a previously generated alias. Adds a regression test.
The earlier fix only remapped the color legend labels via labelExpr; the melted-color tooltip on hover still showed internal aliases because its displayed value comes from the raw data column. Rewrite the melted color column's values back to the original user-facing y column names right after melting so both the legend and the tooltip pick them up naturally, and drop the now-redundant labelExpr indirection. Adds a regression test covering the tooltip values.
## Describe your changes Automated snapshot updates for #16089 created via the snapshot autofix CI workflow. This workflow was triggered by adding the `update-snapshots` label to a PR after Playwright E2E tests failed with snapshot mismatches. **Updated snapshots:** 6 file(s)⚠️ **Please review the snapshot changes carefully** - they could mask visual bugs if accepted blindly. This PR targets a feature branch and can be merged without review approval. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Snapshot-only PRs can hide unintended visual bugs if baselines are accepted without review; risk is moderate because six images changed but no runtime code did. > > **Overview** > **Automated Playwright E2E baseline refresh** for the feature work in #16089, produced by the `snapshot-autofix` workflow after the `update-snapshots` label was applied when visual snapshot tests failed in CI. > > This PR **replaces six existing screenshot snapshots** with versions taken from the latest failed Playwright run so E2E visual checks align with the new UI from the parent branch. There is **no application or test logic change** here—only committed image assets under the E2E snapshot directories. > > Reviewers should **compare the image diffs** to confirm the pixel changes match intentional UI updates from #16089 and are not accidental regressions. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit ef707c4. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> Co-authored-by: Streamlit Bot <core+streamlitbot-github@streamlit.io>
69dbe90 to
3a88dbd
Compare
There was a problem hiding this comment.
Summary
This PR fixes GitHub issue #7714: built-in charts (st.bar_chart, st.line_chart, st.area_chart, st.scatter_chart) rendered blank when a column name contained a character Vega-Lite treats as special inside a field string (., [, ], \). Vega-Lite interprets col.name as nested-object access, so the single-column shortcut path produced an empty chart.
The fix is contained entirely in lib/streamlit/elements/lib/built_in_chart_utils.py. Offending columns are renamed to an internal safe alias inside _convert_col_names_to_str_in_place, which returns an alias_to_original map that is threaded through the x/y/color/size/tooltip/sort encoding helpers so axis titles, legends, and tooltips still display the user's original column name. For the multi-series melt path, the melted-color values are rewritten back to the originals so the legend/tooltip stay correct without a Vega-Lite labelExpr remap. Coverage adds 6 Python unit/integration tests plus 2 Playwright regression cases and snapshots for st_bar_chart.
All three reviewers agreed the change is well-scoped, well-commented, and correct for the core regression. The one point of disagreement was the multi-series melt/color-remap path, which one reviewer flagged as merge-blocking; this is addressed under Code Quality and Verdict below.
Code Quality
Reviewers unanimously found the change clean, well-contained in the shared helper, and correct in its handling of the tricky edge cases: per-index aliases so columns with colliding stringified names map to distinct DataFrame columns, a reserved_names guard plus counter suffix so a generated alias cannot shadow a plain user column, and a position-aware _remap_y (FIFO queue) so duplicate y labels each address a distinct column while single-value args use first-occurrence-wins to match pandas' duplicate-label selection.
The one disagreement: one reviewer flagged the post-melt color remap (built_in_chart_utils.py:463) as a merge-blocking correctness bug, arguing that remapping melted color values back to the original name collapses distinct series when two y-columns stringify to the same special-character name. I verified this empirically. The observation is technically accurate — two y-columns sharing an identical display name do merge into one color group — but it is not a regression and not merge-blocking: plain duplicate labels (e.g. ["ab", "ab"]) already collapse into a single color group via pd.melt on the pre-existing path, no data is dropped (all melted values remain present), and this special-character case previously rendered fully blank. The PR therefore makes the degenerate duplicate-name input behave consistently with plain names while fixing the blank-render bug. This is captured as a low-severity inline note rather than a blocker.
One neutral behavioral note (not a regression): _prep_data now returns and generate_chart now consumes the stringified/prepared y_column_list for the color-scale domain. For integer column names combined with an explicit color list this tightens domain/data consistency, so it is an improvement rather than a break.
Test Coverage
Coverage for the core regression is strong and matches documented patterns. Backend tests in vega_charts_test.py and built_in_chart_utils_test.py exercise dotted/bracketed names, single vs. multi column, tooltip/axis-title preservation, sort-by-dotted-column aliasing, the melted-color legend/tooltip round-trip (asserting no stale labelExpr and no streamlit-generated leakage), alias/plain-column collision, and colliding stringified names each getting distinct aliases. Good anti-regression assertions are present (e.g. test_bar_chart_plain_column_names_are_not_aliased, the collision test asserting not df.columns.duplicated().any()). The e2e additions in st_bar_chart.py/st_bar_chart_test.py add dotted and bracketed single-column cases, relying on the existing get_vega_graphics_document count assertion (which proves the charts are no longer blank) plus snapshots.
The remaining gaps are breadth, not depth, and are non-blocking: the new command-level regressions only cover st.bar_chart even though the shared helper affects all four built-in charts; there is no regression for a backslash-containing name; and the color-column (not color list) alias-title branch in _get_color_encoding is only indirectly exercised.
Backwards Compatibility
No breaking changes. For plain column names the alias maps are empty and every helper returns the same stringified names as before, so the generated spec is unchanged. st.altair_chart/st.vega_lite_chart passthrough paths do not use generate_chart and are untouched. The only externally observable changes are the intended fix (previously-blank charts now render) and the more consistent color-domain typing noted above.
Security & Risk
No security concerns, and reviewers agreed. The change is pure server-side chart spec/data-prep logic: no WebSocket/session/auth handling, no file/asset serving or path handling, no cookies/CORS/CSP, no new dependencies, no eval/exec/subprocess, no HTML/Markdown rendering, and no iframe/postMessage logic. Alias strings are internally generated and cannot contain the special characters themselves, so they cannot reintroduce the field-string injection behavior. The main residual risk is functional regression in the shared built-in chart path, assessed above as consistent with existing behavior.
External test recommendation
- Recommend external_test: No
- Triggered categories: None
- Evidence:
lib/streamlit/elements/lib/built_in_chart_utils.py: pure chart spec/data-prep logic; no routing, transport, auth, embedding, assets, storage, or headers involved.e2e_playwright/st_bar_chart.py/st_bar_chart_test.py: standard local-app snapshot/count assertions; no cross-origin, iframe, or host-page behavior.
- Suggested external_test focus areas: none — the Vega-Lite spec is produced on the backend and is identical regardless of hosting context.
- Confidence: High (unanimous across all three reviews)
- Assumptions and gaps: assessment is based on the diff and changed-file review; the rendered spec does not diverge between local and externally hosted/embedded runs.
Accessibility
No accessibility regressions, and the change is directionally positive: axis titles, legend labels, and tooltip titles now display the user's original column name instead of a blank chart or an internal alias. No changes to DOM structure, focus handling, ARIA, or color contrast.
Readability
The added comments and docstrings are thorough and consistently explain why (Vega-Lite field-string semantics, the collision/duplicate rationale, the "intentionally destructive" pop) rather than restating mechanics; naming (_needs_field_alias, alias_to_original, per_name_aliases) is clear and self-describing. One docstring wording issue on _prep_data is raised as an inline comment.
On the PR title/description, reviewers agreed the metadata is functional but could be sharper. The title describes the symptom (fails on column names containing ".") and understates scope, since the shared helper affects all four built-in charts; consider something like [fix] Handle special field characters in built-in charts. The description leads with the bug rather than the change and appears stale where it mentions labelExpr aliasing, even though the current diff rewrites melted color values instead; consider leading with the change, listing the affected commands, and dropping the raw pass-count from the testing plan. All of this is optional polish.
Recommendations
- (Optional, follow-up) Broaden regression coverage: add a regression for at least one non-bar built-in chart and a backslash-containing column name to match the broadened implementation scope.
- (Optional, follow-up) Add a unit test for a color column with a special-character name to directly exercise the alias-title branch in
_get_color_encoding. - (Optional) Tighten the PR title and description to reflect all built-in charts, lead with the change, and drop the stale
labelExprmention and raw pass count. - (Optional, addressed inline) Refine the
_prep_datadocstring wording and, if desired, document the duplicate-display-name melt behavior.
Verdict
APPROVED: A well-scoped, well-tested bugfix with no security, compatibility, or genuine correctness regressions. Two of three reviewers approved; the one change request was based on the multi-series melt/color-remap path, which I verified is consistent with pre-existing behavior for duplicate display names and a strict improvement over the previously-blank render — a non-blocking edge case rather than a merge blocker. Remaining items are optional coverage-breadth and readability follow-ups.
This is a consolidated automated AI review by claude-opus-4-8-thinking-xhigh, synthesizing reviews from gpt-5.4-xhigh, gemini-3.1-pro, and claude-opus-4-8-thinking-xhigh. Please verify the feedback and use your judgment.
This review also includes 2 inline comment(s) on specific code lines.
| and alias_to_original | ||
| and color_column in melted_data.columns | ||
| ): | ||
| melted_data[color_column] = melted_data[color_column].map( |
There was a problem hiding this comment.
thought: When two y-columns share an identical special-character display name (e.g. duplicate "a.b" labels), remapping the melted color values back to the original name collapses both series into a single color group. Note this is not a regression: plain duplicate labels (e.g. ["ab", "ab"]) already collapse the same way via pd.melt, and this case previously rendered blank, so the change is a strict improvement. If you want to distinguish duplicate-named series in the legend, keep a unique internal grouping key and remap only the displayed legend text via labelExpr — but that would diverge from the existing plain-name behavior and produce two identical legend entries. Optional follow-up at most.
|
|
||
| Returns the prepared dataframe and the new names of the x column (taking the index | ||
| reset into consideration) and y, color, and size columns. | ||
| reset into consideration), the y, color, and size columns, the aliased y column |
There was a problem hiding this comment.
nitpick: The docstring calls the returned list "the aliased y column list", but its contents depend on the path: for a single special-char column it holds the alias, while for the multi-column melt path it is rewritten back to the original names (and for plain columns it holds the stringified originals). It also omits sort_column. Consider rewording to "the y column list (in user-facing form after any melt remap)" and including sort_column so a newcomer isn't misled about what the value contains.
Describe your changes
st.bar_chart(and other built-in charts) failed to render when a single-column dataframe had a.in its column name — Vega-Lite parses.in a field string as nested access, socol.namewas interpreted asrow["col"]["name"].The multi-column path already escaped this via a melt/rename, but the single-column shortcut passed raw column names to Altair.
Fix:
_convert_col_names_to_str_in_placenow detects unsafe characters (.,[,],\) and renames such columns to a safe alias (col -- streamlit-generated-<idx>), returning analias_to_originalmap. The map is threaded through x/y/color/size/tooltip/sort encoding helpers so axis titles, legends, tooltips, and sort refs still show the original name.Also covers
.-in-name forst.scatter_chart,st.line_chart, andst.area_chart, plus bracket/backslash edge cases.GitHub Issue Link (if applicable)
Testing Plan
lib/tests/streamlit/elements/, 6 new tests invega_charts_test.pyandbuilt_in_chart_utils_test.pycovering dotted/bracketed names, single/multi-column, tooltip/sort/color/size pathsst_bar_chart_test.py(snapshots will need CI regen)st.altair_chartandst.vega_lite_chartpassthrough paths untouched; explicity=["a.b", "c.d"]legend now useslabelExpraliasing — visually equivalentNote
Medium Risk
Touches shared chart data-prep and Vega-Lite encoding for all built-in chart types; behavior change is targeted but central to rendering correctness.
Overview
Fixes blank built-in charts when DataFrame columns contain
.,[,], or\— Vega-Lite was treating those names as nested/property access instead of literal field names (notably the single-column path in #7714).built_in_chart_utilsnow renames affected columns to internal safe aliases during prep, keeps analias_to_originalmap, and threads it through axis, color, size, tooltip, and sort encodings so legends and labels still show the user’s column names. Multi-series melts rewrite melted color values back to originals so legends/tooltips stay correct withoutlabelExpr.Adds Playwright regression charts for dotted and bracketed names plus unit/integration tests for aliasing, collisions, sort, and multi-column duplicate labels.
Reviewed by Cursor Bugbot for commit 3a88dbd. Bugbot is set up for automated code reviews on this repo. Configure here.