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.

Sign up for GitHub

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

Jump to bottom

Fix DB::ColumnTypeMismatchError for Int32 columns in CockroachDB #920

Merged
merged 1 commit into from
Jan 16, 2023
Merged

Fix DB::ColumnTypeMismatchError for Int32 columns in CockroachDB #920

merged 1 commit into from
Jan 16, 2023

Conversation

akadusei
Copy link
Contributor

Fixes #919

There's no specs, though; can't think of any.

@akadusei
Fix DB::ColumnTypeMismatchError for Int32 columns in CockroachDB
30a118a 
Fixes luckyframework#919

```
In PG::ResultSet#read the column one returned a Int64 but a Int32 was expected. (DB::ColumnTypeMismatchError)
    from lib/db/src/db/result_set.cr:89:9 in 'read'
    from lib/db/src/db/query_methods.cr:202:9 in 'query_one?:as:args'
    from lib/avram/src/avram/database.cr:88:3 in 'query_one?:as:args:queryable'
    from lib/avram/src/avram/database.cr:88:3 in 'query_one?:args:queryable:as'
    from lib/avram/src/avram/queryable.cr:224:17 in 'any?'
    # ...
```
@akadusei
Copy link
Contributor Author

Don't merge yet. I would like to confirm first that this works on CockroachDB...

@akadusei
Copy link
Contributor Author

My bad, but this was actually fixed in #900. I'm running the same tests against Avram master, and the issue does not exist.

So this PR is not really necessary. You may still merge, though, to forestall any surprises in future.

@jwoertink
Copy link
Member

Ah, cool. That's good then. I'm trying to get another release out, but still hung up on a few things.

Looking at the pg docs, it seems they're just aliases, but maybe using int4 specifically will just be more explicit for other PG-like adapters. I'm cool with merging it if it's not gonna affect anything. Like, if I have an existing app which uses int on one table and int4 on another, it should still all work the same, right?

@akadusei
Copy link
Contributor Author

Sorry, again, but #900 did not fix this wholly, so this PR is still needed. Without this change, some specs in another app using Bill fail with the same error.

@jwoertink
Copy link
Member

I assume those specs are all passing with this? Do you think we need to change bigint to int8 and smallint to int2? Or was it just that int was ambiguous and cockroach treated it differently?

@jwoertink
Copy link
Member

It would be great if we could just use CockroachDB in the CI specs. Looks like it may not be possible just yet? cockroachdb/cockroach#87043

@akadusei
Copy link
Contributor Author

Do you think we need to change bigint to int8 and smallint to int2?

No, its just int and serial . All other types work OK, from what I've tested so far.

@akadusei
Copy link
Contributor Author

akadusei commented Jan 16, 2023 •
edited
Loading

It would be great if we could just use CockroachDB in the CI specs.

Yeah, I was thinking same, but I wasn't sure. That should help a lot.

@akadusei
Copy link
Contributor Author

There's still one last issue with #select_sum methods. In CockroachDB, they return PG::Numeric leading to this error:

cast from PG::Numeric to (Int64 | Nil) failed, at /path/to/lib/avram/src/avram/charms/int32_extensions.cr:51:9:51 (TypeCastError)
    from lib/avram/src/avram/charms/int32_extensions.cr:51:9 in 'select_sum'
    from lib/avram/src/avram/charms/int32_extensions.cr:55:9 in 'select_sum!'
    # ...

I'm pushing that as a separate PR.

@akadusei akadusei mentioned this pull request Jan 16, 2023
Merged
@jwoertink jwoertink merged commit 6d5357e into luckyframework:main Jan 16, 2023
@akadusei akadusei deleted the fix/919 branch July 24, 2024 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jwoertink jwoertink jwoertink approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Getting DB::ColumnTypeMismatchError for Int32 columns in CockroachDB
2 participants
@akadusei @jwoertink