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

Type suggestions broken #3105

Closed
seancolsen opened this issue Jul 27, 2023 · 14 comments
Closed

Type suggestions broken #3105

seancolsen opened this issue Jul 27, 2023 · 14 comments
Labels
priority: urgent Blocks other ongoing work regression restricted: maintainers Only maintainers can resolve this issue type: bug Something isn't working work: backend Related to Python, Django, and simple SQL
Milestone

Comments

@seancolsen
Copy link
Contributor

seancolsen commented Jul 27, 2023

Steps to reproduce

  1. Import birds.csv

  2. Expect numerical columns to be inferred as "Number".

    image

  3. Observe all columns are inferred as "Text".

    image

This is because the response from the /api/db/v0/tables/3017/type_suggestions/ endpoint is:

{
  "Common name": "text",
  "Binomial name": "text",
  "Average mass kg": "text",
  "Max mass kg": "text",
  "Flighted": "text"
}

I'm not able to reproduce this on 73ca8e9. Then immediately after #3097 was merged, I am able to reproduce this on 1edd461.

CC @mathemancer who authored #3097 and @dmos62 who reviewed it.

This regression makes my work on optional type inference a bit difficult.

@seancolsen seancolsen added ready Ready for implementation type: bug Something isn't working work: backend Related to Python, Django, and simple SQL restricted: maintainers Only maintainers can resolve this issue regression labels Jul 27, 2023
@seancolsen seancolsen added this to the v0.1.3 milestone Jul 27, 2023
@seancolsen seancolsen added the priority: urgent Blocks other ongoing work label Jul 27, 2023
@seancolsen
Copy link
Contributor Author

Ok, what the heck? Type suggestions are working for me again! Something strange is going on. I've tried on exactly the same commit. No code changes. I've tried exactly the same steps. Different behavior. I'm going to leave this ticket open in case someone might have some insight. But it's possible I ended up in some kind of weird state that is gone now.

@spapas
Copy link
Contributor

spapas commented Jul 28, 2023

Hey friends, if it helps I tried importing the provided birds.csv on my dev instance (commit 59e6e76) and all columns are also imported as text.

@spapas
Copy link
Contributor

spapas commented Jul 28, 2023

Also, when I tried converting one of the text columns to a decimal I got the following error in a red popup at the top right:

function msar.alter_columns(integer, unknown) does not exist LINE 1: SELECT msar.alter_columns($1,$2) ^ HINT: No function matches the given name and argument types. You might need to add explicit type casts.

I tried changing the Average mass kg column to a Decimal(5,3)

Scratch that, after following the instructions below it works.

@seancolsen
Copy link
Contributor Author

Oh, that's very helpful @spapas. Thanks!

@dmos62
Copy link
Contributor

dmos62 commented Jul 28, 2023

I had a, maybe related problem, where I had to --force-recreate during docker compose up, so that the new functions would be installed. @seancolsen is it possible that our Postgres libraries hadn't updated when you experienced those bugs and then they did when you stopped experiencing them? If so, the problem is with our Postgres libraries not updating automatically. We talked about that briefly with @mathemancer yesterday. I don't have a solution atm.

@seancolsen
Copy link
Contributor Author

seancolsen commented Jul 28, 2023

Ok interesting hypothesis @dmos62.

I performed the following actions in an attempt to corroborate your hypothesis...

  1. git checkout 73ca8e90f7ec6e60f700c287762ca7c4040a18b9

  2. sudo docker compose -f docker-compose.yml -f docker-compose.dev.yml up dev-service --force-recreate --build dev-service

    (Now I'm at a clean starting point before Add DDL functions for altering columns #3097.)

  3. Run reproduction steps... 👌 cannot reproduce

  4. git checkout 1edd461b50f51ead45ffa848ce67dddf099cb1d0

    (Now I've merged Add DDL functions for altering columns #3097 but not recreated the container.)

  5. Run reproduction steps... 👌 cannot reproduce

  6. sudo docker compose -f docker-compose.yml -f docker-compose.dev.yml up dev-service --force-recreate --build dev-service

    (Now I've also recreated the container.)

  7. Run reproduction steps... 👌 cannot reproduce

I was really hoping I'd be able to reproduce it, especially in step 5. I don't know what to make of this. It seems like a slippery issue!

@spapas
Copy link
Contributor

spapas commented Jul 28, 2023

@dmos62 how can I check if these postgresql functions have been installed on my db and install them if needed? Notice that I use a bare metal installation and to upgrade I fetch the latest changes and run manage.py migrate. Do I need to do anything else?

@dmos62
Copy link
Contributor

dmos62 commented Jul 28, 2023

@spapas I asked @mathemancer to respond to you. Some related things have been in flux and I'm not confident that my understanding is up-to-date.

@seancolsen for what it's worth, I've not been able to reproduce on a post-#3097 branch.

@mathemancer
Copy link
Contributor

mathemancer commented Jul 28, 2023

@spapas If you want to run the script via python to upgrade the DB functions, run:

python install.py --skip-confirm >> /tmp/install.py.log

from the mathesar directory.

However, note that upgrading from develop to a newer develop is not yet officially supported. If something goes wrong, you can log into the DB through an alternate client, run

DROP SCHEMA msar CASCADE;

DANGER: if you've built any tables that depend on functions from that schema (unlikely), those will be dropped. After that, run the python command above, and you should have the functions and types properly installed.

Edit: To simply see whether the functions are installed, try to import a CSV. The most recent DB-layer change changed a function signature in a way that will break if you don't have the newest DB functions installed.

@spapas
Copy link
Contributor

spapas commented Jul 28, 2023

Hey @mathemancer thanks for the tip; after I run install.py the alter column type functionality works!

Wouldn't it be possible to create a migration for installing these functions to the database to avoid running the code?

@spapas
Copy link
Contributor

spapas commented Jul 28, 2023

Also I'm happy to confirm that after running install.py the type suggestion works fine for birds.csv on my dev env.

@rajatvijay rajatvijay added status: draft and removed ready Ready for implementation labels Jul 31, 2023
@rajatvijay
Copy link
Contributor

Marking it as status:draft since as per the thread here, its not reproducible.

@dmos62
Copy link
Contributor

dmos62 commented Aug 10, 2023

Should this be closed? We could open a new issue for occassional problems when upgrading our Postgres libraries.

@spapas
Copy link
Contributor

spapas commented Aug 10, 2023

I agree to close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: urgent Blocks other ongoing work regression restricted: maintainers Only maintainers can resolve this issue type: bug Something isn't working work: backend Related to Python, Django, and simple SQL
Projects
No open projects
Development

No branches or pull requests

5 participants