JSON Reader Faster Coercion of Primitives to String

[alamb]

Is your feature request related to a problem or challenge? Please describe what you are trying to do.
While reviewing #7263 from @zhuqi-lucas I noticed that the json reader is allocating a new for each string field it parses

arrow-rs/arrow-json/src/reader/string_array.rs

Lines 110 to 120 in f4fde76

	TapeElement::I32(n) if coerce_primitive => {
	builder.append_value(n.to_string());
	}
	TapeElement::F32(n) if coerce_primitive => {
	builder.append_value(n.to_string());
	}
	TapeElement::F64(high) if coerce_primitive => match tape.get(p + 1) {
	TapeElement::F32(low) => {
	let val = f64::from_bits(((high as u64) << 32) | low as u64);
	builder.append_value(val.to_string());
	}

Describe the solution you'd like

I would like to make the json reader faster by not allocating in the inner loop

Describe alternatives you've considered

I think instead of doing

for ... {
  builder.append_value(n.to_string())
}

A typically faster pattern is

// temp buffer
let mut s = String::new();

for ... {
  s.clear(); // reuse allocation
  write!(&mut s, "{n}");
  builder.append_value(s)
}

Additional context

[alamb]

You can run the json reader benchmark here:

cargo bench --bench json_reader

and compare before/after performance

Since this is well specified I think this would be a good first issue for anyone who wants to learn about low level rust optimization

[alamb]

BTW it may turn out that the suggested change doesn't improve performance -- but we can measure with the benchmark

[tustvold]

I believe this codepath is only hit when coercing integer to strings, e.g.

{"a": 123}

Reading into a StringArray column, anything encoded as a string will be unaffected. E.g.

{"a": "123"}

So any benchmark would need to be doing this in order to see improvement

It is also worth noting that parsing a primitive only to cast it back to a string is inherently wasteful, but avoiding this would be extremely difficult as the tape decoder is unaware of the schema

[ndemir]

Hi,

I realized I can send a PR for that and I implemented and sent it: #7274

[ndemir]

closing the PR.
I see that the performance is consistently BETTER even though it is not hitting the refactored code.
Will investigate.

[ndemir]

I found the issue.
The following code path is executed during serialization.

 TapeElement::I32(n) if coerce_primitive => { 
     builder.append_value(n.to_string()); 
 } 
 TapeElement::F32(n) if coerce_primitive => { 
     builder.append_value(n.to_string()); 
 } 
 TapeElement::F64(high) if coerce_primitive => match tape.get(p + 1) { 
     TapeElement::F32(low) => { 
         let val = f64::from_bits(((high as u64) << 32) | low as u64); 
         builder.append_value(val.to_string()); 
     } 

And, the following is executed during reading.

                TapeElement::Number(idx) if coerce_primitive => {
                    builder.append_value(tape.get_string(idx));
                }

I do not see a reason to optimize the previous code block (TapeElement::Number(idx) if coerce_primitive => {) because it is already appending as string as it is coming from Tape.

But, with some quick tests, I saw that we may take the advantage of new approach during serialization.
Will write some tests to confirm.

[ndemir]

I am not sending a PR because the performance did not improve.
Here is the diff you can see.

Here is what I did:

• I implemented the changes.
• I implemented a new test function in json_writer.rs, the new test function name is bench_coerce_primitive.
• I did benchmark: main branch vs that new branch.

The results are not good.

bench_coerce_primitive  time:   [8.5429 ms 8.6018 ms 8.6619 ms]
                        change: [+3.7518% +4.8659% +5.9167%] (p = 0.00 < 0.05)
                        Performance has regressed.

That also made me think that if other similar changes are really statistically significant.

[alamb]

Thanks for trying @ndemir