Preserve millisecond precision for TIMESTAMP comparisons in the multi-stage engine#18883
Merged
yashmayya merged 2 commits intoJun 29, 2026
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #18883 +/- ##
=============================================
+ Coverage 37.16% 64.81% +27.64%
- Complexity 1321 1322 +1
=============================================
Files 3393 3393
Lines 211305 211336 +31
Branches 33226 33235 +9
=============================================
+ Hits 78541 136980 +58439
+ Misses 125557 63296 -62261
- Partials 7207 11060 +3853
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…recision, matching the single-stage engine
xiangfu0
approved these changes
Jun 29, 2026
xiangfu0
left a comment
Contributor
There was a problem hiding this comment.
Core fix looks right to me. The planner now uses millisecond TIMESTAMP precision, preserving sub-second literals while keeping the TIMESTAMP column unwrapped, and the regression coverage exercises both engines. CI is green.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
In the multi-stage engine, comparing a
TIMESTAMPcolumn to a sub-second epoch-millis literal (e.g.WHERE ts = 1761667561482) silently returned no rows. This is a regression surfaced by #18396 and tracked in #18881. The single-stage engine is unaffected (it treatsTIMESTAMPas a plainlong).Root cause
#18396 changed
PinotTypeCoercion#binaryComparisonCoercionso that fortsColumn <op> bigintLiteral, the implicit cast is placed on the literal (tsColumn <op> CAST(literal AS TIMESTAMP)) instead of on the column, to keep the column unwrapped for index applicability. The cast target was built viafactory.createSqlType(SqlTypeName.TIMESTAMP)with no precision.TypeSystem.getDefaultPrecision(...)only overrodeDECIMAL, soTIMESTAMPfell through to Calcite's default precision of 0 (whole seconds). When the literal cast is constant-folded,RexBuilder.cleanrounds theTimestampStringto the type precision viaTimestampString.round(0), truncating the sub-second component:The stored value is
...561482; the folded literal is...561000, so the predicate never matches.Fix
Override
TypeSystem.getDefaultPrecision(TIMESTAMP)to return 3 (millisecond precision), matching Pinot's epoch-millisLONGstorage.3is also Calcite'sMAX_DATETIME_PRECISION, so it is simultaneously the default and the max forTIMESTAMP— no clamping. This keeps the column unwrapped (preserving the #18396 improvement) and preserves milliseconds on the folded literal.The precision is planner-only metadata and is not on the wire: the broker→server boundary maps
SqlTypeName.TIMESTAMPtoColumnDataType.TIMESTAMP(backed byLONG, no precision), TIMESTAMP literals are serialized to the server as raw epoch-millis, and no execution path reads the SQL precision — so the change is rolling-upgrade safe and does not alter results beyond the intended millisecond preservation.Tests
PinotTypeCoercionTest#testTimestampColumnVsSubSecondLiteralPreservesMillis— planner-level guard that the folded literal keeps its millis. Fails without the fix (literal truncates to whole seconds).TimestampTest#testSubSecondTimestampEqualityQueries— end-to-end on both engines: a row with a sub-secondTIMESTAMPis matched byWHERE tsCol = <epoch-millis>. On the multi-stage engine this returned 0 rows before the fix (expected [1] but found [0]).LiteralEvaluationPlans.jsonand twoPinotTypeCoercionTestassertions for theTIMESTAMP(0)→TIMESTAMP(3)plan rendering (no behavioral change; the folded epoch values are identical for whole-second literals).Backward compatibility
No wire-format or segment-format change. Affects only multi-stage planner-emitted
TIMESTAMPtypes. After upgrade, multi-stage queries comparing aTIMESTAMPcolumn to a sub-second epoch-millis literal return the correct rows.Related behavior change: because the precision is now millisecond, multi-stage queries that fold a
TIMESTAMP-returning function to a sub-second value (e.g.CAST(now() AS BIGINT)) now preserve milliseconds instead of truncating to whole seconds — which is what the single-stage engine already did (now()is alongthere). ThetestLiteralOnlyFunc/testLiteralOnlyFuncV2integration-test assertions are updated to match the single-stage bounds.Closes #18881