akravchukdremio commented on code in PR #48867:
URL: https://github.com/apache/arrow/pull/48867#discussion_r2729079752
##########
cpp/src/gandiva/precompiled/time.cc:
##########
@@ -566,6 +567,28 @@ bool is_valid_time(const int hours, const int minutes,
const int seconds) {
seconds < 60;
}
+// 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
Review Comment:
> 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:
https://github.com/apache/arrow/pull/48867#discussion_r2726066109.
> 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:
https://github.com/apache/arrow/pull/48867/changes/b377b5911192173d0f224006175aa4e45b0dfabf,
but then there was a comment to factor out a common code:
https://github.com/apache/arrow/pull/48867#discussion_r2715024569 - 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
