Skip to content

Fix converting rows with FLOAT4 and BOOLEAN #213

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

Merged
merged 2 commits into from
Feb 28, 2023
Merged

Fix converting rows with FLOAT4 and BOOLEAN #213

merged 2 commits into from
Feb 28, 2023

Conversation

iwysiu
Copy link
Contributor

@iwysiu iwysiu commented Feb 24, 2023

From investigating with our external dev credentials, it looks like we were converting Float4 and Boolean from the wrong types. When I set the redshift column to the REAL type, it returned a Float4, so I don't think our issue was that we weren't supporting the type. Also, I suspect that the FLOAT type should also be converted as a double, but in Redshift it's synonymous with DOUBLE and I couldn't get it to actually return that type.
I'm a little suspicious of the solution because it implies either that FLOAT4 has never worked or that AWS changed the return type.

Fixes #204

@github-actions
Copy link

Backend code coverage report for PR #213

Plugin Main PR Difference
redshift 24.0% 24.0% 0%
@github-actions
Copy link

Frontend code coverage report for PR #213
No changes

@iwysiu iwysiu marked this pull request as ready for review February 24, 2023 16:43
@iwysiu iwysiu requested a review from a team as a code owner February 24, 2023 16:43
@fridgepoet
Copy link
Contributor

fridgepoet commented Feb 27, 2023

Nice finds!

Do you think we should just consider FLOAT the same case as FLOAT8? It's what I'm seeing for the DOUBLE PRECISION column type.

I'm a little suspicious of the solution because it implies either that FLOAT4 has never worked or that AWS changed the return type.

Same about the boolean, right?

By the way, for future reference if we want to test this out, it's much easier to write a query like SELECT CAST('123456' AS REAL); than to create a cluster/table/column of a certain type.

@@ -126,13 +126,14 @@ func Test_convertRow(t *testing.T) {
{
name: "numeric type float4",
metadata: &redshiftdataapiservice.ColumnMetadata{
Name: aws.String("other"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wondering out loud: Regarding all the special checks for time column (#62), I wonder why we don't ever do it for other "number" types. Or why we don't somehow ensure it is PostgresSQL before just going for the assumption it is Unix time.

@iwysiu iwysiu merged commit cdf36fd into main Feb 28, 2023
@iwysiu iwysiu deleted the iwysiu/204 branch February 28, 2023 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants