Implement arrow-row encoding/decoding for view types

[XiangpengHao]

Which issue does this PR close?

Closes #5921 .

Rationale for this change

The core of this pr is simple, just simple logic to allow GenericByteViewArray to be encoded/decoded to arrow-row.

To make things easy (and elegant), I made some opinionated changes:

• Move comparing byte view elements to GenericByteView, rather than in ArrayOrd, so that the function can be used by anyone who have access to GenericByteView, rather than depending on arrow-ord
• Move the core of casting GenericByteArray to GenericByteViewArray to GenericByteViewArray (rather than in the casting subcrate, for the same reason).

What changes are included in this PR?

Are there any user-facing changes?

[alamb]

Thank you @XiangpengHao -- this looks awesome 🙏

To be mergerable I think this PR

1. needs test coverage for BinaryView (even though I know it is very likely going to be the same as StringView)
2. Run the comparision benchmarks to make sure the refactoring doesn't cause the compare kernels to regress.

Some other requests (could be other PRs):

1. Add a benchmark for StringView / BinaryArray conversion in https://github.com/apache/arrow-rs/blob/master/arrow/benches/row_format.rs
2. Add a message to

   arrow-rs/arrow-row/src/lib.rs

   Line 1130 in 20a569a

   _ => unreachable!(),

   To say "support for the type is not yet implemented" rather than "unreachable"?

[alamb]

I considered if we could avoid these clones, but I think are necessary as this function gets references in, but must return something which has no borrowed data (e.g. DynComparator)

[alamb]

in the previous version of this code, the get(left_idx) is unchecked but now it is being checked.

Can you please run the benchmarks to ensure this nice refactoring hasn't decreased the equality kernels?

[XiangpengHao]

Let's wait #5924 to merge and we'll have a better idea

[alamb]

makes sense

[alamb]

I think it got significantly slower -- see #5922 (comment)

[alamb]

👍

[XiangpengHao]

2. Add a message to

   arrow-rs/arrow-row/src/lib.rs

   Line 1130 in 20a569a

   _ => unreachable!(),

   
   To say "support for the type is not yet implemented" rather than "unreachable"?

Changed the (applicable) unreachable to unimplemented with better error message!

[alamb]

Looks good except for the benchmark results. Let's get #5924 in and then merge this PR in and run the benchmarks to be sure

[alamb]

I merged up from main and will now run benchmarks on this branch

[alamb]

According to my benchmarks, this branch shows a significant slowdown when comparing StringViewArrays

++ critcmp master row-view
group                                 master                                 row-view
-----                                 ------                                 --------
eq StringArray StringArray            1.01     38.2±0.27ms        ? ?/sec    1.00     37.9±0.27ms        ? ?/sec
eq StringViewArray StringViewArray    1.00     32.6±0.21ms        ? ?/sec    2.14     69.9±0.09ms        ? ?/sec
eq scalar StringArray                 1.02     28.2±0.23ms        ? ?/sec    1.00     27.8±0.35ms        ? ?/sec
eq scalar StringViewArray             1.00     29.8±0.17ms        ? ?/sec    2.38     71.0±0.09ms        ? ?/sec

Here is the script I used

Details

set -x -e

# cargo bench --bench comparison_kernels String
BENCH_COMMAND="cargo bench --bench comparison_kernels"
BENCH_FILTER="String"
REPO_NAME="XiangpengHao"
BRANCH_NAME="row-view"

git fetch -p apache
git fetch -p korowa
git fetch -p XiangpengHao

# remove old test runs
rm -rf target/criterion/

git checkout $BRANCH_NAME
git reset --hard "$REPO_NAME/$BRANCH_NAME"

# Run on test branch
$BENCH_COMMAND -- --save-baseline ${BRANCH_NAME} ${BENCH_FILTER}

# Run on master
MERGE_BASE=$(git merge-base HEAD apache/master)
echo "** Comparing to ${MERGE_BASE}"

git checkout ${MERGE_BASE}
$BENCH_COMMAND -- --save-baseline master  ${BENCH_FILTER}

critcmp master ${BRANCH_NAME}

[XiangpengHao]

I'll add a compare_unchecked then

[XiangpengHao]

The performance regression is fixed, but not in the way I'd like it to be: https://github.com/apache/arrow-rs/pull/5922/files#diff-9fa51ea9c3da28e8a560d3f76c67178bfa1394874a03aa7d2d33a3979faca72bR556-R557

I think the performance of string view needs some better understanding/profiling. I leave it as a (near) future work.

[XiangpengHao]

Looking a bit deeper at the performance regression. tldr: it seems more like a compiler bug/feature than our problem.

More details:

If we craft a benchmark that never run the code to compare two strings larger than 12 bytes, then we will never reach the logic after the unreachable!("benchmark won't run this code");.

However, adding this unreachable will increase the benchmark performance by 50%, the same as comparing two StringArray. I guess the unreachable allows the compiler to identify some optimization patterns...

 pub unsafe fn compare_unchecked(
        left: &GenericByteViewArray<T>,
        left_idx: usize,
        right: &GenericByteViewArray<T>,
        right_idx: usize,
    ) -> std::cmp::Ordering {
        let l_view = left.views().get_unchecked(left_idx);
        let l_len = *l_view as u32;

        let r_view = right.views().get_unchecked(right_idx);
        let r_len = *r_view as u32;

        if l_len <= 12 && r_len <= 12 {
            let l_data = unsafe { GenericByteViewArray::<T>::inline_value(l_view, l_len as usize) };
            let r_data = unsafe { GenericByteViewArray::<T>::inline_value(r_view, r_len as usize) };
            return l_data.cmp(r_data);
        }

        // one of the string is larger than 12 bytes,
        // we then try to compare the inlined data first
        let l_inlined_data = unsafe { GenericByteViewArray::<T>::inline_value(l_view, 4) };
        let r_inlined_data = unsafe { GenericByteViewArray::<T>::inline_value(r_view, 4) };
        if r_inlined_data != l_inlined_data {
            return l_inlined_data.cmp(r_inlined_data);
        }

        unreachable!("benchmark won't run this code");
        // unfortunately, we need to compare the full data
        let l_full_data: &[u8] = unsafe { left.value_unchecked(left_idx).as_ref() };
        let r_full_data: &[u8] = unsafe { right.value_unchecked(right_idx).as_ref() };
        l_full_data.cmp(r_full_data)
    }

[XiangpengHao]

After many experiments and examining the assembly code, I believe the root cause is that Rust's inline strategy actually makes performance worse due to register spilling.
When calling into value_unchecked, rust try to inline it, which overwhelms the stack registers, which causes excessive memory access.

I've updated the comments and code to prevent value_unchecked from being inlined.

[tustvold]

If lots of the strings are < 12 bytes I can see the difference being meaningful

[alamb]

I agree. However, I think this is a good first implementation think we can do it as a follow on PR: #5945

[alamb]

I re-ran the benchmarks and I no longer see a regression:

++ critcmp master row-view
group                                 master                                 row-view
-----                                 ------                                 --------
eq StringViewArray StringViewArray    1.01     28.1±0.33ms        ? ?/sec    1.00     27.8±0.23ms        ? ?/sec
eq scalar StringViewArray             1.01     19.8±0.19ms        ? ?/sec    1.00     19.6±0.17ms        ? ?/sec
lt scalar StringViewArray                                                    1.00     37.7±0.30ms        ? ?/sec

[alamb]

Thank you @XiangpengHao and @tustvold - I think this one looks good to me

Really nice!

[alamb]

nice

[alamb]

I agree. However, I think this is a good first implementation think we can do it as a follow on PR: #5945

[alamb]

Thanks again @XiangpengHao and @tustvold - I double checked and it seems to me all the feedback has been addressed so merging this PR in