xiangfu0 commented on PR #19223: URL: https://github.com/apache/pinot/pull/19223#issuecomment-5273911749
Addressed the latest review feedback in f0f1036c99. Key changes: - Replaced per-thread Fory parsers with one process-wide, fixed-size Fory pool; only the small path context remains thread-local. - Replaced exception-driven container fallback with a sentinel and routes known Fory-ineligible transform types to Jayway once during initialization. - Restored Fast and FirstMatch routing for BYTES plus BIG_DECIMAL. - Aligned Fory depth with Jackson constraints, removed the dead lexical prescan, and counts configured token limits during the streaming walk. - Made Fory optional and excluded it from the Pinot Common shaded JAR and default runtime dependency graph. Applications must add Fory to the application classpath to activate the experimental path. - Added direct-path, sentinel, depth, token-limit, missing-runtime, concurrency, routing, and fallback benchmarks/tests. Directional paired JMH rerun on JDK 25.0.4 with GC profiling. Scalar ratios are T1; production ValueBlock transform ratios are T32. Each cell is Fory/Fast and Fory/normal-Jayway: | Surface | Type | 700 B | 8 KiB | 64 KiB | |---|---:|---:|---:|---:| | jsonPath scalar | STRING | 1.21x / 4.89x | 2.66x / 3.71x | 4.41x / 9.86x | | jsonPath scalar | LONG | 1.24x / 3.44x | 2.46x / 3.25x | 3.70x / 9.47x | | jsonPath scalar | DOUBLE | 1.35x / 3.54x | 3.17x / 4.60x | 4.68x / 10.94x | | jsonExtractScalar ValueBlock | LONG | 1.15x / 1.70x | 2.24x / 2.71x | 3.62x / 9.97x | | jsonExtractScalar ValueBlock | DOUBLE | 1.32x / 2.16x | 1.51x / 2.40x | 4.13x / 10.58x | The new direct depth cases no longer fall back: depth-selected and depth-unrelated reached 13.9M and 17.1M ops/s at about 48 B/op. The old implementation measured 0.77M and 1.59M ops/s with about 16.0 KiB and 10.7 KiB/op. Important caveat: Fory 1.6 still materializes skipped strings because it has no public allocation-free fast-skip API. Fory allocates roughly 1.1-1.3 KiB, 8.6-8.8 KiB, and 66 KiB per 700 B, 8 KiB, and 64 KiB row, versus roughly 0.7-0.96 KiB for Fast. This remains the reason to keep support experimental and optional. The one-fork T32 confidence intervals are wide, so these are directional results rather than statistically precise claims. Validation: - Fory common tests: 27/27 passed. - JsonExtractScalarTransformFunctionTest: 246/246 passed. - JsonPathTest integration suite: 48/48 passed. - Spotless, license format/check, and checkstyle passed for all affected modules. - Pinot Common shaded JAR contains no Fory classes; Pinot Core has no runtime Fory dependency. - UdfTest did not reach its snapshot assertion because current main has an unrelated NotUdf reflection mismatch: NotUdf requests LogicalFunctions.not(boolean), while the source exposes not(Boolean). No unrelated UDF change is included here. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
