Fix missing trailing comma in json writer

[karthikeyann]

Description

Fix missing trailing comma in json writer for non-Lines json format.
Updates default rows_per_chunk because 8 is too small and writer is slow.
closes #12687

Checklist

• I am familiar with the Contributing Guidelines.
• New or existing tests cover these changes.
• The documentation is up to date with these changes.

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (branch-23.04@bad94b9). Click here to learn what that means.
Patch has no changes to coverable lines.

❗ Current head d16da78 differs from pull request most recent head 28ede56. Consider uploading reports for the commit 28ede56 to get more accurate results

Additional details and impacted files

@@               Coverage Diff               @@
##             branch-23.04   #12688   +/-   ##
===============================================
  Coverage                ?   85.81%           
===============================================
  Files                   ?      158           
  Lines                   ?    25153           
  Branches                ?        0           
===============================================
  Hits                    ?    21586           
  Misses                  ?     3567           
  Partials                ?        0           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[bdice]

Looks good. Do you have performance data for different chunk sizes?

[karthikeyann]

[bdice]

@karthikeyann Is there a reason we don't use a larger chunk size? It appears that chunk size 512k performance is better than chunk size 256k. Are there other constraints like memory usage that balance against runtime performance? (Are "chunks" needed at all?)

[karthikeyann]

@bdice Yes. Memory usage is a major concern. chunk is not neeed. If we don't pass chunk size, it will convert entire table into string and will incur high memory usage (and may fail). Just to be safe, I used 256K.
If we want to use rows_per_chunk effectively, we should measure free memory and estimate the chunk size based on free memory, number of columns (child columns and non-child). It is still an approximation.

For example: In these perf measurements, I used a 2 column dataframe.
without chunksize, >128M rows fail, >2GB json fail.
For 256K chunksize, >512M rows fail, >8GB json fail.

Using 512K would be good too. I am trying to keep the rows_per_chunk small so that if number of columns are high, it will not exceed memory limit.

[bdice]

Interesting! Let's make sure that there is guidance in the docs about this: rows_per_chunk should be decreased if writes fail due to running out of memory.

[bdice]

See also this related request for CSV writing: #12690 (comment)

[ttnghia]

So this was unused before?

[karthikeyann]

It was used before too. Instead of passing std::string as argument, now string_scalar is passed.

[karthikeyann]

/merge