You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "neilconway (via GitHub)" <gi...@apache.org> on 2026/04/25 14:37:40 UTC
[PR] perf: Use bulk-NULL builder in `chr` [datafusion]
neilconway opened a new pull request, #21847:
URL: https://github.com/apache/datafusion/pull/21847
## Which issue does this PR close?
- Closes #21846.
## Rationale for this change
Optimize `chr` by avoiding per-row NULL bitmap maintenance, and also split the hot loop to avoid taking a branch when no NULL bitmap is given.
Benchmarks:
```
`chr/array`: 3.8768 µs → 3.1548 µs, −18.57% (`p < 0.05`)
```
## What changes are included in this PR?
* Optimize `chr` to reduce NULL-handling overhead
## Are these changes tested?
Yes.
## Are there any user-facing changes?
No.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: Use bulk-NULL builder in `chr` [datafusion]
Posted by "Weijun-H (via GitHub)" <gi...@apache.org>.
Weijun-H commented on code in PR #21847:
URL: https://github.com/apache/datafusion/pull/21847#discussion_r3142912040
##########
datafusion/functions/src/string/chr.rs:
##########
@@ -32,31 +33,46 @@ use datafusion_macros::user_doc;
/// Returns the character with the given code.
/// chr(65) = 'A'
fn chr_array(integer_array: &Int64Array) -> Result<ArrayRef> {
- let mut builder = GenericStringBuilder::<i32>::with_capacity(
- integer_array.len(),
- // 1 byte per character, assuming that is the common case
- integer_array.len(),
+ let len = integer_array.len();
+ let mut builder = GenericStringArrayBuilder::<i32>::with_capacity(
+ len, // 1 byte per character, assuming that is the common case
+ len,
);
let mut buf = [0u8; 4];
+ let nulls = integer_array.nulls();
- for integer in integer_array {
- match integer {
- Some(integer) => {
- if let Ok(u) = u32::try_from(integer)
- && let Some(c) = core::char::from_u32(u)
- {
- builder.append_value(c.encode_utf8(&mut buf));
- continue;
- }
-
- return exec_err!("invalid Unicode scalar value: {integer}");
+ if let Some(n) = nulls {
+ for i in 0..len {
+ if n.is_null(i) {
+ builder.append_placeholder();
Review Comment:
Could we add an explicit assertion in `test_chr_normal` that the `None` input remains NULL, e.g. `assert!(string_array.is_null(7))`? The test currently checks `value(7) == ""`, which would also pass if this path accidentally produced a non-null empty string.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: Use bulk-NULL builder in `chr` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #21847:
URL: https://github.com/apache/datafusion/pull/21847
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: Use bulk-NULL builder in `chr` [datafusion]
Posted by "Weijun-H (via GitHub)" <gi...@apache.org>.
Weijun-H commented on code in PR #21847:
URL: https://github.com/apache/datafusion/pull/21847#discussion_r3142912040
##########
datafusion/functions/src/string/chr.rs:
##########
@@ -32,31 +33,46 @@ use datafusion_macros::user_doc;
/// Returns the character with the given code.
/// chr(65) = 'A'
fn chr_array(integer_array: &Int64Array) -> Result<ArrayRef> {
- let mut builder = GenericStringBuilder::<i32>::with_capacity(
- integer_array.len(),
- // 1 byte per character, assuming that is the common case
- integer_array.len(),
+ let len = integer_array.len();
+ let mut builder = GenericStringArrayBuilder::<i32>::with_capacity(
+ len, // 1 byte per character, assuming that is the common case
+ len,
);
let mut buf = [0u8; 4];
+ let nulls = integer_array.nulls();
- for integer in integer_array {
- match integer {
- Some(integer) => {
- if let Ok(u) = u32::try_from(integer)
- && let Some(c) = core::char::from_u32(u)
- {
- builder.append_value(c.encode_utf8(&mut buf));
- continue;
- }
-
- return exec_err!("invalid Unicode scalar value: {integer}");
+ if let Some(n) = nulls {
+ for i in 0..len {
+ if n.is_null(i) {
+ builder.append_placeholder();
Review Comment:
Since this path now relies on preserving the input null bitmap via `finish(nulls.cloned())`, could we add an explicit assertion that the NULL slot remains NULL? `test_chr_normal` currently includes a `None` input, but only checks `value(i) == ""`, which would also pass if the result accidentally became a non-null empty string.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: Use bulk-NULL builder in `chr` [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway commented on code in PR #21847:
URL: https://github.com/apache/datafusion/pull/21847#discussion_r3143496731
##########
datafusion/functions/src/string/chr.rs:
##########
@@ -32,31 +33,46 @@ use datafusion_macros::user_doc;
/// Returns the character with the given code.
/// chr(65) = 'A'
fn chr_array(integer_array: &Int64Array) -> Result<ArrayRef> {
- let mut builder = GenericStringBuilder::<i32>::with_capacity(
- integer_array.len(),
- // 1 byte per character, assuming that is the common case
- integer_array.len(),
+ let len = integer_array.len();
+ let mut builder = GenericStringArrayBuilder::<i32>::with_capacity(
+ len, // 1 byte per character, assuming that is the common case
+ len,
);
let mut buf = [0u8; 4];
+ let nulls = integer_array.nulls();
- for integer in integer_array {
- match integer {
- Some(integer) => {
- if let Ok(u) = u32::try_from(integer)
- && let Some(c) = core::char::from_u32(u)
- {
- builder.append_value(c.encode_utf8(&mut buf));
- continue;
- }
-
- return exec_err!("invalid Unicode scalar value: {integer}");
+ if let Some(n) = nulls {
+ for i in 0..len {
+ if n.is_null(i) {
+ builder.append_placeholder();
Review Comment:
Good idea! Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org