scovich commented on PR #7871: URL: https://github.com/apache/arrow-rs/pull/7871#issuecomment-3038845753
Very very nice catch! A couple missed details and some seeming misses by fmt, but the approach LGTM! > I wonder if these optimizations show up in any benchmark results 🤔 Oh my, yes. From https://github.com/apache/arrow-rs/issues/7869: > I added several timestamp markers (`std::time::Instant::elapsed`) to the `test_json_to_variant_object_very_large` test and ran with `cargo test --release`: > |elapsed time (ms)|delta (ms) | finished step| > |-:|-:|-| > 5 | 5 |generate data structures and json string > 250 | 245 | `json_to_variant` > 593 | 343 | `Variant::try_new` > 1758 | 1165 | `variant_to_json_string` > 1839 | 81 | build variant directly > 2171 | 332 | `Variant::try_new` > 2758 | 587 | `JsonToVariantTest::run` The PR currently only fixes two of the three places that wrongly trigger full validation, but with all three fixed locally it becomes: |elapsed time (ms)|delta (ms) | finished step| |-:|-:|-| 5 | 5 |generate data structures and json string 269 | 264 | `json_to_variant` 270 | 0.3 (!!) | `Variant::try_new` 637 | 367 | `variant_to_json_string` 719 | 82 | build variant directly 719 | 0.1 (!!) | `Variant::try_new` 973 | 254 (!) | `JsonToVariantTest::run` (I'm reasonably confident we can stop wondering if we caught them all, when the result drops to sub-ms times) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
