feat: Support Utf8View in JSON reader by zhuqi-lucas · Pull Request #7263 · apache/arrow-rs

zhuqi-lucas · 2025-03-11T06:41:44Z

Which issue does this PR close?

Closes #7244

Rationale for this change

Support Utf8View in JSON reader

What changes are included in this PR?

Support Utf8View in JSON reader

Are there any user-facing changes?

Support Utf8View in JSON reader

alamb

Thank you very much @zhuqi-lucas!

I think this PR just needs

some adjustment on data allocation
A performance benchmark. Perhaps you could extend this one:

https://github.com/apache/arrow-rs/blob/a75da00eed762f8ab201c6cb4388921ad9b67e7e/arrow/benches/json_reader.rs#L45-L44

alamb · 2025-03-11T17:23:55Z

+        assert_eq!(col1.null_count(), 2);
+        assert_eq!(col1.value(0), "1");
+        assert_eq!(col1.value(1), "hello");
+        assert_eq!(col1.value(2), "\nfoobar😀asfgÿ");


this value has more than 12 bytes so it will exercise the longer string view path

alamb · 2025-03-11T17:24:18Z

+                TapeElement::String(idx) => {
+                    data_capacity += tape.get_string(idx).len();
+                }
+                TapeElement::Null => { /* 不增加容量 */ }


would it be possible to use english in the comments?

alamb · 2025-03-11T17:30:16Z

+    fn decode(&mut self, tape: &Tape<'_>, pos: &[u32]) -> Result<ArrayData, ArrowError> {
+        let coerce = self.coerce_primitive;
+        let mut data_capacity = 0;
+        for &p in pos {


StringView is different that StringArray in that only "long" strings (longer than 12 bytes) contributed to the data

Thus I think these calculations should be adjusted:

only increase capacity for strings if the data is over 12 bytes

don't increase for boolean

For I32 probably we can use zero as well (as the longest such value is -2147483647 whcih is less than 12 bytes)

For I64 maybe we could be more sophisticated and only add data length if the value is over 999999999999 etc.

For F32 and F64 I am not sure what hte maximum length of a string representation is so we should probably keep the existing estimate

More details on the layout are here: https://docs.rs/arrow/latest/arrow/array/struct.GenericByteViewArray.html

Good explain and guide, thank you @alamb!

alamb · 2025-03-11T17:31:55Z

+                    _ => unreachable!(),
+                },
+                TapeElement::I32(n) if coerce => {
+                    builder.append_value(n.to_string());


This allocation is quite unfortunate (to_string() allocates a string) but I see this is consistent with the other JSON string implementation:

https://github.com/apache/arrow-rs/blob/a0c3186c55ac8ed3f6b8a15d1305548fd6305ebb/arrow-json/src/reader/string_array.rs#L111-L110

alamb · 2025-03-11T17:35:31Z

(BTW the MSRV test is fixed on main thanks to @tustvold -- you should be able to merge up from main and the CI will pass)

zhuqi-lucas · 2025-03-12T10:14:07Z

Thank you very much @zhuqi-lucas!

I think this PR just needs

some adjustment on data allocation

A performance benchmark. Perhaps you could extend this one:

https://github.com/apache/arrow-rs/blob/a75da00eed762f8ab201c6cb4388921ad9b67e7e/arrow/benches/json_reader.rs#L45-L44

Thank you @alamb for review, addressed the comments now.

And also the benchmark result for utf8view seems better:

small_bench_primitive   time:   [6.2293 µs 6.2540 µs 6.2810 µs]
                        change: [+0.6034% +1.1278% +1.6922%] (p = 0.00 < 0.05)
                        Change within noise threshold.
Found 11 outliers among 100 measurements (11.00%)
  1 (1.00%) low mild
  7 (7.00%) high mild
  3 (3.00%) high severe

small_bench_primitive_with_utf8view
                        time:   [6.0220 µs 6.0420 µs 6.0649 µs]
Found 3 outliers among 100 measurements (3.00%)
  1 (1.00%) low mild
  2 (2.00%) high mild

alamb

Thank you very much @zhuqi-lucas -- I think this is a really nice contribution.

I left some small code comment suggestions but I can also add them as a follow on PR as well

Thanks again!

alamb · 2025-03-12T20:48:29Z

+        let coerce = self.coerce_primitive;
+        let mut data_capacity = 0;
+        for &p in pos {
+            match tape.get(p) {


I think a little context on the rationale here would be helpful:

Suggested change

match tape.get(p) {

// note that StringView is different that StringArray in that only

// "long" strings (longer than 12 bytes) are stored in the buffer.

// "short" strings are inlined into a fixed length structure.

match tape.get(p) {

alamb · 2025-03-12T20:48:51Z

+                        data_capacity += s.len();
+                    }
+                }
+                // For I64, only add capacity if the absolute value is greater than 999,999,999,999


Suggested change

// For I64, only add capacity if the absolute value is greater than 999,999,999,999

// For I64, only add capacity if the absolute value is greater than 999,999,999,999

// (the largest number that can fit in 12 bytes)

alamb · 2025-03-12T20:49:38Z

+                TapeElement::Null => {
+                    // Do not increase capacity for null values
+                }
+                // For booleans, do not increase capacity


Suggested change

// For booleans, do not increase capacity

// For booleans, do not increase capacity (both "true" and "false" are less than

// 12 bytes)

alamb · 2025-03-12T20:50:23Z

+                    TapeElement::I32(low) => {
+                        let val = ((high as i64) << 32) | (low as u32) as i64;
+                        tmp_buf.clear();
+                        // Reuse the temporary buffer instead of allocating a new String


zhuqi-lucas · 2025-03-13T04:51:52Z

Thank you very much @zhuqi-lucas -- I think this is a really nice contribution.

I left some small code comment suggestions but I can also add them as a follow on PR as well

Thanks again!

Thank you @alamb for review! I also addressed the suggestions in latest PR, thanks!

alamb · 2025-03-13T10:40:37Z

Thanks again @zhuqi-lucas

feat: Support Utf8View in JSON reader

d1898b8

github-actions Bot added the arrow Changes to the arrow crate label Mar 11, 2025

zhuqi-lucas added 2 commits March 11, 2025 14:47

Add code

ef211a5

Fix fmt

b2ed990

alamb reviewed Mar 11, 2025

View reviewed changes

alamb mentioned this pull request Mar 11, 2025

JSON Reader Faster Coercion of Primitives to String #7273

Closed

zhuqi-lucas added 4 commits March 12, 2025 17:40

Address comments

9470010

Merge remote-tracking branch 'upstream/main' into issue_7244

b1af0b0

Add benchmark

6013718

Add benchmark

c851c61

Fix lint

460a19e

zhuqi-lucas requested a review from alamb March 12, 2025 10:16

alamb approved these changes Mar 12, 2025

View reviewed changes

alamb mentioned this pull request Mar 12, 2025

Optimize numeric-to-string coercion in StringArrayDecoder #7274

Closed

Clean up comments

753524b

alamb merged commit c26e427 into apache:main Mar 13, 2025

alamb mentioned this pull request Mar 14, 2025

feat: Support Utf8View in JSON reader #7244

Closed

alamb added parquet Changes to the parquet crate arrow Changes to the arrow crate and removed arrow Changes to the arrow crate parquet Changes to the parquet crate labels Mar 15, 2025

-            match tape.get(p) {
+            // note that StringView is different that StringArray in that only
+            // "long" strings (longer than 12 bytes) are stored in the buffer.
+            // "short" strings are inlined into a fixed length structure.
+            match tape.get(p) {

	// For I64, only add capacity if the absolute value is greater than 999,999,999,999
	// For I64, only add capacity if the absolute value is greater than 999,999,999,999
	// (the largest number that can fit in 12 bytes)

	// For booleans, do not increase capacity
	// For booleans, do not increase capacity (both "true" and "false" are less than
	// 12 bytes)

Conversation

zhuqi-lucas commented Mar 11, 2025

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Uh oh!

alamb 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented Mar 11, 2025

Uh oh!

zhuqi-lucas commented Mar 12, 2025

Uh oh!

alamb 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!

Choose a reason for hiding this comment

Uh oh!

zhuqi-lucas commented Mar 13, 2025

Uh oh!

alamb commented Mar 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants