Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support float4 data type #1481

Merged
merged 19 commits into from
Mar 11, 2024
Merged

feat: support float4 data type #1481

merged 19 commits into from
Mar 11, 2024

Conversation

olavloite
Copy link
Collaborator

@olavloite olavloite commented Mar 4, 2024

Adds support for the float4 / real data type.

Integration tests use float8 on environments that do not yet support float4.

@@ -212,8 +214,6 @@ private static Object toValidSpannerElement(@Nonnull Object value, int elementOi
return ((Short) value).longValue();
case Oid.INT4:
return ((Integer) value).longValue();
case Oid.FLOAT4:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is being removed now, as these are data types that needed special handling because:

  1. We allow PostgreSQL clients to send us int2[], int4[] and float4[] arrays.
  2. These are converted to int8[] and float8[] before being sent to Cloud Spanner.
  3. This conversion is no longer needed for float4 when Cloud Spanner supports float4 natively.

@@ -384,7 +384,6 @@ public class PgType implements PgCatalogTable {
+ " || (case typname\n"
+ " when 'int2' then 'false'\n"
+ " when 'int4' then 'false'\n"
+ " when 'float4' then 'false'\n"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the 'typisdefined' column in pg_type. Up until now we listed float4 as a 'type that we know, but that is not implemented'.

+ this.tableColumns.keySet().size()
+ " columns, but only found "
+ record.numColumns(),
String.format(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This change is not directly related to the addition of float4, but it popped up during the development that the error message was a bit weird. The error is thrown when the number of columns in the input of the COPY operation is different from the expected number of columns. The error message 'assumed' that the number of input columns was always smaller than the expected number of values (see the '... but only found' part).

@olavloite olavloite added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Mar 7, 2024
@olavloite olavloite marked this pull request as ready for review March 7, 2024 09:50
@olavloite olavloite requested a review from ankiaga March 7, 2024 09:50
Copy link

codecov bot commented Mar 7, 2024

Codecov Report

Attention: Patch coverage is 94.28571% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 90.79%. Comparing base (16eb184) to head (fbe73b1).
Report is 10 commits behind head on postgresql-dialect.

Files Patch % Lines
...e/cloud/spanner/pgadapter/parsers/FloatParser.java 83.33% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@                   Coverage Diff                    @@
##             postgresql-dialect    #1481      +/-   ##
========================================================
+ Coverage                 90.72%   90.79%   +0.07%     
- Complexity                 2684     2701      +17     
========================================================
  Files                       144      144              
  Lines                      9226     9249      +23     
  Branches                   1342     1343       +1     
========================================================
+ Hits                       8370     8398      +28     
+ Misses                      578      574       -4     
+ Partials                    278      277       -1     
Flag Coverage Δ
all_tests 90.79% <94.28%> (+0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

@arawind arawind left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@olavloite olavloite removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Mar 11, 2024
@olavloite olavloite merged commit c2554fc into postgresql-dialect Mar 11, 2024
43 checks passed
@olavloite olavloite deleted the float32 branch March 11, 2024 18:42
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.

2 participants