feat(c/driver/postgresql): Improve conversion of decimal to Postgresql numeric and add tests #3787
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…esql numeric format logic and add test for different scales
lidavidm
reviewed
Dec 22, 2025
lidavidm
reviewed
Dec 29, 2025
Member
lidavidm
left a comment
lidavidm
left a comment
There was a problem hiding this comment.
The general comment I have is that it's rather hard to follow the code (albeit it was hard to follow before too). Commenting the parts and perhaps providing sample values at each stage/explaining what's going on would be valuable. It may also be good to summarize the Postgres representation in here
| int effective_scale = scale_; | ||
| if (scale_ < 0) { | ||
| int zeros_to_append = -scale_; | ||
| memset(decimal_string + total_digits, '0', zeros_to_append); |
Member
There was a problem hiding this comment.
Suggested change
| memset(decimal_string + total_digits, '0', zeros_to_append); | |
| std::memset(decimal_string + total_digits, '0', zeros_to_append); |
nit
| const std::string_view substr{decimal_string + start_pos, len}; | ||
| int total_digits = DecimalToString<bitwidth_>(&decimal, decimal_string); | ||
|
|
||
| // Handle negative scale by appending zeros |
Member
There was a problem hiding this comment.
Is there a way to handle scale without needing to append zeroes? The Postgres decimal representation should store this separately, so I thought we would only need to convert the integer part and then set dscale/weight appropriately.
| bool seen_decimal = scale_ == 0; | ||
| bool truncating_trailing_zeros = true; | ||
| int16_t weight; | ||
| int16_t dscale; |
Member
There was a problem hiding this comment.
Suggested change
| int16_t dscale; | |
| // "decimal scale". Number of digits after the decimal point (>=0) | |
| // dscale may be more than the actual number of stored digits, | |
| // implying there are significant zeroes that were not stored | |
| int16_t dscale; |
| int16_t dscale = scale_; | ||
| bool seen_decimal = scale_ == 0; | ||
| bool truncating_trailing_zeros = true; | ||
| int16_t weight; |
Member
There was a problem hiding this comment.
Suggested change
| int16_t weight; | |
| // There are `weight + 1` base 10000 digits before the decimal point | |
| // (may be negative) | |
| int16_t weight; |
lidavidm
pushed a commit
that referenced
this pull request
Dec 29, 2025
…3821) This PR contains: - Setup validataion suite for the postgresql driver. (128 tests pass. 0 failing. Decimal ingest test, and decimal bind tests were skipped due to bug and the fix has not been merged yet. #3787) - PostgreSQL specific test overrides - Update postgresql integration test in CI workflow to run validation tests - Update RAT license exlusions file to ignore the .lock files and test sql files. There was a small change made in the c/driver/postgresql/result_helper.cc due to the segfault when the number of query parameters doesn't match the number of result columns. The test that triggered the bug was calling get_parameter_schema() on `SELECT 1 + $1 + $2` . This query has 2 parameters but returns only 1 result column (the computed sum). The code was incorrectly using PQfname(result_, i) to retrieve parameter names, but PQfname() actually retrieves result column names from the result set. When iterating through parameters, the first iteration (i=0) would work fine accessing column 0, but the second iteration (i=1) would access an out-of-range column. PQfname() returns NULL for out-of-range column numbers, and passing this NULL pointer to AppendChild() as a C string causes the segfault.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The logic for converting Arrow Decimal type to the PostgreSQL has been refactored to fix the data not being inserted correctly when the scale is not a multiple of 4.
Adds new test cases covering various scales and zero padding scenarios.
Closes #3485