chore: Add benches for datetime funcs - #5767
Conversation
|
Flexible in terms of consolidating all the benches into a single file as well |
sunchao
left a comment
There was a problem hiding this comment.
Correctness
Reviewed head 921623f0eb121322d03f67d65d985bfd68fd5407 against base 7f1e00189b1ed86f1cb5acd872d97fce694482b1. No verified correctness issue requiring a change before merge. The patch adds ten benchmark targets and three typed input builders; production datetime implementations, Spark serialization and dependency versions are unchanged.
The fixtures use the types required by the existing native implementations: Date32 for date operations, microsecond timestamps for extraction, Decimal128(16,6) seconds for make_time, and six Int32 components plus Float64 seconds for make_interval. The extraction cases distinguish UTC-tagged timestamps converted to America/Los_Angeles from TimestampNTZ inputs. The interval fixture matches the existing native adapter, including its documented compatibility restrictions; it does not establish parity with Spark's default execution path.
The inspected benchmark check compiled the benches and linted all targets. Rust CI passed 1,183 tests with four skipped. Both logs identify merge checkout 23b9d4b4, whose parents are this base/head and whose source tree equals the reviewed head. Local formatting and diff checks passed. No local benchmark or Spark/JNI execution was performed; the CI jobs above did not run the 171 benchmark cases. Canonical Spark context was checked on maintained 3.5/4.0 branches; maintained 3.4/4.1 sources were unavailable.
Performance
This supplies 171 synthetic cases across three batch sizes and three null ratios, with extra variants for date parts, timestamp timezone metadata and numeric input types. Input arrays and UDF instances are constructed outside the timed loop. The UDF cases time invocation together with argument cloning, return-field/configuration construction and output lifecycle, so their results include wrapper overhead. In particular, date_from_unix_date uses an existing zero-copy cast; its batch-size labels do not imply row-by-row computation.
The PR provides no timing table, and the inspected CI establishes compilation and existing-test success only. The fixtures cover ordinary valid values and correlated nulls; they do not measure scalar, error, overflow or fractional-input workloads. These limits are consistent with adding initial native benchmarks, but they provide no evidence of a Spark performance gain.
Design
The change fills a benchmark-coverage gap by exposing existing datetime entry points through independently selectable Criterion targets. Each target keeps its input generation and return type visible, while the existing shared row-count/null-ratio matrix gives repeatable comparisons. Grouping day/month names and hour/minute/second extraction reflects their shared implementations. No production behavior, fallback policy or default setting changes.
Abstraction & complexity
The new Float32, Int32 and Date32 builders follow the existing small array-helper pattern. The benchmark bodies remain straightforward, including the decimal construction needed by make_time; there is no new general runner or dispatch framework. Keeping the targets separate is reasonable despite the author's offer to consolidate them, and I found no abstraction change needed before merge.
Which issue does this PR close?
Part of #5396
Rationale for this change
Adding comprehensive benches
What changes are included in this PR?
All comet custom implemented date time related benches
How are these changes tested?
Local compilation , format, cargo check and runs