Repository navigation
fix: keep fractional seconds in EXTRACT(EPOCH) for Duration - #26676
Conversation
31bfea9 to
e391b4d
Compare
|
run benchmark datetime_expressions env:
BENCH_FILTER: date_part_epoch
baseline:
ref: 353e4cee59f9172d86d284bdc4e08989e4a05192 |
|
run benchmark datetime_expressions env:
BENCH_FILTER: date_part_epoch
baseline:
ref: 353e4cee59f9172d86d284bdc4e08989e4a05192
resources:
arch: amd64 |
|
run benchmark datetime_expressions env:
BENCH_FILTER: date_part_epoch
RUSTFLAGS: "-C llvm-args=-align-all-functions=6 -C llvm-args=-align-all-nofallthru-blocks=6"
baseline:
ref: 353e4cee59f9172d86d284bdc4e08989e4a05192 |
|
run benchmark datetime_expressions env:
BENCH_FILTER: date_part_epoch
RUSTFLAGS: "-C llvm-args=-align-all-functions=6 -C llvm-args=-align-all-nofallthru-blocks=6"
baseline:
ref: 353e4cee59f9172d86d284bdc4e08989e4a05192
resources:
arch: amd64 |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing fix-epoch-duration-fraction (e391b4d) to 353e4ce diff Run configurationrun benchmark datetime_expressions
env:
BENCH_FILTER: "date_part_epoch"
baseline:
ref: "353e4cee59f9172d86d284bdc4e08989e4a05192"Results will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing fix-epoch-duration-fraction (e391b4d) to 353e4ce diff Run configurationrun benchmark datetime_expressions
env:
BENCH_FILTER: "date_part_epoch"
RUSTFLAGS: "-C llvm-args=-align-all-functions=6 -C llvm-args=-align-all-nofallthru-blocks=6"
baseline:
ref: "353e4cee59f9172d86d284bdc4e08989e4a05192"
resources:
arch: "amd64"Results will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing fix-epoch-duration-fraction (e391b4d) to 353e4ce diff Run configurationrun benchmark datetime_expressions
env:
BENCH_FILTER: "date_part_epoch"
baseline:
ref: "353e4cee59f9172d86d284bdc4e08989e4a05192"
resources:
arch: "amd64"Results will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing fix-epoch-duration-fraction (e391b4d) to 353e4ce diff Run configurationrun benchmark datetime_expressions
env:
BENCH_FILTER: "date_part_epoch"
RUSTFLAGS: "-C llvm-args=-align-all-functions=6 -C llvm-args=-align-all-nofallthru-blocks=6"
baseline:
ref: "353e4cee59f9172d86d284bdc4e08989e4a05192"Results will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing fix-epoch-duration-fraction (e391b4d) to 353e4ce diff Run configurationrun benchmark datetime_expressions
env:
BENCH_FILTER: "date_part_epoch"
baseline:
ref: "353e4cee59f9172d86d284bdc4e08989e4a05192"CPU Details (lscpu)Details
Resource Usagedatetime_expressions — base (merge-base)
datetime_expressions — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing fix-epoch-duration-fraction (e391b4d) to 353e4ce diff Run configurationrun benchmark datetime_expressions
env:
BENCH_FILTER: "date_part_epoch"
RUSTFLAGS: "-C llvm-args=-align-all-functions=6 -C llvm-args=-align-all-nofallthru-blocks=6"
baseline:
ref: "353e4cee59f9172d86d284bdc4e08989e4a05192"CPU Details (lscpu)Details
Resource Usagedatetime_expressions — base (merge-base)
datetime_expressions — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing fix-epoch-duration-fraction (e391b4d) to 353e4ce diff Run configurationrun benchmark datetime_expressions
env:
BENCH_FILTER: "date_part_epoch"
baseline:
ref: "353e4cee59f9172d86d284bdc4e08989e4a05192"
resources:
arch: "amd64"CPU Details (lscpu)Details
Resource Usagedatetime_expressions — base (merge-base)
datetime_expressions — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing fix-epoch-duration-fraction (e391b4d) to 353e4ce diff Run configurationrun benchmark datetime_expressions
env:
BENCH_FILTER: "date_part_epoch"
RUSTFLAGS: "-C llvm-args=-align-all-functions=6 -C llvm-args=-align-all-nofallthru-blocks=6"
baseline:
ref: "353e4cee59f9172d86d284bdc4e08989e4a05192"
resources:
arch: "amd64"CPU Details (lscpu)Details
Resource Usagedatetime_expressions — base (merge-base)
datetime_expressions — branch
File an issue against this benchmark runner |
alamb
left a comment
There was a problem hiding this comment.
This is great -- thank you @mbutrovich
It is effectively fixing a bug by deleting code
| Duration(_) => return seconds(array, Second), | ||
| Duration(Second) => as_duration_second_array(array)?.unary(|x| x as f64), | ||
| Duration(Millisecond) => { | ||
| as_duration_millisecond_array(array)?.unary(|x| x as f64 / 1_000_f64) |
There was a problem hiding this comment.
that certainly looks much faster and simpler
Which issue does this PR close?
EXTRACT EPOCHinconsistently extracts fractional seconds #26673.Rationale for this change
The old code was complicated because it reused a helper written for timestamps. #12514 sent
Durationepochs throughsecondsand added% 1_000_000_000and a NULL-as-zero branch to recover the fraction, which works only while the total fits ini32. #19807 fixed the same bug forIntervaland leftDurationon that path.What changes are included in this PR?
The
Durationarms ofepochnow divide thei64value by the unit's factor withunary, like theTimestamparms, andsecondsis gone. The first commit adds two cases to thedatetime_expressionsbench without the fix, so the benchmark bot can compare against it.It is also 2.5x to 12x faster on the benchmark bot.
mainmade three passes over the batch and fell back to a row-by-row iterator on any NULL or large value. The new loop takes the same time whatever the values or NULLs.Numbers and codegen
Speedups over
353e4cee5(the bench commit) on 1000 rows, with the bot's thin LTO. "Nulls" means 10% of rows are NULL. The aligned runs add-align-all-functions=6 -align-all-nofallthru-blocks=6.duration_nsduration_ns_nullsduration_ns_smallns(unchanged)In the default Xeon run the unchanged cases are 1.09x to 1.19x, so that run overstates the gain by about 15%.
Timing each arrow kernel of
main's path separately on 8192 rows on an Apple M5 Max accounts for its totals.date_part(Second)date_part(Nanosecond)binarycollectLLVM merges the new loops with the existing
TimestampandTime64ones, so this PR adds no new loop code. On aarch64 the loop handles 8 rows per iteration withscvtf.2dandfdiv.2d. On x86_64 the division is packed (divpd,vdivpdwith AVX2) and thei64tof64conversion is scalarcvtsi2sd, because the packed form needs AVX-512DQ. I read this withcargo asm(rustc 1.99.0,release-nonlto).The code divides rather than multiplying by
1e-9. The division is most of the loop's time on Neoverse-V2, wheredate_part_epoch_s_1000(no division) takes 412 ns against 1.21 us forns. The multiply would be faster, but it changes 14% ofDuration(ms)and 41% ofDuration(ns)results, for example 12.92335 s becomes12.923350000000001.What is the testing strategy for this PR?
New cases in
datafusion/sqllogictest/test_files/datetime/date_part.sltundertest_extract_durationcover thei32::MAXns boundary, eachDurationunit, the issue's reproducer, negative values, NULLs mixed with values, andi64extremes.Are there any user-facing changes?
EXTRACT(EPOCH)onDurationreturns correct results. There are no public API changes.AI Disclosure
I wrote this PR with help from Claude but understand and support the core approach, implementation, and test coverage.