You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "paleolimbot (via GitHub)" <gi...@apache.org> on 2024/01/12 19:22:07 UTC
[PR] feat: Add decimal support to integration tester [arrow-nanoarrow]
paleolimbot opened a new pull request, #361:
URL: https://github.com/apache/arrow-nanoarrow/pull/361
(no comment)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add decimal support to integration tester [arrow-nanoarrow]
Posted by "danepitkin (via GitHub)" <gi...@apache.org>.
danepitkin commented on PR #361:
URL: https://github.com/apache/arrow-nanoarrow/pull/361#issuecomment-1908804924
Overall LGTM!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add decimal support to integration tester [arrow-nanoarrow]
Posted by "danepitkin (via GitHub)" <gi...@apache.org>.
danepitkin commented on code in PR #361:
URL: https://github.com/apache/arrow-nanoarrow/pull/361#discussion_r1455727949
##########
src/nanoarrow/utils.c:
##########
@@ -223,3 +223,186 @@ struct ArrowBufferAllocator ArrowBufferDeallocator(
allocator.private_data = private_data;
return allocator;
}
+
+static const int kInt32DecimalDigits = 9;
+
+static const uint64_t kUInt32PowersOfTen[] = {
+ 1ULL, 10ULL, 100ULL, 1000ULL, 10000ULL,
+ 100000ULL, 1000000ULL, 10000000ULL, 100000000ULL, 1000000000ULL};
+
+// Adapted from Arrow C++ to use 32-bit words for better C portability
+// https://github.com/apache/arrow/blob/main/cpp/src/arrow/util/decimal.cc#L524-L544
+static void ShiftAndAdd(struct ArrowStringView value, uint32_t* out, int64_t out_size) {
+ // We use strtoll for parsing, which needs input that is null-terminated
+ char chunk_string[16];
+
+ for (int64_t posn = 0; posn < value.size_bytes;) {
+ int64_t remaining = value.size_bytes - posn;
+
+ int64_t group_size;
+ if (remaining > kInt32DecimalDigits) {
+ group_size = kInt32DecimalDigits;
+ } else {
+ group_size = remaining;
+ }
+
+ const uint64_t multiple = kUInt32PowersOfTen[group_size];
+
+ memcpy(chunk_string, value.data + posn, group_size);
+ chunk_string[group_size] = '\0';
+ uint32_t chunk = (uint32_t)strtoll(chunk_string, NULL, 10);
+
+ for (int64_t i = 0; i < out_size; i++) {
+ uint64_t tmp = out[i];
+ tmp *= multiple;
+ tmp += chunk;
+ out[i] = (uint32_t)(tmp & 0xFFFFFFFFULL);
+ chunk = (uint32_t)(tmp >> 32);
+ }
+ posn += group_size;
+ }
+}
+
+ArrowErrorCode ArrowDecimalSetIntString(struct ArrowDecimal* decimal,
+ struct ArrowStringView value) {
+ // Check for sign
+ int is_negative = value.data[0] == '-';
+ int has_sign = is_negative || value.data[0] == '+';
+ value.data += has_sign;
+ value.size_bytes -= has_sign;
+
+ // Check all characters are digits that are not the negative sign
+ for (int64_t i = 0; i < value.size_bytes; i++) {
+ char c = value.data[i];
+ if (c < '0' || c > '9') {
+ return EINVAL;
+ }
Review Comment:
ignore me then! This code is already very readable IMO. Other parts of the diff look like they are specifically written to avoid branching in loops (e.g. `if` statements), so I wasn't sure if that was an active decision.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add decimal support to integration tester [arrow-nanoarrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #361:
URL: https://github.com/apache/arrow-nanoarrow/pull/361#discussion_r1462435746
##########
src/nanoarrow/utils.c:
##########
@@ -223,3 +223,186 @@ struct ArrowBufferAllocator ArrowBufferDeallocator(
allocator.private_data = private_data;
return allocator;
}
+
+static const int kInt32DecimalDigits = 9;
+
+static const uint64_t kUInt32PowersOfTen[] = {
+ 1ULL, 10ULL, 100ULL, 1000ULL, 10000ULL,
+ 100000ULL, 1000000ULL, 10000000ULL, 100000000ULL, 1000000000ULL};
+
+// Adapted from Arrow C++ to use 32-bit words for better C portability
+// https://github.com/apache/arrow/blob/cd3321b28b0c9703e5d7105d6146c1270bbadd7f/cpp/src/arrow/util/decimal.cc#L524-L544
+static void ShiftAndAdd(struct ArrowStringView value, uint32_t* out, int64_t out_size) {
+ // We use strtoll for parsing, which needs input that is null-terminated
+ char chunk_string[16];
+
+ for (int64_t posn = 0; posn < value.size_bytes;) {
+ int64_t remaining = value.size_bytes - posn;
+
+ int64_t group_size;
+ if (remaining > kInt32DecimalDigits) {
+ group_size = kInt32DecimalDigits;
+ } else {
+ group_size = remaining;
+ }
+
+ const uint64_t multiple = kUInt32PowersOfTen[group_size];
+
+ memcpy(chunk_string, value.data + posn, group_size);
+ chunk_string[group_size] = '\0';
+ uint32_t chunk = (uint32_t)strtoll(chunk_string, NULL, 10);
+
+ for (int64_t i = 0; i < out_size; i++) {
+ uint64_t tmp = out[i];
+ tmp *= multiple;
+ tmp += chunk;
+ out[i] = (uint32_t)(tmp & 0xFFFFFFFFULL);
+ chunk = (uint32_t)(tmp >> 32);
+ }
+ posn += group_size;
+ }
+}
+
+ArrowErrorCode ArrowDecimalSetDigits(struct ArrowDecimal* decimal,
+ struct ArrowStringView value) {
+ // Check for sign
+ int is_negative = value.data[0] == '-';
+ int has_sign = is_negative || value.data[0] == '+';
+ value.data += has_sign;
+ value.size_bytes -= has_sign;
+
+ // Check all characters are digits that are not the negative sign
+ for (int64_t i = 0; i < value.size_bytes; i++) {
+ char c = value.data[i];
+ if (c < '0' || c > '9') {
+ return EINVAL;
+ }
+ }
+
+ // Skip over leading 0s
+ int64_t n_leading_zeroes = 0;
+ for (int64_t i = 0; i < value.size_bytes; i++) {
+ if (value.data[i] == '0') {
+ n_leading_zeroes++;
+ } else {
+ break;
+ }
+ }
+
+ value.data += n_leading_zeroes;
+ value.size_bytes -= n_leading_zeroes;
+
+ // Use 32-bit words for portability
+ uint32_t words32[8];
+ int n_words32 = decimal->n_words * 2;
+ memset(words32, 0, sizeof(words32));
+
+ ShiftAndAdd(value, words32, n_words32);
+
+ if (decimal->low_word_index == 0) {
+ memcpy(decimal->words, words32, sizeof(uint32_t) * n_words32);
+ } else {
+ uint64_t lo;
+ uint64_t hi;
+
+ for (int i = 0; i < decimal->n_words; i++) {
+ lo = (uint64_t)words32[i * 2];
+ hi = (uint64_t)words32[i * 2 + 1] << 32;
+ decimal->words[decimal->n_words - i - 1] = lo | hi;
+ }
+ }
+
+ if (is_negative) {
+ ArrowDecimalNegate(decimal);
+ }
+
+ return NANOARROW_OK;
+}
+
+// Adapted from Arrow C++ for C
+// https://github.com/apache/arrow/blob/cd3321b28b0c9703e5d7105d6146c1270bbadd7f/cpp/src/arrow/util/decimal.cc#L365
+ArrowErrorCode ArrowDecimalAppendDigitsToBuffer(const struct ArrowDecimal* decimal,
+ struct ArrowBuffer* buffer) {
+ int is_negative = ArrowDecimalSign(decimal) < 0;
+
+ uint64_t words_little_endian[4];
+ if (decimal->low_word_index == 0) {
+ memcpy(words_little_endian, decimal->words, decimal->n_words * sizeof(uint64_t));
+ } else {
+ for (int i = 0; i < decimal->n_words; i++) {
+ words_little_endian[i] = decimal->words[decimal->n_words - i - 1];
+ }
+ }
+
+ // We've already made a copy, so negate that if needed
+ if (is_negative) {
+ uint64_t carry = 1;
+ for (int i = 0; i < decimal->n_words; i++) {
+ uint64_t elem = words_little_endian[i];
+ elem = ~elem + carry;
+ carry &= (elem == 0);
+ words_little_endian[i] = elem;
+ }
+ }
+
+ int most_significant_elem_idx = -1;
+ for (int i = decimal->n_words - 1; i >= 0; i--) {
+ if (words_little_endian[i] != 0) {
+ most_significant_elem_idx = i;
+ break;
+ }
+ }
+
+ if (most_significant_elem_idx == -1) {
+ NANOARROW_RETURN_NOT_OK(ArrowBufferAppendInt8(buffer, '0'));
+ return NANOARROW_OK;
+ }
+
+ const uint32_t k1e9 = 1000000000U;
+ int num_segments = 0;
+ uint32_t segments[9];
+ memset(segments, 0, sizeof(segments));
+ uint64_t* most_significant_elem = words_little_endian + most_significant_elem_idx;
+
+ do {
+ uint32_t remainder = 0;
+ uint64_t* elem = most_significant_elem;
+ do {
+ uint32_t hi = (uint32_t)(*elem >> 32);
+ uint32_t lo = (uint32_t)(*elem & 0xFFFFFFFFULL);
+ uint64_t dividend_hi = ((uint64_t)(remainder) << 32) | hi;
+ uint64_t quotient_hi = dividend_hi / k1e9;
+ remainder = (uint32_t)(dividend_hi % k1e9);
+ uint64_t dividend_lo = ((uint64_t)(remainder) << 32) | lo;
+ uint64_t quotient_lo = dividend_lo / k1e9;
+ remainder = (uint32_t)(dividend_lo % k1e9);
+
+ *elem = (quotient_hi << 32) | quotient_lo;
+ } while (elem-- != words_little_endian);
+
+ segments[num_segments++] = remainder;
+ } while (*most_significant_elem != 0 || most_significant_elem-- != words_little_endian);
+
+ // We know our output has no more than 9 digits per segment, plus a negative sign,
+ // plus any further digits between our output of 9 digits and the maximum theoretical
+ // number of digits in an unsigned long, including the null terminator.
Review Comment:
The idea was to make sure that the `21` in `snprintf(, 21, "%lu")` is always accurate (I added this to the comment).
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add decimal support to integration tester [arrow-nanoarrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #361:
URL: https://github.com/apache/arrow-nanoarrow/pull/361#discussion_r1461996897
##########
src/nanoarrow/utils.c:
##########
@@ -223,3 +223,186 @@ struct ArrowBufferAllocator ArrowBufferDeallocator(
allocator.private_data = private_data;
return allocator;
}
+
+static const int kInt32DecimalDigits = 9;
+
+static const uint64_t kUInt32PowersOfTen[] = {
+ 1ULL, 10ULL, 100ULL, 1000ULL, 10000ULL,
+ 100000ULL, 1000000ULL, 10000000ULL, 100000000ULL, 1000000000ULL};
+
+// Adapted from Arrow C++ to use 32-bit words for better C portability
+// https://github.com/apache/arrow/blob/cd3321b28b0c9703e5d7105d6146c1270bbadd7f/cpp/src/arrow/util/decimal.cc#L524-L544
+static void ShiftAndAdd(struct ArrowStringView value, uint32_t* out, int64_t out_size) {
+ // We use strtoll for parsing, which needs input that is null-terminated
+ char chunk_string[16];
+
+ for (int64_t posn = 0; posn < value.size_bytes;) {
+ int64_t remaining = value.size_bytes - posn;
+
+ int64_t group_size;
+ if (remaining > kInt32DecimalDigits) {
+ group_size = kInt32DecimalDigits;
+ } else {
+ group_size = remaining;
+ }
+
+ const uint64_t multiple = kUInt32PowersOfTen[group_size];
+
+ memcpy(chunk_string, value.data + posn, group_size);
+ chunk_string[group_size] = '\0';
+ uint32_t chunk = (uint32_t)strtoll(chunk_string, NULL, 10);
+
+ for (int64_t i = 0; i < out_size; i++) {
+ uint64_t tmp = out[i];
+ tmp *= multiple;
+ tmp += chunk;
+ out[i] = (uint32_t)(tmp & 0xFFFFFFFFULL);
+ chunk = (uint32_t)(tmp >> 32);
+ }
+ posn += group_size;
+ }
+}
+
+ArrowErrorCode ArrowDecimalSetDigits(struct ArrowDecimal* decimal,
+ struct ArrowStringView value) {
+ // Check for sign
+ int is_negative = value.data[0] == '-';
+ int has_sign = is_negative || value.data[0] == '+';
+ value.data += has_sign;
+ value.size_bytes -= has_sign;
+
+ // Check all characters are digits that are not the negative sign
+ for (int64_t i = 0; i < value.size_bytes; i++) {
+ char c = value.data[i];
+ if (c < '0' || c > '9') {
+ return EINVAL;
+ }
+ }
+
+ // Skip over leading 0s
+ int64_t n_leading_zeroes = 0;
+ for (int64_t i = 0; i < value.size_bytes; i++) {
+ if (value.data[i] == '0') {
+ n_leading_zeroes++;
+ } else {
+ break;
+ }
+ }
+
+ value.data += n_leading_zeroes;
+ value.size_bytes -= n_leading_zeroes;
+
+ // Use 32-bit words for portability
+ uint32_t words32[8];
+ int n_words32 = decimal->n_words * 2;
+ memset(words32, 0, sizeof(words32));
+
+ ShiftAndAdd(value, words32, n_words32);
+
+ if (decimal->low_word_index == 0) {
+ memcpy(decimal->words, words32, sizeof(uint32_t) * n_words32);
+ } else {
+ uint64_t lo;
+ uint64_t hi;
+
+ for (int i = 0; i < decimal->n_words; i++) {
+ lo = (uint64_t)words32[i * 2];
+ hi = (uint64_t)words32[i * 2 + 1] << 32;
+ decimal->words[decimal->n_words - i - 1] = lo | hi;
+ }
+ }
+
+ if (is_negative) {
+ ArrowDecimalNegate(decimal);
+ }
+
+ return NANOARROW_OK;
+}
+
+// Adapted from Arrow C++ for C
+// https://github.com/apache/arrow/blob/cd3321b28b0c9703e5d7105d6146c1270bbadd7f/cpp/src/arrow/util/decimal.cc#L365
+ArrowErrorCode ArrowDecimalAppendDigitsToBuffer(const struct ArrowDecimal* decimal,
+ struct ArrowBuffer* buffer) {
+ int is_negative = ArrowDecimalSign(decimal) < 0;
+
+ uint64_t words_little_endian[4];
+ if (decimal->low_word_index == 0) {
+ memcpy(words_little_endian, decimal->words, decimal->n_words * sizeof(uint64_t));
+ } else {
+ for (int i = 0; i < decimal->n_words; i++) {
+ words_little_endian[i] = decimal->words[decimal->n_words - i - 1];
+ }
+ }
+
+ // We've already made a copy, so negate that if needed
+ if (is_negative) {
+ uint64_t carry = 1;
+ for (int i = 0; i < decimal->n_words; i++) {
+ uint64_t elem = words_little_endian[i];
+ elem = ~elem + carry;
+ carry &= (elem == 0);
+ words_little_endian[i] = elem;
+ }
+ }
+
+ int most_significant_elem_idx = -1;
+ for (int i = decimal->n_words - 1; i >= 0; i--) {
+ if (words_little_endian[i] != 0) {
+ most_significant_elem_idx = i;
+ break;
+ }
+ }
+
+ if (most_significant_elem_idx == -1) {
+ NANOARROW_RETURN_NOT_OK(ArrowBufferAppendInt8(buffer, '0'));
+ return NANOARROW_OK;
+ }
+
+ const uint32_t k1e9 = 1000000000U;
+ int num_segments = 0;
+ uint32_t segments[9];
+ memset(segments, 0, sizeof(segments));
+ uint64_t* most_significant_elem = words_little_endian + most_significant_elem_idx;
+
+ do {
Review Comment:
You should comment on what this is doing.
##########
src/nanoarrow/utils.c:
##########
@@ -223,3 +223,186 @@ struct ArrowBufferAllocator ArrowBufferDeallocator(
allocator.private_data = private_data;
return allocator;
}
+
+static const int kInt32DecimalDigits = 9;
+
+static const uint64_t kUInt32PowersOfTen[] = {
+ 1ULL, 10ULL, 100ULL, 1000ULL, 10000ULL,
+ 100000ULL, 1000000ULL, 10000000ULL, 100000000ULL, 1000000000ULL};
+
+// Adapted from Arrow C++ to use 32-bit words for better C portability
+// https://github.com/apache/arrow/blob/cd3321b28b0c9703e5d7105d6146c1270bbadd7f/cpp/src/arrow/util/decimal.cc#L524-L544
+static void ShiftAndAdd(struct ArrowStringView value, uint32_t* out, int64_t out_size) {
+ // We use strtoll for parsing, which needs input that is null-terminated
+ char chunk_string[16];
+
+ for (int64_t posn = 0; posn < value.size_bytes;) {
+ int64_t remaining = value.size_bytes - posn;
+
+ int64_t group_size;
+ if (remaining > kInt32DecimalDigits) {
+ group_size = kInt32DecimalDigits;
+ } else {
+ group_size = remaining;
+ }
+
+ const uint64_t multiple = kUInt32PowersOfTen[group_size];
+
+ memcpy(chunk_string, value.data + posn, group_size);
+ chunk_string[group_size] = '\0';
+ uint32_t chunk = (uint32_t)strtoll(chunk_string, NULL, 10);
+
+ for (int64_t i = 0; i < out_size; i++) {
+ uint64_t tmp = out[i];
+ tmp *= multiple;
+ tmp += chunk;
+ out[i] = (uint32_t)(tmp & 0xFFFFFFFFULL);
+ chunk = (uint32_t)(tmp >> 32);
+ }
+ posn += group_size;
+ }
+}
+
+ArrowErrorCode ArrowDecimalSetDigits(struct ArrowDecimal* decimal,
+ struct ArrowStringView value) {
+ // Check for sign
+ int is_negative = value.data[0] == '-';
+ int has_sign = is_negative || value.data[0] == '+';
+ value.data += has_sign;
+ value.size_bytes -= has_sign;
+
+ // Check all characters are digits that are not the negative sign
+ for (int64_t i = 0; i < value.size_bytes; i++) {
+ char c = value.data[i];
+ if (c < '0' || c > '9') {
+ return EINVAL;
+ }
+ }
+
+ // Skip over leading 0s
+ int64_t n_leading_zeroes = 0;
+ for (int64_t i = 0; i < value.size_bytes; i++) {
+ if (value.data[i] == '0') {
+ n_leading_zeroes++;
+ } else {
+ break;
+ }
+ }
+
+ value.data += n_leading_zeroes;
+ value.size_bytes -= n_leading_zeroes;
+
+ // Use 32-bit words for portability
+ uint32_t words32[8];
+ int n_words32 = decimal->n_words * 2;
Review Comment:
Assert that this is <= 8?
##########
src/nanoarrow/utils_test.cc:
##########
@@ -269,3 +292,154 @@ TEST(DecimalTest, Decimal256Test) {
ArrowDecimalSetBytes(&decimal, bytes_neg);
EXPECT_EQ(memcmp(decimal.words, bytes_neg, sizeof(bytes_neg)), 0);
}
+
+TEST(DecimalTest, DecimalStringTestBasic) {
+ struct ArrowDecimal decimal;
+ ArrowDecimalInit(&decimal, 128, 39, 0);
+
+ struct ArrowBuffer buffer;
+ ArrowBufferInit(&buffer);
+
+ // Only spans one 32-bit word
+ ASSERT_EQ(ArrowDecimalSetDigits(&decimal, ArrowCharView("123456")), NANOARROW_OK);
+ EXPECT_EQ(ArrowDecimalGetIntUnsafe(&decimal), 123456);
+
+ // Check roundtrip to string
+ buffer.size_bytes = 0;
+ ASSERT_EQ(ArrowDecimalAppendDigitsToBuffer(&decimal, &buffer), NANOARROW_OK);
+ EXPECT_EQ(std::string(reinterpret_cast<char*>(buffer.data), buffer.size_bytes),
+ "123456");
+
+ // Negative value
+ ASSERT_EQ(ArrowDecimalSetDigits(&decimal, ArrowCharView("-123456")), NANOARROW_OK);
+ EXPECT_EQ(ArrowDecimalGetIntUnsafe(&decimal), -123456);
+
+ // Check roundtrip to string
+ buffer.size_bytes = 0;
+ ASSERT_EQ(ArrowDecimalAppendDigitsToBuffer(&decimal, &buffer), NANOARROW_OK);
+ EXPECT_EQ(std::string(reinterpret_cast<char*>(buffer.data), buffer.size_bytes),
+ "-123456");
+
+ // Spans >1 32-bit word
+ ASSERT_EQ(ArrowDecimalSetDigits(&decimal, ArrowCharView("1234567899")), NANOARROW_OK);
+ EXPECT_EQ(ArrowDecimalGetIntUnsafe(&decimal), 1234567899L);
+
+ // Check roundtrip to string
+ buffer.size_bytes = 0;
+ ASSERT_EQ(ArrowDecimalAppendDigitsToBuffer(&decimal, &buffer), NANOARROW_OK);
+ EXPECT_EQ(std::string(reinterpret_cast<char*>(buffer.data), buffer.size_bytes),
+ "1234567899");
+
+ // Check maximum value of a 64-bit integer
+ ASSERT_EQ(ArrowDecimalSetDigits(&decimal, ArrowCharView("18446744073709551615")),
+ NANOARROW_OK);
+ EXPECT_EQ(decimal.words[decimal.low_word_index], std::numeric_limits<uint64_t>::max());
+ EXPECT_EQ(decimal.words[decimal.high_word_index], 0);
+
+ // Check roundtrip to string
+ buffer.size_bytes = 0;
+ ASSERT_EQ(ArrowDecimalAppendDigitsToBuffer(&decimal, &buffer), NANOARROW_OK);
+ EXPECT_EQ(std::string(reinterpret_cast<char*>(buffer.data), buffer.size_bytes),
+ "18446744073709551615");
+
+ // Check with the maximum value of a 128-bit integer
Review Comment:
Could you also have a similar test for 256 bits?
##########
src/nanoarrow/utils.c:
##########
@@ -223,3 +223,186 @@ struct ArrowBufferAllocator ArrowBufferDeallocator(
allocator.private_data = private_data;
return allocator;
}
+
+static const int kInt32DecimalDigits = 9;
+
+static const uint64_t kUInt32PowersOfTen[] = {
+ 1ULL, 10ULL, 100ULL, 1000ULL, 10000ULL,
+ 100000ULL, 1000000ULL, 10000000ULL, 100000000ULL, 1000000000ULL};
+
+// Adapted from Arrow C++ to use 32-bit words for better C portability
+// https://github.com/apache/arrow/blob/cd3321b28b0c9703e5d7105d6146c1270bbadd7f/cpp/src/arrow/util/decimal.cc#L524-L544
+static void ShiftAndAdd(struct ArrowStringView value, uint32_t* out, int64_t out_size) {
+ // We use strtoll for parsing, which needs input that is null-terminated
+ char chunk_string[16];
+
+ for (int64_t posn = 0; posn < value.size_bytes;) {
+ int64_t remaining = value.size_bytes - posn;
+
+ int64_t group_size;
+ if (remaining > kInt32DecimalDigits) {
+ group_size = kInt32DecimalDigits;
+ } else {
+ group_size = remaining;
+ }
+
+ const uint64_t multiple = kUInt32PowersOfTen[group_size];
+
+ memcpy(chunk_string, value.data + posn, group_size);
+ chunk_string[group_size] = '\0';
+ uint32_t chunk = (uint32_t)strtoll(chunk_string, NULL, 10);
+
+ for (int64_t i = 0; i < out_size; i++) {
+ uint64_t tmp = out[i];
+ tmp *= multiple;
+ tmp += chunk;
+ out[i] = (uint32_t)(tmp & 0xFFFFFFFFULL);
+ chunk = (uint32_t)(tmp >> 32);
+ }
+ posn += group_size;
+ }
+}
+
+ArrowErrorCode ArrowDecimalSetDigits(struct ArrowDecimal* decimal,
+ struct ArrowStringView value) {
+ // Check for sign
+ int is_negative = value.data[0] == '-';
+ int has_sign = is_negative || value.data[0] == '+';
+ value.data += has_sign;
+ value.size_bytes -= has_sign;
+
+ // Check all characters are digits that are not the negative sign
+ for (int64_t i = 0; i < value.size_bytes; i++) {
+ char c = value.data[i];
+ if (c < '0' || c > '9') {
+ return EINVAL;
+ }
+ }
+
+ // Skip over leading 0s
+ int64_t n_leading_zeroes = 0;
+ for (int64_t i = 0; i < value.size_bytes; i++) {
+ if (value.data[i] == '0') {
+ n_leading_zeroes++;
+ } else {
+ break;
+ }
+ }
+
+ value.data += n_leading_zeroes;
+ value.size_bytes -= n_leading_zeroes;
+
+ // Use 32-bit words for portability
+ uint32_t words32[8];
+ int n_words32 = decimal->n_words * 2;
+ memset(words32, 0, sizeof(words32));
+
+ ShiftAndAdd(value, words32, n_words32);
+
+ if (decimal->low_word_index == 0) {
+ memcpy(decimal->words, words32, sizeof(uint32_t) * n_words32);
+ } else {
+ uint64_t lo;
+ uint64_t hi;
+
+ for (int i = 0; i < decimal->n_words; i++) {
+ lo = (uint64_t)words32[i * 2];
+ hi = (uint64_t)words32[i * 2 + 1] << 32;
+ decimal->words[decimal->n_words - i - 1] = lo | hi;
+ }
+ }
+
+ if (is_negative) {
+ ArrowDecimalNegate(decimal);
+ }
+
+ return NANOARROW_OK;
+}
+
+// Adapted from Arrow C++ for C
+// https://github.com/apache/arrow/blob/cd3321b28b0c9703e5d7105d6146c1270bbadd7f/cpp/src/arrow/util/decimal.cc#L365
+ArrowErrorCode ArrowDecimalAppendDigitsToBuffer(const struct ArrowDecimal* decimal,
+ struct ArrowBuffer* buffer) {
+ int is_negative = ArrowDecimalSign(decimal) < 0;
+
+ uint64_t words_little_endian[4];
+ if (decimal->low_word_index == 0) {
+ memcpy(words_little_endian, decimal->words, decimal->n_words * sizeof(uint64_t));
+ } else {
+ for (int i = 0; i < decimal->n_words; i++) {
+ words_little_endian[i] = decimal->words[decimal->n_words - i - 1];
+ }
+ }
+
+ // We've already made a copy, so negate that if needed
+ if (is_negative) {
+ uint64_t carry = 1;
+ for (int i = 0; i < decimal->n_words; i++) {
+ uint64_t elem = words_little_endian[i];
+ elem = ~elem + carry;
+ carry &= (elem == 0);
+ words_little_endian[i] = elem;
+ }
+ }
+
+ int most_significant_elem_idx = -1;
+ for (int i = decimal->n_words - 1; i >= 0; i--) {
+ if (words_little_endian[i] != 0) {
+ most_significant_elem_idx = i;
+ break;
+ }
+ }
+
+ if (most_significant_elem_idx == -1) {
+ NANOARROW_RETURN_NOT_OK(ArrowBufferAppendInt8(buffer, '0'));
+ return NANOARROW_OK;
+ }
+
+ const uint32_t k1e9 = 1000000000U;
+ int num_segments = 0;
+ uint32_t segments[9];
+ memset(segments, 0, sizeof(segments));
+ uint64_t* most_significant_elem = words_little_endian + most_significant_elem_idx;
+
+ do {
+ uint32_t remainder = 0;
+ uint64_t* elem = most_significant_elem;
+ do {
+ uint32_t hi = (uint32_t)(*elem >> 32);
+ uint32_t lo = (uint32_t)(*elem & 0xFFFFFFFFULL);
+ uint64_t dividend_hi = ((uint64_t)(remainder) << 32) | hi;
+ uint64_t quotient_hi = dividend_hi / k1e9;
+ remainder = (uint32_t)(dividend_hi % k1e9);
+ uint64_t dividend_lo = ((uint64_t)(remainder) << 32) | lo;
+ uint64_t quotient_lo = dividend_lo / k1e9;
+ remainder = (uint32_t)(dividend_lo % k1e9);
+
+ *elem = (quotient_hi << 32) | quotient_lo;
+ } while (elem-- != words_little_endian);
+
+ segments[num_segments++] = remainder;
+ } while (*most_significant_elem != 0 || most_significant_elem-- != words_little_endian);
+
+ // We know our output has no more than 9 digits per segment, plus a negative sign,
+ // plus any further digits between our output of 9 digits and the maximum theoretical
+ // number of digits in an unsigned long, including the null terminator.
Review Comment:
I don't understand what the maximum theoretical number of digits has to do with this, can you explain?
##########
src/nanoarrow/utils.c:
##########
@@ -223,3 +223,186 @@ struct ArrowBufferAllocator ArrowBufferDeallocator(
allocator.private_data = private_data;
return allocator;
}
+
+static const int kInt32DecimalDigits = 9;
+
+static const uint64_t kUInt32PowersOfTen[] = {
+ 1ULL, 10ULL, 100ULL, 1000ULL, 10000ULL,
+ 100000ULL, 1000000ULL, 10000000ULL, 100000000ULL, 1000000000ULL};
+
+// Adapted from Arrow C++ to use 32-bit words for better C portability
+// https://github.com/apache/arrow/blob/cd3321b28b0c9703e5d7105d6146c1270bbadd7f/cpp/src/arrow/util/decimal.cc#L524-L544
+static void ShiftAndAdd(struct ArrowStringView value, uint32_t* out, int64_t out_size) {
+ // We use strtoll for parsing, which needs input that is null-terminated
+ char chunk_string[16];
+
+ for (int64_t posn = 0; posn < value.size_bytes;) {
+ int64_t remaining = value.size_bytes - posn;
+
+ int64_t group_size;
+ if (remaining > kInt32DecimalDigits) {
+ group_size = kInt32DecimalDigits;
+ } else {
+ group_size = remaining;
+ }
+
+ const uint64_t multiple = kUInt32PowersOfTen[group_size];
+
+ memcpy(chunk_string, value.data + posn, group_size);
+ chunk_string[group_size] = '\0';
+ uint32_t chunk = (uint32_t)strtoll(chunk_string, NULL, 10);
+
+ for (int64_t i = 0; i < out_size; i++) {
+ uint64_t tmp = out[i];
+ tmp *= multiple;
+ tmp += chunk;
+ out[i] = (uint32_t)(tmp & 0xFFFFFFFFULL);
+ chunk = (uint32_t)(tmp >> 32);
+ }
+ posn += group_size;
+ }
+}
+
+ArrowErrorCode ArrowDecimalSetDigits(struct ArrowDecimal* decimal,
+ struct ArrowStringView value) {
+ // Check for sign
+ int is_negative = value.data[0] == '-';
+ int has_sign = is_negative || value.data[0] == '+';
+ value.data += has_sign;
+ value.size_bytes -= has_sign;
+
+ // Check all characters are digits that are not the negative sign
+ for (int64_t i = 0; i < value.size_bytes; i++) {
+ char c = value.data[i];
+ if (c < '0' || c > '9') {
+ return EINVAL;
+ }
+ }
+
+ // Skip over leading 0s
+ int64_t n_leading_zeroes = 0;
+ for (int64_t i = 0; i < value.size_bytes; i++) {
+ if (value.data[i] == '0') {
+ n_leading_zeroes++;
+ } else {
+ break;
+ }
+ }
+
+ value.data += n_leading_zeroes;
+ value.size_bytes -= n_leading_zeroes;
+
+ // Use 32-bit words for portability
+ uint32_t words32[8];
+ int n_words32 = decimal->n_words * 2;
+ memset(words32, 0, sizeof(words32));
+
+ ShiftAndAdd(value, words32, n_words32);
+
+ if (decimal->low_word_index == 0) {
+ memcpy(decimal->words, words32, sizeof(uint32_t) * n_words32);
+ } else {
+ uint64_t lo;
+ uint64_t hi;
+
+ for (int i = 0; i < decimal->n_words; i++) {
+ lo = (uint64_t)words32[i * 2];
+ hi = (uint64_t)words32[i * 2 + 1] << 32;
+ decimal->words[decimal->n_words - i - 1] = lo | hi;
+ }
+ }
+
+ if (is_negative) {
+ ArrowDecimalNegate(decimal);
+ }
+
+ return NANOARROW_OK;
+}
+
+// Adapted from Arrow C++ for C
+// https://github.com/apache/arrow/blob/cd3321b28b0c9703e5d7105d6146c1270bbadd7f/cpp/src/arrow/util/decimal.cc#L365
+ArrowErrorCode ArrowDecimalAppendDigitsToBuffer(const struct ArrowDecimal* decimal,
+ struct ArrowBuffer* buffer) {
+ int is_negative = ArrowDecimalSign(decimal) < 0;
+
+ uint64_t words_little_endian[4];
+ if (decimal->low_word_index == 0) {
+ memcpy(words_little_endian, decimal->words, decimal->n_words * sizeof(uint64_t));
+ } else {
+ for (int i = 0; i < decimal->n_words; i++) {
+ words_little_endian[i] = decimal->words[decimal->n_words - i - 1];
+ }
+ }
+
+ // We've already made a copy, so negate that if needed
+ if (is_negative) {
+ uint64_t carry = 1;
+ for (int i = 0; i < decimal->n_words; i++) {
+ uint64_t elem = words_little_endian[i];
+ elem = ~elem + carry;
+ carry &= (elem == 0);
+ words_little_endian[i] = elem;
+ }
+ }
+
+ int most_significant_elem_idx = -1;
+ for (int i = decimal->n_words - 1; i >= 0; i--) {
+ if (words_little_endian[i] != 0) {
+ most_significant_elem_idx = i;
+ break;
+ }
+ }
+
+ if (most_significant_elem_idx == -1) {
+ NANOARROW_RETURN_NOT_OK(ArrowBufferAppendInt8(buffer, '0'));
+ return NANOARROW_OK;
+ }
+
+ const uint32_t k1e9 = 1000000000U;
+ int num_segments = 0;
+ uint32_t segments[9];
+ memset(segments, 0, sizeof(segments));
+ uint64_t* most_significant_elem = words_little_endian + most_significant_elem_idx;
+
+ do {
+ uint32_t remainder = 0;
+ uint64_t* elem = most_significant_elem;
+ do {
+ uint32_t hi = (uint32_t)(*elem >> 32);
+ uint32_t lo = (uint32_t)(*elem & 0xFFFFFFFFULL);
+ uint64_t dividend_hi = ((uint64_t)(remainder) << 32) | hi;
+ uint64_t quotient_hi = dividend_hi / k1e9;
+ remainder = (uint32_t)(dividend_hi % k1e9);
+ uint64_t dividend_lo = ((uint64_t)(remainder) << 32) | lo;
+ uint64_t quotient_lo = dividend_lo / k1e9;
+ remainder = (uint32_t)(dividend_lo % k1e9);
+
+ *elem = (quotient_hi << 32) | quotient_lo;
+ } while (elem-- != words_little_endian);
+
+ segments[num_segments++] = remainder;
+ } while (*most_significant_elem != 0 || most_significant_elem-- != words_little_endian);
+
+ // We know our output has no more than 9 digits per segment, plus a negative sign,
+ // plus any further digits between our output of 9 digits and the maximum theoretical
+ // number of digits in an unsigned long, including the null terminator.
+ NANOARROW_RETURN_NOT_OK(ArrowBufferReserve(buffer, num_segments * 9 + 1 + 21 - 9));
+ if (is_negative) {
+ buffer->data[buffer->size_bytes++] = '-';
+ }
+
+ // The most significant segment should have no leading zeroes
+ int n_chars = snprintf((char*)buffer->data + buffer->size_bytes, 21, "%lu",
+ (unsigned long)segments[num_segments - 1]);
+ buffer->size_bytes += n_chars;
+
+ // Subsequent output needs to be left-padded with zeroes such that each segment
+ // takes up exactly 9 digits.
+ for (int i = num_segments - 2; i >= 0; i--) {
+ int n_chars = snprintf((char*)buffer->data + buffer->size_bytes, 21, "%09lu",
+ (unsigned long)segments[i]);
+ buffer->size_bytes += n_chars;
+ }
Review Comment:
Can you assert that `buffer->size_bytes` is not longer than the size allocated above?
##########
src/nanoarrow/utils_test.cc:
##########
@@ -233,6 +233,29 @@ TEST(DecimalTest, Decimal128Test) {
EXPECT_EQ(memcmp(decimal.words, bytes_neg, sizeof(bytes_neg)), 0);
}
+TEST(DecimalTest, DecimalNegateTest) {
+ struct ArrowDecimal decimal;
+
+ for (auto bitwidth : {128, 256}) {
+ ArrowDecimalInit(&decimal, bitwidth, 10, 3);
+ ArrowDecimalSetInt(&decimal, 12345);
Review Comment:
Can you test with larger numbers?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add decimal support to integration tester [arrow-nanoarrow]
Posted by "danepitkin (via GitHub)" <gi...@apache.org>.
danepitkin commented on code in PR #361:
URL: https://github.com/apache/arrow-nanoarrow/pull/361#discussion_r1455732373
##########
src/nanoarrow/nanoarrow.h:
##########
@@ -280,6 +284,14 @@ void ArrowLayoutInit(struct ArrowLayout* layout, enum ArrowType storage_type);
/// \brief Create a string view from a null-terminated string
static inline struct ArrowStringView ArrowCharView(const char* value);
+/// \brief Sets the integer value of an ArrowDecimal from a string
+ArrowErrorCode ArrowDecimalSetIntString(struct ArrowDecimal* decimal,
+ struct ArrowStringView value);
+
+/// \brief Get the integer value of an ArrowDecimal as string
+ArrowErrorCode ArrowDecimalAppendIntStringToBuffer(const struct ArrowDecimal* decimal,
+ struct ArrowBuffer* buffer);
+
Review Comment:
Even better, I love this change!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add decimal support to integration tester [arrow-nanoarrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #361:
URL: https://github.com/apache/arrow-nanoarrow/pull/361#issuecomment-1892027238
> Is this expected to work for Decimal256 as well?
Yes!
It would probably be more useful for ADBC if it handled the decimal point and a trailing exponent, but that should be fairly straightforward to add after the integer version.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add decimal support to integration tester [arrow-nanoarrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #361:
URL: https://github.com/apache/arrow-nanoarrow/pull/361#discussion_r1462434380
##########
src/nanoarrow/utils.c:
##########
@@ -223,3 +223,186 @@ struct ArrowBufferAllocator ArrowBufferDeallocator(
allocator.private_data = private_data;
return allocator;
}
+
+static const int kInt32DecimalDigits = 9;
+
+static const uint64_t kUInt32PowersOfTen[] = {
+ 1ULL, 10ULL, 100ULL, 1000ULL, 10000ULL,
+ 100000ULL, 1000000ULL, 10000000ULL, 100000000ULL, 1000000000ULL};
+
+// Adapted from Arrow C++ to use 32-bit words for better C portability
+// https://github.com/apache/arrow/blob/cd3321b28b0c9703e5d7105d6146c1270bbadd7f/cpp/src/arrow/util/decimal.cc#L524-L544
+static void ShiftAndAdd(struct ArrowStringView value, uint32_t* out, int64_t out_size) {
+ // We use strtoll for parsing, which needs input that is null-terminated
+ char chunk_string[16];
+
+ for (int64_t posn = 0; posn < value.size_bytes;) {
+ int64_t remaining = value.size_bytes - posn;
+
+ int64_t group_size;
+ if (remaining > kInt32DecimalDigits) {
+ group_size = kInt32DecimalDigits;
+ } else {
+ group_size = remaining;
+ }
+
+ const uint64_t multiple = kUInt32PowersOfTen[group_size];
+
+ memcpy(chunk_string, value.data + posn, group_size);
+ chunk_string[group_size] = '\0';
+ uint32_t chunk = (uint32_t)strtoll(chunk_string, NULL, 10);
+
+ for (int64_t i = 0; i < out_size; i++) {
+ uint64_t tmp = out[i];
+ tmp *= multiple;
+ tmp += chunk;
+ out[i] = (uint32_t)(tmp & 0xFFFFFFFFULL);
+ chunk = (uint32_t)(tmp >> 32);
+ }
+ posn += group_size;
+ }
+}
+
+ArrowErrorCode ArrowDecimalSetDigits(struct ArrowDecimal* decimal,
+ struct ArrowStringView value) {
+ // Check for sign
+ int is_negative = value.data[0] == '-';
+ int has_sign = is_negative || value.data[0] == '+';
+ value.data += has_sign;
+ value.size_bytes -= has_sign;
+
+ // Check all characters are digits that are not the negative sign
+ for (int64_t i = 0; i < value.size_bytes; i++) {
+ char c = value.data[i];
+ if (c < '0' || c > '9') {
+ return EINVAL;
+ }
+ }
+
+ // Skip over leading 0s
+ int64_t n_leading_zeroes = 0;
+ for (int64_t i = 0; i < value.size_bytes; i++) {
+ if (value.data[i] == '0') {
+ n_leading_zeroes++;
+ } else {
+ break;
+ }
+ }
+
+ value.data += n_leading_zeroes;
+ value.size_bytes -= n_leading_zeroes;
+
+ // Use 32-bit words for portability
+ uint32_t words32[8];
+ int n_words32 = decimal->n_words * 2;
+ memset(words32, 0, sizeof(words32));
+
+ ShiftAndAdd(value, words32, n_words32);
+
+ if (decimal->low_word_index == 0) {
+ memcpy(decimal->words, words32, sizeof(uint32_t) * n_words32);
+ } else {
+ uint64_t lo;
+ uint64_t hi;
+
+ for (int i = 0; i < decimal->n_words; i++) {
+ lo = (uint64_t)words32[i * 2];
+ hi = (uint64_t)words32[i * 2 + 1] << 32;
+ decimal->words[decimal->n_words - i - 1] = lo | hi;
+ }
+ }
+
+ if (is_negative) {
+ ArrowDecimalNegate(decimal);
+ }
+
+ return NANOARROW_OK;
+}
+
+// Adapted from Arrow C++ for C
+// https://github.com/apache/arrow/blob/cd3321b28b0c9703e5d7105d6146c1270bbadd7f/cpp/src/arrow/util/decimal.cc#L365
+ArrowErrorCode ArrowDecimalAppendDigitsToBuffer(const struct ArrowDecimal* decimal,
+ struct ArrowBuffer* buffer) {
+ int is_negative = ArrowDecimalSign(decimal) < 0;
+
+ uint64_t words_little_endian[4];
+ if (decimal->low_word_index == 0) {
+ memcpy(words_little_endian, decimal->words, decimal->n_words * sizeof(uint64_t));
+ } else {
+ for (int i = 0; i < decimal->n_words; i++) {
+ words_little_endian[i] = decimal->words[decimal->n_words - i - 1];
+ }
+ }
+
+ // We've already made a copy, so negate that if needed
+ if (is_negative) {
+ uint64_t carry = 1;
+ for (int i = 0; i < decimal->n_words; i++) {
+ uint64_t elem = words_little_endian[i];
+ elem = ~elem + carry;
+ carry &= (elem == 0);
+ words_little_endian[i] = elem;
+ }
+ }
+
+ int most_significant_elem_idx = -1;
+ for (int i = decimal->n_words - 1; i >= 0; i--) {
+ if (words_little_endian[i] != 0) {
+ most_significant_elem_idx = i;
+ break;
+ }
+ }
+
+ if (most_significant_elem_idx == -1) {
+ NANOARROW_RETURN_NOT_OK(ArrowBufferAppendInt8(buffer, '0'));
+ return NANOARROW_OK;
+ }
+
+ const uint32_t k1e9 = 1000000000U;
+ int num_segments = 0;
+ uint32_t segments[9];
+ memset(segments, 0, sizeof(segments));
+ uint64_t* most_significant_elem = words_little_endian + most_significant_elem_idx;
+
+ do {
Review Comment:
Done!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add decimal support to integration tester [arrow-nanoarrow]
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on PR #361:
URL: https://github.com/apache/arrow-nanoarrow/pull/361#issuecomment-1891227252
Very cool - this would be very helpful for arrow-adbc. Is this expected to work for Decimal256 as well?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add decimal support to integration tester [arrow-nanoarrow]
Posted by "danepitkin (via GitHub)" <gi...@apache.org>.
danepitkin commented on code in PR #361:
URL: https://github.com/apache/arrow-nanoarrow/pull/361#discussion_r1455727949
##########
src/nanoarrow/utils.c:
##########
@@ -223,3 +223,186 @@ struct ArrowBufferAllocator ArrowBufferDeallocator(
allocator.private_data = private_data;
return allocator;
}
+
+static const int kInt32DecimalDigits = 9;
+
+static const uint64_t kUInt32PowersOfTen[] = {
+ 1ULL, 10ULL, 100ULL, 1000ULL, 10000ULL,
+ 100000ULL, 1000000ULL, 10000000ULL, 100000000ULL, 1000000000ULL};
+
+// Adapted from Arrow C++ to use 32-bit words for better C portability
+// https://github.com/apache/arrow/blob/main/cpp/src/arrow/util/decimal.cc#L524-L544
+static void ShiftAndAdd(struct ArrowStringView value, uint32_t* out, int64_t out_size) {
+ // We use strtoll for parsing, which needs input that is null-terminated
+ char chunk_string[16];
+
+ for (int64_t posn = 0; posn < value.size_bytes;) {
+ int64_t remaining = value.size_bytes - posn;
+
+ int64_t group_size;
+ if (remaining > kInt32DecimalDigits) {
+ group_size = kInt32DecimalDigits;
+ } else {
+ group_size = remaining;
+ }
+
+ const uint64_t multiple = kUInt32PowersOfTen[group_size];
+
+ memcpy(chunk_string, value.data + posn, group_size);
+ chunk_string[group_size] = '\0';
+ uint32_t chunk = (uint32_t)strtoll(chunk_string, NULL, 10);
+
+ for (int64_t i = 0; i < out_size; i++) {
+ uint64_t tmp = out[i];
+ tmp *= multiple;
+ tmp += chunk;
+ out[i] = (uint32_t)(tmp & 0xFFFFFFFFULL);
+ chunk = (uint32_t)(tmp >> 32);
+ }
+ posn += group_size;
+ }
+}
+
+ArrowErrorCode ArrowDecimalSetIntString(struct ArrowDecimal* decimal,
+ struct ArrowStringView value) {
+ // Check for sign
+ int is_negative = value.data[0] == '-';
+ int has_sign = is_negative || value.data[0] == '+';
+ value.data += has_sign;
+ value.size_bytes -= has_sign;
+
+ // Check all characters are digits that are not the negative sign
+ for (int64_t i = 0; i < value.size_bytes; i++) {
+ char c = value.data[i];
+ if (c < '0' || c > '9') {
+ return EINVAL;
+ }
Review Comment:
ignore me then! This code is already very readable IMO. Other parts of the diff look like they are specifically written to avoid branching (e.g. `if` statements), so I wasn't sure if that was an active decision.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add decimal support to integration tester [arrow-nanoarrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot merged PR #361:
URL: https://github.com/apache/arrow-nanoarrow/pull/361
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add decimal support to integration tester [arrow-nanoarrow]
Posted by "danepitkin (via GitHub)" <gi...@apache.org>.
danepitkin commented on code in PR #361:
URL: https://github.com/apache/arrow-nanoarrow/pull/361#discussion_r1454071863
##########
src/nanoarrow/utils.c:
##########
@@ -223,3 +223,186 @@ struct ArrowBufferAllocator ArrowBufferDeallocator(
allocator.private_data = private_data;
return allocator;
}
+
+static const int kInt32DecimalDigits = 9;
+
+static const uint64_t kUInt32PowersOfTen[] = {
+ 1ULL, 10ULL, 100ULL, 1000ULL, 10000ULL,
+ 100000ULL, 1000000ULL, 10000000ULL, 100000000ULL, 1000000000ULL};
+
+// Adapted from Arrow C++ to use 32-bit words for better C portability
+// https://github.com/apache/arrow/blob/main/cpp/src/arrow/util/decimal.cc#L524-L544
Review Comment:
You can add a git hash in place of `main` if you'd like. (Press 'y' after opening the above link)
It also might be nice to copy the comment over as well if applicable.
##########
src/nanoarrow/nanoarrow.h:
##########
@@ -280,6 +284,14 @@ void ArrowLayoutInit(struct ArrowLayout* layout, enum ArrowType storage_type);
/// \brief Create a string view from a null-terminated string
static inline struct ArrowStringView ArrowCharView(const char* value);
+/// \brief Sets the integer value of an ArrowDecimal from a string
+ArrowErrorCode ArrowDecimalSetIntString(struct ArrowDecimal* decimal,
+ struct ArrowStringView value);
+
+/// \brief Get the integer value of an ArrowDecimal as string
+ArrowErrorCode ArrowDecimalAppendIntStringToBuffer(const struct ArrowDecimal* decimal,
+ struct ArrowBuffer* buffer);
+
Review Comment:
IMO the function names are a bit confusing. Maybe something like `ArrowDecimalSetFromString`? I think the `IntString` terminology is the part that is hard for me to parse.
##########
src/nanoarrow/utils.c:
##########
@@ -223,3 +223,186 @@ struct ArrowBufferAllocator ArrowBufferDeallocator(
allocator.private_data = private_data;
return allocator;
}
+
+static const int kInt32DecimalDigits = 9;
+
+static const uint64_t kUInt32PowersOfTen[] = {
+ 1ULL, 10ULL, 100ULL, 1000ULL, 10000ULL,
+ 100000ULL, 1000000ULL, 10000000ULL, 100000000ULL, 1000000000ULL};
+
+// Adapted from Arrow C++ to use 32-bit words for better C portability
+// https://github.com/apache/arrow/blob/main/cpp/src/arrow/util/decimal.cc#L524-L544
+static void ShiftAndAdd(struct ArrowStringView value, uint32_t* out, int64_t out_size) {
+ // We use strtoll for parsing, which needs input that is null-terminated
+ char chunk_string[16];
+
+ for (int64_t posn = 0; posn < value.size_bytes;) {
+ int64_t remaining = value.size_bytes - posn;
+
+ int64_t group_size;
+ if (remaining > kInt32DecimalDigits) {
+ group_size = kInt32DecimalDigits;
+ } else {
+ group_size = remaining;
+ }
+
+ const uint64_t multiple = kUInt32PowersOfTen[group_size];
+
+ memcpy(chunk_string, value.data + posn, group_size);
+ chunk_string[group_size] = '\0';
+ uint32_t chunk = (uint32_t)strtoll(chunk_string, NULL, 10);
+
+ for (int64_t i = 0; i < out_size; i++) {
+ uint64_t tmp = out[i];
+ tmp *= multiple;
+ tmp += chunk;
+ out[i] = (uint32_t)(tmp & 0xFFFFFFFFULL);
+ chunk = (uint32_t)(tmp >> 32);
+ }
+ posn += group_size;
+ }
+}
+
+ArrowErrorCode ArrowDecimalSetIntString(struct ArrowDecimal* decimal,
+ struct ArrowStringView value) {
+ // Check for sign
+ int is_negative = value.data[0] == '-';
+ int has_sign = is_negative || value.data[0] == '+';
+ value.data += has_sign;
+ value.size_bytes -= has_sign;
+
+ // Check all characters are digits that are not the negative sign
+ for (int64_t i = 0; i < value.size_bytes; i++) {
+ char c = value.data[i];
+ if (c < '0' || c > '9') {
+ return EINVAL;
+ }
Review Comment:
this might be an unhelpful guess at micro-optimization.. but if you are trying to avoid branching, you can set a bitmask here and check for failure outside of the loop. e.g. `is_invalid &= c < '0' || c > '9';`. Maybe the compiler would optimize this for us? I wouldn't know.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add decimal support to integration tester [arrow-nanoarrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #361:
URL: https://github.com/apache/arrow-nanoarrow/pull/361#discussion_r1454223782
##########
src/nanoarrow/utils.c:
##########
@@ -223,3 +223,186 @@ struct ArrowBufferAllocator ArrowBufferDeallocator(
allocator.private_data = private_data;
return allocator;
}
+
+static const int kInt32DecimalDigits = 9;
+
+static const uint64_t kUInt32PowersOfTen[] = {
+ 1ULL, 10ULL, 100ULL, 1000ULL, 10000ULL,
+ 100000ULL, 1000000ULL, 10000000ULL, 100000000ULL, 1000000000ULL};
+
+// Adapted from Arrow C++ to use 32-bit words for better C portability
+// https://github.com/apache/arrow/blob/main/cpp/src/arrow/util/decimal.cc#L524-L544
+static void ShiftAndAdd(struct ArrowStringView value, uint32_t* out, int64_t out_size) {
+ // We use strtoll for parsing, which needs input that is null-terminated
+ char chunk_string[16];
+
+ for (int64_t posn = 0; posn < value.size_bytes;) {
+ int64_t remaining = value.size_bytes - posn;
+
+ int64_t group_size;
+ if (remaining > kInt32DecimalDigits) {
+ group_size = kInt32DecimalDigits;
+ } else {
+ group_size = remaining;
+ }
+
+ const uint64_t multiple = kUInt32PowersOfTen[group_size];
+
+ memcpy(chunk_string, value.data + posn, group_size);
+ chunk_string[group_size] = '\0';
+ uint32_t chunk = (uint32_t)strtoll(chunk_string, NULL, 10);
+
+ for (int64_t i = 0; i < out_size; i++) {
+ uint64_t tmp = out[i];
+ tmp *= multiple;
+ tmp += chunk;
+ out[i] = (uint32_t)(tmp & 0xFFFFFFFFULL);
+ chunk = (uint32_t)(tmp >> 32);
+ }
+ posn += group_size;
+ }
+}
+
+ArrowErrorCode ArrowDecimalSetIntString(struct ArrowDecimal* decimal,
+ struct ArrowStringView value) {
+ // Check for sign
+ int is_negative = value.data[0] == '-';
+ int has_sign = is_negative || value.data[0] == '+';
+ value.data += has_sign;
+ value.size_bytes -= has_sign;
+
+ // Check all characters are digits that are not the negative sign
+ for (int64_t i = 0; i < value.size_bytes; i++) {
+ char c = value.data[i];
+ if (c < '0' || c > '9') {
+ return EINVAL;
+ }
Review Comment:
I'm not sure I follow...I wasn't intending to optimize anything, just to make sure we had a valid integer string before proceeding. The other option is to pass `end_ptr` instead of `NULL` into `strtoll()` and check that it parsed the entire chunk of characters. Maybe that would be less confusing?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add decimal support to integration tester [arrow-nanoarrow]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #361:
URL: https://github.com/apache/arrow-nanoarrow/pull/361#issuecomment-1889832185
## [Codecov](https://app.codecov.io/gh/apache/arrow-nanoarrow/pull/361?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Comparison is base [(`931eafc`)](https://app.codecov.io/gh/apache/arrow-nanoarrow/commit/931eafc995267817b25ca220b4022497d5d6a677?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 88.19% compared to head [(`77f9263`)](https://app.codecov.io/gh/apache/arrow-nanoarrow/pull/361?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 86.79%.
> Report is 1 commits behind head on main.
> :exclamation: Current head 77f9263 differs from pull request most recent head 8693900. Consider uploading reports for the commit 8693900 to get more accurate results
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #361 +/- ##
==========================================
- Coverage 88.19% 86.79% -1.40%
==========================================
Files 74 6 -68
Lines 12212 212 -12000
==========================================
- Hits 10770 184 -10586
+ Misses 1442 28 -1414
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/arrow-nanoarrow/pull/361?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add decimal support to integration tester [arrow-nanoarrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #361:
URL: https://github.com/apache/arrow-nanoarrow/pull/361#discussion_r1455601006
##########
src/nanoarrow/nanoarrow.h:
##########
@@ -280,6 +284,14 @@ void ArrowLayoutInit(struct ArrowLayout* layout, enum ArrowType storage_type);
/// \brief Create a string view from a null-terminated string
static inline struct ArrowStringView ArrowCharView(const char* value);
+/// \brief Sets the integer value of an ArrowDecimal from a string
+ArrowErrorCode ArrowDecimalSetIntString(struct ArrowDecimal* decimal,
+ struct ArrowStringView value);
+
+/// \brief Get the integer value of an ArrowDecimal as string
+ArrowErrorCode ArrowDecimalAppendIntStringToBuffer(const struct ArrowDecimal* decimal,
+ struct ArrowBuffer* buffer);
+
Review Comment:
It floated in last night that `Digits` is probably a better term. I pushed that change but feel free to suggest something else!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add decimal support to integration tester [arrow-nanoarrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #361:
URL: https://github.com/apache/arrow-nanoarrow/pull/361#discussion_r1454229386
##########
src/nanoarrow/nanoarrow.h:
##########
@@ -280,6 +284,14 @@ void ArrowLayoutInit(struct ArrowLayout* layout, enum ArrowType storage_type);
/// \brief Create a string view from a null-terminated string
static inline struct ArrowStringView ArrowCharView(const char* value);
+/// \brief Sets the integer value of an ArrowDecimal from a string
+ArrowErrorCode ArrowDecimalSetIntString(struct ArrowDecimal* decimal,
+ struct ArrowStringView value);
+
+/// \brief Get the integer value of an ArrowDecimal as string
+ArrowErrorCode ArrowDecimalAppendIntStringToBuffer(const struct ArrowDecimal* decimal,
+ struct ArrowBuffer* buffer);
+
Review Comment:
I agree that it's not great! I think the reason I didn't pick `ArrowDecimalSetFromString()` is because to me that would parse the decimal point and this function won't do that. It will also error very quickly if anybody tries to put a decimal point there and I'm game to change it.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add decimal support to integration tester [arrow-nanoarrow]
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on PR #361:
URL: https://github.com/apache/arrow-nanoarrow/pull/361#issuecomment-1892973484
> It would probably be more useful for ADBC if it handled the decimal point and a trailing exponent
The postgres implementation in ADBC currently sends the string and decimal placement separately, so this would be helpful for that without going all the way
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org