Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Add BigInteger type support for PG numeric type - #3646

#3646
Merged
roji merged 6 commits into
npgsql:mainnpgsql/npgsql:mainfrom
Emill:BigInteger-numericEmill/Npgsql:BigInteger-numericCopy head branch name to clipboard
Apr 14, 2021
Merged

Add BigInteger type support for PG numeric type#3646
roji merged 6 commits into
npgsql:mainnpgsql/npgsql:mainfrom
Emill:BigInteger-numericEmill/Npgsql:BigInteger-numericCopy head branch name to clipboard

Conversation

@Emill

@Emill Emill commented Apr 9, 2021

Copy link
Copy Markdown
Contributor

Partial fix for #448.

Allows the reading/writing of numerics that don't have any fractional bits (but all fractional bits are zero is allowed).

@Emill
Emill requested review from roji and vonzshik as code owners April 9, 2021 23:32
@Emill

Emill commented Apr 9, 2021

Copy link
Copy Markdown
Contributor Author

When we abort reading because we don't accept the data, such as too big values when reading as System.Decimal, should we just throw or first make sure to Skip to the end of the buffer?

@roji roji left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I didn't inspect the math too closely (:sweat_smile:) but looks good! See comments.

Allows the reading/writing of numerics that don't have any fractional bits (but all fractional bits are zero is allowed).

Maybe add tests for these two cases (with all-zero fractional -> ok, with non-all-zero fractional -> correct exception)?

When we abort reading because we don't accept the data, such as too big values when reading as System.Decimal, should we just throw or first make sure to Skip to the end of the buffer?

You no longer have to handle this in the handler - NpgsqlDataReader already contains the logic to skip to the end of the value as long as the connection isn't broken (you can look in GetFieldValue)

Comment thread src/Npgsql/Internal/TypeHandlers/NumericHandlers/NumericHandler.cs Outdated
Comment thread src/Npgsql/Internal/TypeHandlers/NumericHandlers/NumericHandler.cs Outdated
Comment thread src/Npgsql/Internal/TypeHandlers/NumericHandlers/NumericHandler.cs
Comment thread src/Npgsql/Internal/TypeHandlers/NumericHandlers/NumericHandler.cs
Comment thread src/Npgsql/Internal/TypeHandlers/NumericHandlers/NumericHandler.cs
Comment thread src/Npgsql/Internal/TypeHandlers/NumericHandlers/NumericHandler.cs
Comment thread src/Npgsql/Internal/TypeHandlers/NumericHandlers/NumericHandler.cs Outdated
Comment thread src/Npgsql/Internal/TypeHandlers/NumericHandlers/NumericHandler.cs
Comment thread src/Npgsql/Internal/TypeHandlers/NumericHandlers/NumericHandler.cs
Comment thread test/Npgsql.Tests/Types/NumericTests.cs Outdated
@roji

roji commented Apr 10, 2021

Copy link
Copy Markdown
Member

/cc @YohDeadfall in case you're interested in this

@Emill

Emill commented Apr 10, 2021

Copy link
Copy Markdown
Contributor Author

Maybe add tests for these two cases (with all-zero fractional -> ok, with non-all-zero fractional -> correct exception)?

The tests contain such cases.

When we abort reading because we don't accept the data, such as too big values when reading as System.Decimal, should we just throw or first make sure to Skip to the end of the buffer?

You no longer have to handle this in the handler - NpgsqlDataReader already contains the logic to skip to the end of the value as long as the connection isn't broken (you can look in GetFieldValue)

Does

var writtenBytes = Buffer.ReadPosition - position;
really work if the handler caused the buffer to be flushed and now contains the next chunk of data?

@roji

roji commented Apr 10, 2021

Copy link
Copy Markdown
Member

really work if the handler caused the buffer to be flushed and now contains the next chunk of data?

Good catch - that indeed seems like a bug... There's an assumption in the error-handling code that the type handler hasn't cleared the buffer and repopulated it... I suspect this hasn't been a problem because most recoverable errors (i.e. which don't break the connection) occur at the start of the reading (or we just haven't had cases where they happen to occur after another buffer read).

Fixing this generally at the NpgsqlDataReader seems very tricky, since we're not aware at all the I/O/buffer operations happening below in the type handler, and can't know how much was read in the column. We may need to go back to handling this at the type handler level (/cc @YohDeadfall), though it's seems somewhat of a corner case that only occurs in negative cases anyway (i.e. this is just about the connection not losing protocol sync after an exception is thrown).

I suggest we scope it out of this PR, unless you feel like handling it in the NumericHandler... though can you open a new issue?

@roji

roji commented Apr 14, 2021

Copy link
Copy Markdown
Member

@Emill just to make sure, when you want me to review again don't forget to request that (the arrows icon in the reviewers tab above). It's also helpful to click "Resolve conversation" on things which have already been dealt with.

@Emill
Emill requested a review from roji April 14, 2021 20:49
@Emill

Emill commented Apr 14, 2021

Copy link
Copy Markdown
Contributor Author

I think all outstanding comments have been resolved now :)

@roji roji left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ready (just a couple of nits). Thanks for this @Emill!

Comment thread src/Npgsql/Internal/TypeHandlers/NumericHandlers/NumericHandler.cs
Comment thread test/Npgsql.Tests/Types/NumericTests.cs
@roji
roji merged commit 3b0b0df into npgsql:main Apr 14, 2021
@roji

roji commented Apr 14, 2021

Copy link
Copy Markdown
Member

Thanks @Emill!

@Emill
Emill deleted the BigInteger-numeric branch April 14, 2021 22:48
@roji roji linked an issue Apr 14, 2021 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support mapping PostgreSQL decimal to BigInteger

2 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.