Skip to content

Commit 7559af6

Browse files
akravchukdremioakravchukdremio
authored
DX-120970 Truncate subseconds beyond milliseconds in castTIMESTAMP_utf8 and castTIME_utf8 (cherry-pick to 26.1) (#140)
* apacheGH-48866: [C++][Gandiva] Truncate subseconds beyond milliseconds in `castTIMESTAMP_utf8` and `castTIME_utf8` (apache#48867) ### Rationale for this change Fixes apache#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: apache#48866 Authored-by: Arkadii Kravchuk <arkadii.kravchuk@dremio.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com> * DX-120970 Fix CI: ubuntu-24.04-arm runner, docker-compose shim, remove brew pin - Replace unavailable buildjet-8vcpu-ubuntu-2204-arm with ubuntu-24.04-arm - Add docker-compose shim: ubuntu-24.04-arm ships only Docker Compose v2 as a plugin (docker compose); archery requires the standalone binary - Remove brew pin cmake/boost: newer Homebrew (runner >=20260525) resolves cmake via the API and hits the cmake cask when pinning; the pins are not needed in CI since no brew upgrade runs after the local tap install - Fix stale comment: remove "Add commentMore actions" GitHub UI artifact --------- Signed-off-by: Sutou Kouhei <kou@clear-code.com>
1 parent 011d73e commit 7559af6

3 files changed

Lines changed: 72 additions & 42 deletions

File tree

‎cpp/src/gandiva/precompiled/time.cc‎

Lines changed: 26 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -564,6 +564,27 @@ bool is_valid_time(const int hours, const int minutes, const int seconds) {
564564
seconds < 60;
565565
}
566566

567+
// Normalize sub-seconds value to milliseconds precision (3 digits).
568+
// Truncates if more than 3 digits are provided, pads with zeros if fewer than 3 digits
569+
static inline int32_t normalize_subseconds_to_millis(int32_t subseconds,
570+
int32_t num_digits) {
571+
if (num_digits <= 0 || num_digits == 3) {
572+
// No need to adjust
573+
return subseconds;
574+
}
575+
// Calculate the power of 10 adjustment needed
576+
int32_t digit_diff = num_digits - 3;
577+
while (digit_diff > 0) {
578+
subseconds /= 10;
579+
digit_diff--;
580+
}
581+
while (digit_diff < 0) {
582+
subseconds *= 10;
583+
digit_diff++;
584+
}
585+
return subseconds;
586+
}
587+
567588
// MONTHS_BETWEEN returns number of months between dates date1 and date2.
568589
// If date1 is later than date2, then the result is positive.
569590
// If date1 is earlier than date2, then the result is negative.
@@ -744,17 +765,8 @@ gdv_timestamp castTIMESTAMP_utf8(int64_t context, const char* input, gdv_int32 l
744765
}
745766

746767
// adjust the milliseconds
747-
if (sub_seconds_len > 0) {
748-
if (sub_seconds_len > 3) {
749-
const char* msg = "Invalid millis for timestamp value ";
750-
set_error_for_date(length, input, msg, context);
751-
return 0;
752-
}
753-
while (sub_seconds_len < 3) {
754-
ts_fields[TimeFields::kSubSeconds] *= 10;
755-
sub_seconds_len++;
756-
}
757-
}
768+
ts_fields[TimeFields::kSubSeconds] =
769+
normalize_subseconds_to_millis(ts_fields[TimeFields::kSubSeconds], sub_seconds_len);
758770
// handle timezone
759771
if (encountered_zone) {
760772
int err = 0;
@@ -864,18 +876,9 @@ gdv_time32 castTIME_utf8(int64_t context, const char* input, int32_t length) {
864876
}
865877

866878
// adjust the milliseconds
867-
if (sub_seconds_len > 0) {
868-
if (sub_seconds_len > 3) {
869-
const char* msg = "Invalid millis for time value ";
870-
set_error_for_date(length, input, msg, context);
871-
return 0;
872-
}
873-
874-
while (sub_seconds_len < 3) {
875-
time_fields[TimeFields::kSubSeconds - TimeFields::kHours] *= 10;
876-
sub_seconds_len++;
877-
}
878-
}
879+
time_fields[TimeFields::kSubSeconds - TimeFields::kHours] =
880+
normalize_subseconds_to_millis(
881+
time_fields[TimeFields::kSubSeconds - TimeFields::kHours], sub_seconds_len);
879882

880883
int32_t input_hours = time_fields[TimeFields::kHours - TimeFields::kHours];
881884
int32_t input_minutes = time_fields[TimeFields::kMinutes - TimeFields::kHours];

‎cpp/src/gandiva/precompiled/time_test.cc‎

Lines changed: 39 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -121,15 +121,26 @@ TEST(TestTime, TestCastTimestamp) {
121121
"Not a valid time for timestamp value 2000-01-01 00:00:100");
122122
context.Reset();
123123

124-
EXPECT_EQ(castTIMESTAMP_utf8(context_ptr, "2000-01-01 00:00:00.0001", 24), 0);
125-
EXPECT_EQ(context.get_error(),
126-
"Invalid millis for timestamp value 2000-01-01 00:00:00.0001");
127-
context.Reset();
128-
129-
EXPECT_EQ(castTIMESTAMP_utf8(context_ptr, "2000-01-01 00:00:00.1000", 24), 0);
130-
EXPECT_EQ(context.get_error(),
131-
"Invalid millis for timestamp value 2000-01-01 00:00:00.1000");
132-
context.Reset();
124+
// Test truncation of subseconds to 3 digits (milliseconds)
125+
// "2000-01-01 00:00:00.0001" should truncate to "2000-01-01 00:00:00.000"
126+
EXPECT_EQ(castTIMESTAMP_utf8(context_ptr, "2000-01-01 00:00:00.0001", 24),
127+
castTIMESTAMP_utf8(context_ptr, "2000-01-01 00:00:00.000", 23));
128+
129+
// "2000-01-01 00:00:00.1000" should truncate to "2000-01-01 00:00:00.100"
130+
EXPECT_EQ(castTIMESTAMP_utf8(context_ptr, "2000-01-01 00:00:00.1000", 24),
131+
castTIMESTAMP_utf8(context_ptr, "2000-01-01 00:00:00.100", 23));
132+
133+
// "2000-01-01 00:00:00.123456789" should truncate to "2000-01-01 00:00:00.123"
134+
EXPECT_EQ(castTIMESTAMP_utf8(context_ptr, "2000-01-01 00:00:00.123456789", 29),
135+
castTIMESTAMP_utf8(context_ptr, "2000-01-01 00:00:00.123", 23));
136+
137+
// "2000-01-01 00:00:00.1999" should truncate to "2000-01-01 00:00:00.199"
138+
EXPECT_EQ(castTIMESTAMP_utf8(context_ptr, "2000-01-01 00:00:00.1999", 24),
139+
castTIMESTAMP_utf8(context_ptr, "2000-01-01 00:00:00.199", 23));
140+
141+
// "2000-01-01 00:00:00.1994" should truncate to "2000-01-01 00:00:00.199"
142+
EXPECT_EQ(castTIMESTAMP_utf8(context_ptr, "2000-01-01 00:00:00.1994", 24),
143+
castTIMESTAMP_utf8(context_ptr, "2000-01-01 00:00:00.199", 23));
133144
}
134145

135146
TEST(TestTime, TestCastTimeUtf8) {
@@ -165,13 +176,26 @@ TEST(TestTime, TestCastTimeUtf8) {
165176
EXPECT_EQ(context.get_error(), "Not a valid time value 00:00:100");
166177
context.Reset();
167178

168-
EXPECT_EQ(castTIME_utf8(context_ptr, "00:00:00.0001", 13), 0);
169-
EXPECT_EQ(context.get_error(), "Invalid millis for time value 00:00:00.0001");
170-
context.Reset();
179+
// Test truncation of subseconds to 3 digits (milliseconds)
180+
// "00:00:00.0001" should truncate to "00:00:00.000"
181+
EXPECT_EQ(castTIME_utf8(context_ptr, "00:00:00.0001", 13),
182+
castTIME_utf8(context_ptr, "00:00:00.000", 12));
171183

172-
EXPECT_EQ(castTIME_utf8(context_ptr, "00:00:00.1000", 13), 0);
173-
EXPECT_EQ(context.get_error(), "Invalid millis for time value 00:00:00.1000");
174-
context.Reset();
184+
// "00:00:00.1000" should truncate to "00:00:00.100"
185+
EXPECT_EQ(castTIME_utf8(context_ptr, "00:00:00.1000", 13),
186+
castTIME_utf8(context_ptr, "00:00:00.100", 12));
187+
188+
// "9:45:30.123456789" should truncate to "9:45:30.123"
189+
EXPECT_EQ(castTIME_utf8(context_ptr, "9:45:30.123456789", 17),
190+
castTIME_utf8(context_ptr, "9:45:30.123", 11));
191+
192+
// "00:00:00.1999" should truncate to "00:00:00.199"
193+
EXPECT_EQ(castTIME_utf8(context_ptr, "00:00:00.1999", 13),
194+
castTIME_utf8(context_ptr, "00:00:00.199", 12));
195+
196+
// "00:00:00.1994" should truncate to "00:00:00.199"
197+
EXPECT_EQ(castTIME_utf8(context_ptr, "00:00:00.1994", 13),
198+
castTIME_utf8(context_ptr, "00:00:00.199", 12));
175199
}
176200

177201
#ifndef _WIN32

‎dev/tasks/java-jars/github.yml‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ jobs:
3939
archery_arch: "amd64"
4040
archery_arch_alias: "x86_64"
4141
archery_arch_short: "amd64"
42-
- runs_on: ["buildjet-8vcpu-ubuntu-2204-arm"]
42+
- runs_on: ["ubuntu-24.04-arm"]
4343
arch: "aarch_64"
4444
archery_arch: "arm64v8"
4545
archery_arch_alias: "aarch64"
@@ -65,6 +65,11 @@ jobs:
6565
env:
6666
{{ macros.github_set_sccache_envvars()|indent(8) }}
6767
run: |
68+
# ubuntu-24.04-arm ships Docker Compose v2 as a plugin only; archery needs standalone docker-compose
69+
if ! command -v docker-compose &>/dev/null; then
70+
printf '#!/bin/sh\nexec docker compose "$@"\n' | sudo tee /usr/local/bin/docker-compose >/dev/null
71+
sudo chmod +x /usr/local/bin/docker-compose
72+
fi
6873
archery docker run \
6974
-e ARROW_JAVA_BUILD=OFF \
7075
-e ARROW_JAVA_TEST=OFF \
@@ -148,7 +153,7 @@ jobs:
148153
# used on test We uninstall Homebrew's Protobuf to ensure using
149154
# bundled Protobuf.
150155
brew uninstall protobuf
151-
# fix cmake and boost versionsAdd commentMore actions
156+
# fix cmake and boost versions
152157
brew uninstall -f boost || true
153158
brew uninstall -f cmake || true
154159
mkdir -p homebrew-custom/Formula
@@ -158,8 +163,6 @@ jobs:
158163
cp ./homebrew-custom/Formula/*.rb "$(brew --repo local/homebrew-custom)/Formula/"
159164
brew install -v local/homebrew-custom/cmake
160165
brew install -v local/homebrew-custom/boost
161-
brew pin cmake
162-
brew pin boost
163166
#
164167
165168

0 commit comments

Comments
 (0)