Repository navigation
DX-126900: [C++][Gandiva] Add TimeIR for unit-aware time64[us/ns] support - #146
Open
akravchukdremio wants to merge 1 commit into
Open
akravchukdremio wants to merge 1 commit into
akravchukdremio wants to merge 1 commit into
Conversation
…port Register time64 function signatures (extractHour/Minute/Second, comparisons, isnull/isnotnull, is_distinct_from/is_not_distinct_from) with time64[us]. Signature matching ignores the time64 unit, and LLVMGenerator::ResolveTimePcName() remaps time64[ns] calls to the _ns precompiled variants, mirroring TimestampIR. Unlike timestamp, a time64[ns] call without an _ns variant is rejected instead of falling through to the microsecond function, and mixed time64 units are rejected. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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
1 participant
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.
Rationale for this change
Dremio now produces native Arrow
time64[us]/time64[ns]columns forTIME(6)/TIME(9)(epic DX-122844). Gandiva registered no time64 functions, so every such expression fell back to Java. This adds native time64 support the same way TimestampIR (#137, #138) added timestamp[us/ns] support. Jira: https://dremio.atlassian.net/browse/DX-126900What changes are included in this PR?
time64[us]:extractHour/Minute/Second(aliaseshour/minute/second)equal(eq,same),not_equal,less_than,less_than_or_equal_to,greater_than,greater_than_or_equal_toisnull,isnotnull,is_distinct_from,is_not_distinct_fromfunction_signature.cc: the time64 unit is not part of the signature (like timestamp), sotime64[ns]matches the same entry.time_ir.h/.cc+LLVMGenerator::ResolveTimePcName(): remapstime64[ns]calls to_nsvariants in the newprecompiled/time_unit_ops.cc. Comparisons and null checks are unit-agnostic and need no variant.time64[ns]call without an_nsvariant is a compile error instead of silently running the microsecond function (the failure mode behind thenext_daybug in DX-105463: [C++][Gandiva] Add TimestampIR wrapper for next_day #135).Are these changes tested?
Yes.
precompiled/time_test.cc: raw_time64/_time64_nsextract functions, including boundary values.tests/date_time_test.cc, end to end through Projector for both units:llvm_generator_test.cc:ResolveTimePcNamecases, plusVerifyTime64Functions, which checks every registered time64 function resolves to an existing precompiled function for both units.function_signature_test.cc: time64[us] and time64[ns] signatures compare equal and hash equally.Locally: gandiva-precompiled-test (133), gandiva-internals-test (197) and gandiva-projector-test (256, with
TZ=UTC) all pass.Are there any user-facing changes?
New time64 functions in the Gandiva registry. Dremio routes
TIME(6)/TIME(9)expressions to them via its pushdown sieve (DX-124702).Requires the companion arrow-java fix that reports time64 types as
TIME64in the expression registry (dremio/arrow-java#35).🤖 Generated with Claude Code