[Variant] Use simdutf8 for UTF-8 validation #7908

codephage2020 · 2025-07-11T16:42:11Z

Which issue does this PR close?

We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax.

Closes [Variant] Offer simdutf8 as an optional dependency when validating metadata #7902.

Rationale for this change

What changes are included in this PR?

add simdutf8 dependency for parquet-variant
add a fn extract_and_validate_utf8_slice

Are these changes tested?

We typically require tests for all PRs in order to:

Prevent the code from being accidentally broken by subsequent changes
Serve as another way to document the expected behavior of the code

If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?

Are there any user-facing changes?

If there are user-facing changes then we may require documentation to be updated before approving the PR.

If there are any breaking changes to public APIs, please call them out.

Signed-off-by: codephage2020 <tingwangyan2020@163.com>

codephage2020 · 2025-07-11T16:58:33Z

CC @alamb @friendlymatthew @Dandandan .
Please review when you have time. Let me know if any adjustments are needed!

friendlymatthew

Thank you very much @codephage2020. I just have a few comments

parquet-variant/Cargo.toml

parquet-variant/src/utils.rs

Signed-off-by: codephage2020 <tingwangyan2020@163.com>

codephage2020 · 2025-07-12T03:57:59Z

I just have a few comments

hi, @friendlymatthew . Thanks for your review! I've addressed all your comments in the latest update.

friendlymatthew

Thank you @codephage2020. I had some comments but overall LGTM

parquet-variant/Cargo.toml

parquet-variant/src/utils.rs

Signed-off-by: codephage2020 <tingwangyan2020@163.com>

scovich

LGTM, tho I question the utility of one bit of complexity this PR adds.

scovich · 2025-07-12T16:45:35Z

parquet-variant/src/utils.rs

+    let offset_buffer = match offset {
+        0 => slice_from_slice(slice, range)?,
+        _ => slice_from_slice_at_offset(slice, offset, range)?,
+    };


The branch to test for zero probably costs more than applying an offset of zero?

Maybe we could get a benchmark and show

I agree that unless we have a benchmark that shows this optimization makes a measurable difference, it would be better to go with the simpler code

…utf8-validation

alamb

Thanks @codephage2020 @scovich and @friendlymatthew

alamb · 2025-07-12T17:03:23Z

parquet-variant/src/utils.rs

+    let offset_buffer = match offset {
+        0 => slice_from_slice(slice, range)?,
+        _ => slice_from_slice_at_offset(slice, offset, range)?,
+    };


Maybe we could get a benchmark and show

I agree that unless we have a benchmark that shows this optimization makes a measurable difference, it would be better to go with the simpler code

codephage2020 · 2025-07-13T12:46:18Z

CC @alamb @scovich .
I conducted a simple benchmark test locally.

result:

 direct_slice_at_offset_zero
                        time:   [1.0067 ns 1.0092 ns 1.0122 ns]
                        change: [−0.9043% −0.2643% +0.3090%] (p = 0.42 > 0.05)
                        No change in performance detected.
Found 6 outliers among 100 measurements (6.00%)
  4 (4.00%) high mild
  2 (2.00%) high severe

branched_slice_at_offset_zero
                        time:   [1.4050 ns 1.4100 ns 1.4157 ns]
                        change: [−0.3173% +0.0921% +0.4766%] (p = 0.65 > 0.05)
                        No change in performance detected.
Found 2 outliers among 100 measurements (2.00%)
  1 (1.00%) high mild
  1 (1.00%) high severe

@scovich The results show that you are right. And I will delete the branch for testing zero.

Below is the code for the benchmark test. Please let me know if there are any issues. This is a temporary test and I won't submit it.

const LARGE_DATA_SIZE: usize = 1 << 20;

fn setup_data() -> Vec<u8> {
    let mut rng = StdRng::seed_from_u64(42);
    (0..LARGE_DATA_SIZE).map(|_| rng.random()).collect()
}

fn bench_simple_direct(c: &mut Criterion) {
    let data = setup_data();
    let range = 0..1000;
    
    c.bench_function("direct_slice_at_offset_zero", |b| {
        b.iter(|| {
            let _ = black_box(slice_from_slice_at_offset(&data, 0, range.clone()));
        })
    });
}

fn bench_with_branch_zero_offset(c: &mut Criterion) {
    let data = setup_data();
    let range = 0..1000;
    let offset = 0;
    
    c.bench_function("branched_slice_at_offset_zero", |b| {
        b.iter(|| {
            let _ = black_box(match offset {
                0 => slice_from_slice(&data, range.clone()),
                _ => slice_from_slice_at_offset(&data, offset, range.clone()),
            });
        })
    });
}

Signed-off-by: codephage2020 <tingwangyan2020@163.com>

codephage2020 · 2025-07-14T14:12:37Z

Hi @alamb ,
Just checking in - could you help merge this PR when you have a moment? Thanks for your review!

alamb · 2025-07-14T16:04:51Z

Sorry for the delay -- thank you @codephage2020 , @scovich and @friendlymatthew

codephage2020 · 2025-07-15T02:14:11Z

Sorry for the delay

All good! Thank you @alamb, @scovich and @friendlymatthew for reviewing and merging this!
Let's move this project forward🚀 !

codephage2020 added 2 commits July 12, 2025 00:30

Use simdutf8 for UTF-8 validation

167ebef

Signed-off-by: codephage2020 <tingwangyan2020@163.com>

add a comment for extract_and_validate_utf8_slice

b46b8fa

Signed-off-by: codephage2020 <tingwangyan2020@163.com>

github-actions bot added the parquet Changes to the parquet crate label Jul 11, 2025

fmt

3641e5a

Signed-off-by: codephage2020 <tingwangyan2020@163.com>

codephage2020 marked this pull request as ready for review July 11, 2025 16:56

friendlymatthew reviewed Jul 11, 2025

View reviewed changes

parquet-variant/Cargo.toml Outdated Show resolved Hide resolved

parquet-variant/src/utils.rs Outdated Show resolved Hide resolved

build(deps): Update the simdutf8 dependency and optimize error handling

b0139ad

Signed-off-by: codephage2020 <tingwangyan2020@163.com>

github-actions bot added the arrow Changes to the arrow crate label Jul 12, 2025

make clippy happy

396ffd5

Signed-off-by: codephage2020 <tingwangyan2020@163.com>

codephage2020 requested a review from friendlymatthew July 12, 2025 03:58

friendlymatthew approved these changes Jul 12, 2025

View reviewed changes

parquet-variant/Cargo.toml Outdated Show resolved Hide resolved

parquet-variant/src/utils.rs Show resolved Hide resolved

codephage2020 added 2 commits July 12, 2025 23:32

Make the simdutf8 dependency optional and optimize UTF-8 validation

1c44c26

Signed-off-by: codephage2020 <tingwangyan2020@163.com>

chore

c6567b6

Signed-off-by: codephage2020 <tingwangyan2020@163.com>

scovich approved these changes Jul 12, 2025

View reviewed changes

alamb added 2 commits July 12, 2025 13:01

Merge remote-tracking branch 'apache/main' into feat/variant-use-simd…

89cd495

…utf8-validation

fix fmt

88176e9

alamb approved these changes Jul 12, 2025

View reviewed changes

codephage2020 added 2 commits July 13, 2025 21:15

Merge branch 'apache:main' into feat/variant-use-simdutf8-validation

9cbac8f

delete the branch for testing zero

4b826ed

Signed-off-by: codephage2020 <tingwangyan2020@163.com>

codephage2020 requested review from alamb and scovich July 14, 2025 14:03

alamb merged commit 52fd59c into apache:main Jul 14, 2025
29 checks passed

codephage2020 deleted the feat/variant-use-simdutf8-validation branch July 15, 2025 02:14

alamb mentioned this pull request Jul 28, 2025

[Variant] Offer simdutf8 as an optional dependency when validating metadata #7902

Closed

[Variant] Use simdutf8 for UTF-8 validation #7908

[Variant] Use simdutf8 for UTF-8 validation #7908

Uh oh!

Conversation

codephage2020 commented Jul 11, 2025

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

codephage2020 commented Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

friendlymatthew left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codephage2020 commented Jul 12, 2025

Uh oh!

friendlymatthew left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

scovich left a comment

Choose a reason for hiding this comment

Uh oh!

scovich Jul 12, 2025

Choose a reason for hiding this comment

Uh oh!

alamb Jul 12, 2025

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alamb Jul 12, 2025

Choose a reason for hiding this comment

Uh oh!

codephage2020 commented Jul 13, 2025

Uh oh!

codephage2020 commented Jul 14, 2025

Uh oh!

Uh oh!

alamb commented Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codephage2020 commented Jul 15, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codephage2020 commented Jul 11, 2025 •

edited

Loading

alamb commented Jul 14, 2025 •

edited

Loading