GH-48866: [C++][Gandiva] Truncate subseconds beyond milliseconds in `castTIMESTAMP_utf8` and `castTIME_utf8` #48867

akravchukdremio · 2026-01-15T13:46:33Z

Rationale for this change

Fixes #48866. The Gandiva precompiled time functions castTIMESTAMP_utf8 and castTIME_utf8 currently reject timestamp and time string literals with more than 3 subsecond digits (beyond millisecond precision), throwing an "Invalid millis" error. This behavior is inconsistent with other implementations.

What changes are included in this PR?

Fixed castTIMESTAMP_utf8 and castTIME_utf8 functions to truncate subseconds beyond 3 digits instead of throwing an error
Updated tests. Replaced error-expecting tests with truncation verification tests and added edge cases

Are these changes tested?

Yes

Are there any user-facing changes?

No

GitHub Issue: [C++][Gandiva] castTIMESTAMP_utf8 and castTIME_utf8 should truncate subseconds beyond milliseconds, not reject them #48866

github-actions · 2026-01-15T13:47:37Z

⚠️ GitHub issue #48866 has been automatically assigned in GitHub to PR creator.

xxlaykxx · 2026-01-21T18:31:13Z

@kou Could you plz take a look?

kou · 2026-01-22T01:58:15Z

cpp/src/gandiva/precompiled/time.cc

+    // Truncate to 3 digits (milliseconds precision) if more digits are provided
+    while (sub_seconds_len > 3) {
+      ts_fields[TimeFields::kSubSeconds] /= 10;
+      sub_seconds_len--;
    }


Can we factor out this common code?

Thanks for review, moved common code in the separate inline function

kou · 2026-01-22T01:59:02Z

@xxlaykxx Could you also review this? We don't have enough maintainers for Gandiva.

kou · 2026-01-22T01:59:13Z

@akravchukdremio Could you rebase on main to fix CI failures?

akravchukdremio · 2026-01-22T17:05:04Z

@akravchukdremio Could you rebase on main to fix CI failures?

Sure, rebased PR

kou · 2026-01-25T07:45:57Z

@lriggs Could you review this?

kou · 2026-01-26T00:38:43Z

cpp/src/gandiva/precompiled/time.cc


+// Normalize sub-seconds value to milliseconds precision (3 digits).
+// Truncates if more than 3 digits are provided, pads with zeros if fewer than 3 digits
+FORCE_INLINE


Can we use ARROW_FORCE_INLINE instead?

kou · 2026-01-26T00:40:33Z

cpp/src/gandiva/precompiled/time.cc

+// Normalize sub-seconds value to milliseconds precision (3 digits).
+// Truncates if more than 3 digits are provided, pads with zeros if fewer than 3 digits
+FORCE_INLINE
+int32_t normalize_subseconds_to_millis(int32_t subseconds, int32_t num_digits) {


Can we define this in anonymous namespace? (If we can't use anonymous namespace here, can we use static to hide this symbol?)

Sure, make sense. We couldn't define here anonymous namespace since this file is put under extern "C", so I've used static

akravchukdremio · 2026-01-26T15:33:59Z

Fixed such error:

/arrow/cpp/src/gandiva/precompiled/time.cc:573:16: error: 'always_inline' function might not be inlinable unless also declared 'inline' [-Werror=attributes]
  573 | static int32_t normalize_subseconds_to_millis(int32_t subseconds, int32_t num_digits) {
      |                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors

by adding inline keyword for normalize_subseconds_to_millis function in this commit: d233f19

lriggs · 2026-01-26T19:21:35Z

cpp/src/gandiva/precompiled/time.cc


+// Normalize sub-seconds value to milliseconds precision (3 digits).
+// Truncates if more than 3 digits are provided, pads with zeros if fewer than 3 digits
+ARROW_FORCE_INLINE


I might use the regular 'inline'. I don't see ARROW_FORCE_INLINE really being used anywhere and I don't feel like I know enough that this function should be forced inline.

I see that 'FORCE_INLINE' is used a lot, so maybe that is ok.

I don't see ARROW_FORCE_INLINE really being used anywhere

That's right, ARROW_FORCE_INLINE is used only in this file: https://github.com/akravchukdremio/arrow/blob/GH-48866/cpp/src/arrow/compute/kernels/gather_internal.h#L55. Initially I've used FORCE_INLINE, which is used in many places as far as I can see (around 10 files in gandiva), but then I've changed it to ARROW_FORCE_INLINE to address this comment from committer: #48867 (comment).

I don't feel like I know enough that this function should be forced inline

This is just a helper function, initially I've done my changes directly in castTIMESTAMP_utf8 and castTIME_utf8 functions: b377b59, but then there was a comment to factor out a common code: #48867 (comment) - so I decided to create small function for that.

Why I've used FORCE_INLINE initially? Because there was already a function in this file with such macros, see here: https://github.com/akravchukdremio/arrow/blob/GH-48866/cpp/src/gandiva/precompiled/time.cc#L564-L568, and I've decided to use it too to make our code execution faster (gandiva is aiming for it, AFAIK), since invoking function is an action to put new variables in the call stack. But I agree, that probably just inline keyword would be enough, function is not too complex, so compiler seems anyway will inline it by just having inline keyword.

@kou what do you think, is it fine to just remove ARROW_FORCE_INLINE and use only inline keyword instead?

I see that 'FORCE_INLINE' is used a lot, so maybe that is ok.

It was initially there, then I've changed it to ARROW_FORCE_INLINE to address another review comment: #48867 (comment)

If we don't need force inline, inline is OK.

For FORCE_INLINE and ARROW_FORCE_INLINE: I want to remove FORCE_INLINE and GDV_FORCE_INLINE and use ARROW_FORCE_INLINE instead of them. We don't need multiple similar definitions in the same project.

@kou Got it, thanks for explanation. I've removed ARROW_FORCE_INLINE since we don't need force inline

lriggs · 2026-01-26T19:27:22Z

cpp/src/gandiva/precompiled/time_test.cc

-  EXPECT_EQ(context.get_error(),
-            "Invalid millis for timestamp value 2000-01-01 00:00:00.1000");
-  context.Reset();
+  // Test truncation of subseconds to 3 digits (milliseconds)


Can you add some test cases with less than three digits?

We already have such test cases, see here: https://github.com/akravchukdremio/arrow/blob/GH-48866/cpp/src/gandiva/precompiled/time_test.cc#L100-L104 and https://github.com/akravchukdremio/arrow/blob/GH-48866/cpp/src/gandiva/precompiled/time_test.cc#L154-L158. Seems like enough, but if you think we need more - I can add them

lriggs · 2026-01-27T17:14:52Z

Looks good.

…s in `castTIMESTAMP_utf8` and `castTIME_utf8`

…ds_to_millis` by static keywoard

… needed there

github-actions bot added the awaiting review Awaiting review label Jan 15, 2026

github-actions bot added Component: C++ Component: Gandiva labels Jan 15, 2026

kou reviewed Jan 22, 2026

View reviewed changes

akravchukdremio force-pushed the GH-48866 branch from 5aac113 to 3d7ef89 Compare January 22, 2026 17:03

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jan 23, 2026

akravchukdremio force-pushed the GH-48866 branch from 3d7ef89 to b818113 Compare January 23, 2026 16:08

xxlaykxx approved these changes Jan 25, 2026

View reviewed changes

kou reviewed Jan 26, 2026

View reviewed changes

akravchukdremio force-pushed the GH-48866 branch 3 times, most recently from aa201ec to d233f19 Compare January 26, 2026 14:52

lriggs reviewed Jan 26, 2026

View reviewed changes

akravchukdremio added 5 commits January 27, 2026 19:15

apacheGH-48866: [C++][Gandiva] Truncate subseconds beyond millisecond…

c52fb1c

…s in `castTIMESTAMP_utf8` and `castTIME_utf8`

apacheGH-48866: Address comment to factor out common code

601f7dd

apacheGH-48866: Fix code formatting

7029372

apacheGH-48866: Use ARROW_FORCE_INLINE and hide `normalize_subsecon…

e57a00d

…ds_to_millis` by static keywoard

apacheGH-48866: Add inline keywoard to fix the warning

05775b5

akravchukdremio force-pushed the GH-48866 branch from d233f19 to 0c431a3 Compare January 27, 2026 17:16

apacheGH-48866: Remove ARROW_FORCE_INLINE since force inline is not…

58c988d

… needed there

akravchukdremio force-pushed the GH-48866 branch from 0c431a3 to 58c988d Compare January 27, 2026 17:18

GH-48866: [C++][Gandiva] Truncate subseconds beyond milliseconds in castTIMESTAMP_utf8 and castTIME_utf8 #48867

Are you sure you want to change the base?

GH-48866: [C++][Gandiva] Truncate subseconds beyond milliseconds in castTIMESTAMP_utf8 and castTIME_utf8 #48867

Conversation

akravchukdremio commented Jan 15, 2026 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions bot commented Jan 15, 2026

Uh oh!

xxlaykxx commented Jan 21, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kou commented Jan 22, 2026

Uh oh!

kou commented Jan 22, 2026

Uh oh!

akravchukdremio commented Jan 22, 2026

Uh oh!

kou commented Jan 25, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akravchukdremio commented Jan 26, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lriggs commented Jan 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

GH-48866: [C++][Gandiva] Truncate subseconds beyond milliseconds in `castTIMESTAMP_utf8` and `castTIME_utf8` #48867

GH-48866: [C++][Gandiva] Truncate subseconds beyond milliseconds in `castTIMESTAMP_utf8` and `castTIME_utf8` #48867

akravchukdremio commented Jan 15, 2026 •

edited by github-actions bot

Loading