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

Columns re-ordered after type change #1185

Closed
seancolsen opened this issue Mar 16, 2022 · 16 comments · Fixed by #1366
Closed

Columns re-ordered after type change #1185

seancolsen opened this issue Mar 16, 2022 · 16 comments · Fixed by #1366
Assignees
Labels
good first issue Everything in "Help wanted", PLUS being relatively easy and straightforward to implement. help wanted Community contributors can implement this ready Ready for implementation type: enhancement New feature or request work: backend Related to Python, Django, and simple SQL

Comments

@seancolsen
Copy link
Contributor

seancolsen commented Mar 16, 2022

Reproduce

  1. "New Table" > "Import Data" > "Copy and Paste Text"

  2. Paste the following data and proceed to create and view the table (by clicking "Continue" and then "Finish Import").

    id,a,b
    1,100,200
    2,101,201
    
  3. Click on the column header for column a and change the type from "Number" to "Text", clicking "Save".

  4. Expect columns to be ordered id, a, b.

  5. Once I did this and observed the columns to be ordered a, id, b. Another time. I observed the columns to be ordered id, b, a. I'm not sure what affects the ordering of the columns, but the order seems to remain consistent (even if incorrect) after the type-change operation.

Implementation

The backend should make sure that the column list in the API should always be ordered by suitable index.

Additional details

  • The front end displays the columns in the order it receives them from the columns API. In this case the API is returning the columns out-of-order, which is why I've marked this as a backend bug.
  • A while back, we had a discussion about whether the user should be able to re-order columns in which we decided not to support re-ordering columns, at least for now.
@seancolsen seancolsen added ready Ready for implementation type: enhancement New feature or request work: backend Related to Python, Django, and simple SQL good first issue Everything in "Help wanted", PLUS being relatively easy and straightforward to implement. help wanted Community contributors can implement this labels Mar 16, 2022
@seancolsen seancolsen added this to the [12.1] 2022-05 improvements milestone Mar 16, 2022
@seancolsen

This comment was marked as outdated.

@kgodey

This comment was marked as outdated.

@seancolsen

This comment was marked as outdated.

@vorakunal
Copy link

May I work on this issue?

@silentninja
Copy link
Contributor

column_index is just incorrectly computed attnum. attnums is the order by the column are stored on the database, the columns should be ordered by attnum and not by id

@silentninja silentninja added status: started and removed ready Ready for implementation labels Mar 17, 2022
@silentninja
Copy link
Contributor

Thanks! @vorakunal I have assigned the issue to you. Let us know if you need any help.

@mathemancer
Copy link
Contributor

column_index is just incorrectly computed attnum. attnums is the order by the column are stored on the database, the columns should be ordered by attnum and not by id

Sort of, it's an incorrectly computed "dense rank" of the column attnum. I agree with @silentninja that we should just use attnum, though. For more detail see my comment in the relevant discussion.

@kgodey
Copy link
Contributor

kgodey commented Mar 18, 2022

Sounds good, let's use attnum.

@vorakunal
Copy link

vorakunal commented Mar 26, 2022

@seancolsen I am not to able reproduce the error, I followed the instruction but the order is consistent after the type-change operation for both the column.
I'm attaching a screenshot of the table after performing an operation.

Screenshot (140)

@seancolsen
Copy link
Contributor Author

@vorakunal Yeah, the fact that you can't reproduce it is not too surprising, given that I was observing some inconsistent behavior.

@silentninja and @mathemancer since you have a deeper understanding of the root cause of the problem, would you be able to provide more resilient reproduction steps?

@silentninja
Copy link
Contributor

silentninja commented Mar 29, 2022

@vorakunal By default, Postgres uses random order, most of the time it is based on the order it is on the disk(creation order) but it cannot be relied upon as per postgres docs So this issue is a random bug and cannot be deterministically reproduced.

As some of our use cases depend on the order of the columns, we should explicitly specify the sorting order when fetching column to order it based on the attnum property than reply on the random default ordering of the database (which cannot be relied upon)

@seancolsen
Copy link
Contributor Author

@vorakunal just to clarify (from chatting with @silentninja) there is no need to successfully reproduce the bug before applying a fix where you order the columns by the attnum property.

@vorakunal
Copy link

@seancolsen @silentninja Alright, I'll look into it.
Thank you.

@kgodey kgodey added ready Ready for implementation and removed status: started labels May 6, 2022
@kgodey
Copy link
Contributor

kgodey commented May 6, 2022

Unassigned due to inactivity.

@Anish9901
Copy link
Member

Can I work on this?

@kgodey
Copy link
Contributor

kgodey commented May 7, 2022

@Anish9901 Sure, go ahead!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Everything in "Help wanted", PLUS being relatively easy and straightforward to implement. help wanted Community contributors can implement this ready Ready for implementation type: enhancement New feature or request work: backend Related to Python, Django, and simple SQL
Projects
No open projects
6 participants