Skip to content

Fix the NpgsqlCube to use the full G17 floating-point format#6295

Merged
roji merged 2 commits intonpgsql:mainfrom
kirkbrauer:fix-cube-write
Nov 7, 2025
Merged

Fix the NpgsqlCube to use the full G17 floating-point format#6295
roji merged 2 commits intonpgsql:mainfrom
kirkbrauer:fix-cube-write

Conversation

@kirkbrauer
Copy link
Contributor

  • Switch cube floating-point string to use G17 format for 17 digits of precision to follow IEEE 754

@kirkbrauer
Copy link
Contributor Author

kirkbrauer commented Nov 6, 2025

@roji one question I did have is if we should remove the redundant ToSubset() method in the NpgsqlCube now that we have the Subset() extension method. It might be confusing to users to have both, but only one can be translated. A good intermediate could be to just rename it to Subset() which would violate the naming convention, but at least it would be consistent and not introduce another method.

@NinoFloris any thoughts on this?

@kirkbrauer kirkbrauer changed the title Fix the Write() method to use the full G17 floating-point format Fix the NpgsqlCube to use the full G17 floating-point format Nov 6, 2025
Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

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

one question I did have is if we should remove the redundant ToSubset() method in the NpgsqlCube now that we have the Subset() extension method.

I don't think the API of NpgsqlCube should be changed as a function of what we're doing in the EF provider - there may be Npgsql users which don't use EF and want to interact with Cube. So if we think that a client-side implementation of Subset makes sense to have, we should keep it in Npgsql.

Besides, given the conclusion we seem to have gotten to in npgsql/efcore.pg#3651 (comment)), do we even need an extension method in the EF provider? Why not just translate the ToSubset method on NpgsqlCube?

Unrelated: I noticed that NpgsqlCube.cs uses a namespace block instead of a statement, can you please change that in this PR as well to clean up the code? All of Npgsql uses namespace statements.

@kirkbrauer
Copy link
Contributor Author

@roji Ok, I think we will unify on the ToSubset() method instead and just translate it when we find it in efcore.pg and remove the extension method.

@roji roji merged commit 33d8cdc into npgsql:main Nov 7, 2025
13 checks passed
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.

3 participants