chore: Prefer `append_value_n` over `append_value` by Fokko · Pull Request #1868 · delta-io/delta-kernel-rs

Fokko · 2026-02-17T22:03:42Z

What changes are proposed in this pull request?

Instead of adding the value in the loop, we push the operation down to Arrow which is much faster.

How was this change tested?

codecov · 2026-02-17T22:06:56Z

Codecov Report

❌ Patch coverage is 60.00000% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.89%. Comparing base (af71c28) to head (9ca0eb2).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
kernel/src/engine/arrow_expression/mod.rs	60.00%	10 Missing and 2 partials ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1868   +/-   ##
=======================================
  Coverage   85.89%   85.89%           
=======================================
  Files         137      137           
  Lines       40012    40012           
  Branches    40012    40012           
=======================================
  Hits        34368    34368           
  Misses       4155     4155           
  Partials     1489     1489

☔ View full report in Codecov by Sentry.
📢 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.

emkornfield · 2026-02-17T22:13:39Z

kernel/src/engine/arrow_expression/mod.rs

-                    for (builder, value) in field_builders.zip(data.values()) {
-                        value.append_to(builder, 1)?;
-                    }
                    builder.append(true);


There isn't a corresponding bulk operation for this?

Sadly no... I only see one for bulk-appending nulls:
https://docs.rs/arrow/latest/arrow/array/struct.StructBuilder.html#method.append_nulls

Under the hood, both append and append_null are delegating to the underlying NullBufferBuilder, and even the bulk-null is extending from an internally created vec![false; n], which doesn't seem especially fantastic either -- NullBufferBuilder::append_n_nulls would almost certainly be more efficient, and there's a matching NullBufferBuilder::append_n_non_nulls?

Looks like it is missing from the struct_builder.rs: https://github.com/apache/arrow-rs/blob/2f40f78e4feae3aee261d9608cede9535e1429e0/arrow-array/src/builder/struct_builder.rs#L209-L214

OK, maybe lets add a small TODO and consider a contribution upstream?

Created two PRs:

Add append_non_nulls to StructBuilder apache/arrow-rs#9430

Avoid allocating a Vec in StructBuilder apache/arrow-rs#9428

emkornfield · 2026-02-17T22:16:56Z

kernel/src/engine/arrow_expression/mod.rs

                // StructBuilder and ListBuilder both provide it.
                let builder =
                    builder_as!(array::MapBuilder<Box<dyn ArrayBuilder>, Box<dyn ArrayBuilder>>);
                for _ in 0..num_rows {


I think for the null case we should be able to optimize this as well?

I don't see any bulk-null appender nor any way to access the underlying null buffer builder?

See comment above 😢: https://github.com/delta-io/delta-kernel-rs/pull/1868/changes#diff-c828492a4025a2eb1cecb1cc0a2780b9344af82b23dd4a3d4fe7772b08643299R202-R203

Same comment on TODO.

Or we can also try this with REE arrays, if it still isn't fast enough

Raise a PR here:

apache/arrow-rs#9432

@emkornfield You mean RLE?

emkornfield

A few small questions, otherwise generally seems reasonable. It might be good to have a small microbenchmark to demonstrate the speed difference.

scovich

LGTM, given the limitations of arrow-rs API.

scovich · 2026-02-17T22:23:16Z

kernel/src/engine/arrow_expression/mod.rs

-                    for (builder, value) in field_builders.zip(data.values()) {
-                        value.append_to(builder, 1)?;
-                    }
                    builder.append(true);


Sadly no... I only see one for bulk-appending nulls:
https://docs.rs/arrow/latest/arrow/array/struct.StructBuilder.html#method.append_nulls

Under the hood, both append and append_null are delegating to the underlying NullBufferBuilder, and even the bulk-null is extending from an internally created vec![false; n], which doesn't seem especially fantastic either -- NullBufferBuilder::append_n_nulls would almost certainly be more efficient, and there's a matching NullBufferBuilder::append_n_non_nulls?

scovich · 2026-02-17T22:31:52Z

kernel/src/engine/arrow_expression/mod.rs

                // StructBuilder and ListBuilder both provide it.
                let builder =
                    builder_as!(array::MapBuilder<Box<dyn ArrayBuilder>, Box<dyn ArrayBuilder>>);
                for _ in 0..num_rows {


I don't see any bulk-null appender nor any way to access the underlying null buffer builder?

Fokko · 2026-02-18T08:38:08Z

kernel/src/engine/arrow_expression/mod.rs

+            }};
+        }
+
+        // Use append_value in a loop for builders without batch append (String, Binary)


I've created an upstream PR to add this to the GenericByteArray: apache/arrow-rs#9426

It will both harmonize the APIs between the builders and throw in a 10-20% speed improvement 🥳

Fokko · 2026-02-18T12:38:59Z

Added TODOs and moving this forward since there is concensus. Let's pick up the remaining things in separate PRs. Thanks @scovich and @emkornfield for the prompt review 🚀

github-actions bot assigned Fokko Feb 17, 2026

emkornfield reviewed Feb 17, 2026

View reviewed changes

emkornfield requested changes Feb 17, 2026

View reviewed changes

scovich approved these changes Feb 17, 2026

View reviewed changes

emkornfield approved these changes Feb 17, 2026

View reviewed changes

Fokko commented Feb 18, 2026

View reviewed changes

Fokko added 2 commits February 18, 2026 11:14

Chore: Prefer append_value_n over append_value

55e05aa

Add TODOs

c7d6416

Fokko force-pushed the fd-optimize-insert-value branch from 8f0c9f2 to c7d6416 Compare February 18, 2026 10:15

Merge branch 'main' into fd-optimize-insert-value

9ca0eb2

github-actions bot added the breaking-change Change that require a major version bump label Feb 18, 2026

Fokko merged commit 6363e88 into delta-io:main Feb 18, 2026
21 of 22 checks passed

Fokko deleted the fd-optimize-insert-value branch February 18, 2026 12:39

Conversation

Fokko commented Feb 17, 2026

What changes are proposed in this pull request?

How was this change tested?

Uh oh!

codecov bot commented Feb 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fokko Feb 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emkornfield left a comment

Choose a reason for hiding this comment

Uh oh!

scovich left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fokko commented Feb 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Feb 17, 2026 •

edited

Loading

Fokko Feb 18, 2026 •

edited

Loading

Fokko commented Feb 18, 2026 •

edited

Loading