lidavidm commented on code in PR #3787:
URL: https://github.com/apache/arrow-adbc/pull/3787#discussion_r2638908238
##########
c/driver/postgresql/copy/writer.h:
##########
@@ -234,48 +235,101 @@ class PostgresCopyNumericFieldWriter : public
PostgresCopyFieldWriter {
// Number of decimal digits per Postgres digit
constexpr int kDecDigits = 4;
std::vector<int16_t> pg_digits;
- int16_t weight = -(scale_ / kDecDigits);
- int16_t dscale = scale_;
- bool seen_decimal = scale_ == 0;
- bool truncating_trailing_zeros = true;
+ int16_t weight;
+ int16_t dscale;
char decimal_string[max_decimal_digits_ + 1];
- int digits_remaining = DecimalToString<bitwidth_>(&decimal,
decimal_string);
- do {
- const int start_pos =
- digits_remaining < kDecDigits ? 0 : digits_remaining - kDecDigits;
- const size_t len = digits_remaining < 4 ? digits_remaining : kDecDigits;
- const std::string_view substr{decimal_string + start_pos, len};
- int16_t val{};
- std::from_chars(substr.data(), substr.data() + substr.size(), val);
+ int total_digits = DecimalToString<bitwidth_>(&decimal, decimal_string);
- if (val == 0) {
- if (!seen_decimal && truncating_trailing_zeros) {
- dscale -= kDecDigits;
- }
- } else {
- pg_digits.insert(pg_digits.begin(), val);
- if (!seen_decimal && truncating_trailing_zeros) {
- if (val % 1000 == 0) {
- dscale -= 3;
- } else if (val % 100 == 0) {
- dscale -= 2;
- } else if (val % 10 == 0) {
- dscale -= 1;
- }
+ const int n_int_digits = total_digits > scale_ ? total_digits - scale_ : 0;
+ int n_frac_digits = total_digits > n_int_digits ? total_digits -
n_int_digits : 0;
+
+ std::string_view decimal_string_view(decimal_string, total_digits);
+ std::string_view int_part = decimal_string_view.substr(0, n_int_digits);
+
+ std::string frac_part_str;
+ if (n_int_digits == 0 && total_digits < scale_) {
+ frac_part_str.assign(scale_ - total_digits, '0');
+ frac_part_str.append(decimal_string, total_digits);
+ n_frac_digits = scale_;
+ } else {
+ frac_part_str.assign(decimal_string_view.substr(n_int_digits,
n_frac_digits));
+ }
+ std::string_view frac_part(frac_part_str);
+
+ // Count trailing zeros in the fractional part to minimize dscale
+ int actual_trailing_zeros = 0;
+ for (int j = frac_part.length() - 1; j >= 0 && frac_part[j] == '0'; j--) {
+ actual_trailing_zeros++;
+ }
+
+ // Group integer part
+ int i = int_part.length();
+ std::vector<int16_t> int_digits;
+ int n_int_digit_groups = 0;
+ if (i > 0) {
+ // Calculate weight based on original integer length
+ weight = (i + kDecDigits - 1) / kDecDigits - 1;
+
+ while (i > 0) {
+ int chunk_size = std::min(i, kDecDigits);
+ std::string_view chunk = int_part.substr(i - chunk_size, chunk_size);
+ int16_t val{};
+ std::from_chars(chunk.data(), chunk.data() + chunk.size(), val);
+ // Skip trailing zeros in integer part (which appear first when
processing
+ // right-to-left)
+ if (val != 0 || !int_digits.empty()) {
+ int_digits.insert(int_digits.begin(), val);
}
- truncating_trailing_zeros = false;
+ i -= chunk_size;
}
- digits_remaining -= kDecDigits;
- if (digits_remaining <= 0) {
- break;
- }
- weight++;
+ n_int_digit_groups = int_digits.size();
+ pg_digits.insert(pg_digits.end(), int_digits.begin(), int_digits.end());
+ } else {
+ weight = -1;
+ n_int_digit_groups = 0;
+ }
+
+ // Group fractional part
+ // Chunk in 4-digit groups, padding the LAST group on the right if needed
+ i = 0;
+ bool skip_leading_zeros = (n_int_digits == 0);
+
+ while (i < (int)frac_part.length()) {
+ int chunk_size = std::min((int)frac_part.length() - i, kDecDigits);
Review Comment:
nit: don't use c-style casts
##########
c/driver/postgresql/copy/postgres_copy_writer_test.cc:
##########
@@ -500,6 +500,180 @@ TEST_F(PostgresCopyTest, PostgresCopyWriteNumeric) {
}
}
+// Regression test for bug where 44.123456 with Decimal(10,6) became
4412.345500
+// COPY (SELECT CAST(col AS NUMERIC) AS col FROM ( VALUES (44.123456),
+// (0.123456), (123.456789)) AS drvd(col)) TO STDOUT WITH (FORMAT binary);
Review Comment:
It would be good if we also tested max, min, 0, and no fractional digits
(`1234.000000`) for each of these cases
##########
c/driver/postgresql/copy/postgres_copy_writer_test.cc:
##########
Review Comment:
Can we test scale=0, and negative scales too?
--
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]
