Skip to content

fix: handle multibyte characters in physical plan tree renderer - #24664

Open
rustyconover wants to merge 2 commits into
apache:mainfrom
rustyconover:fix/multibyte-tree-renderer
Open

fix: handle multibyte characters in physical plan tree renderer#24664
rustyconover wants to merge 2 commits into
apache:mainfrom
rustyconover:fix/multibyte-tree-renderer

Conversation

@rustyconover

Copy link
Copy Markdown

Which issue does this PR close?

Rationale for this change

Rendering a physical plan in tree format can panic when an operator name or detail contains multibyte UTF-8 characters. The renderer measures text in characters but previously used those character positions as byte offsets when slicing strings. Those offsets are not necessarily valid UTF-8 boundaries.

What changes are included in this PR?

Use character-based iteration consistently when truncating and wrapping text in the physical plan tree renderer. Also use the number of characters, rather than the number of bytes, when checking for remaining text.

Are these changes tested?

Yes. Unit tests cover both wrapping and truncating text containing multibyte characters.

The following checks pass:

  • cargo fmt --all -- --check
  • cargo test -p datafusion-physical-plan --lib
  • cargo clippy -p datafusion-physical-plan --all-targets --all-features -- -D warnings

Are there any user-facing changes?

EXPLAIN output using the tree renderer no longer panics when rendered plan details contain multibyte UTF-8 characters. There are no public API changes.

@github-actions github-actions Bot added the physical-plan Changes to the physical-plan crate label Aug 25, 2026

@saadtajwar saadtajwar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotta love Rust 😆 this makes sense & LGTM!

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.44%. Comparing base (63f5b55) to head (c3dda53).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #24664   +/-   ##
=======================================
  Coverage   81.44%   81.44%           
=======================================
  Files        1118     1118           
  Lines      399550   399568   +18     
  Branches   399550   399568   +18     
=======================================
+ Hits       325398   325414   +16     
+ Misses      55154    55153    -1     
- Partials    18998    19001    +3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@nuno-faria nuno-faria left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @rustyconover. Could you also add a sqllogictest at explain_tree.slt? Something like this which currently panics on main:

create table t (a varchar);
explain select * from t where a = '🌤🌤🌤🌤🌤🌤🌤🌤🌤🌤🌤🌤🌤🌤🌤🌤🌤🌤🌤🌤🌤🌤🌤🌤';

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

Labels

physical-plan Changes to the physical-plan crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Tree-format physical plan renderer can panic on multibyte UTF-8 text

4 participants