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

Replace column index usage with attnums and Column model id #839

Closed
6 tasks done
silentninja opened this issue Nov 23, 2021 · 5 comments
Closed
6 tasks done

Replace column index usage with attnums and Column model id #839

silentninja opened this issue Nov 23, 2021 · 5 comments
Labels
pr-status: revision A PR awaiting follow-up work from its author after review type: enhancement New feature or request type: meta List of related issues. Not meant to be worked on directly. work: backend Related to Python, Django, and simple SQL

Comments

@silentninja
Copy link
Contributor

silentninja commented Nov 23, 2021

Problem

Column index that is being used now is not robust and has few bugs related to its usage. For example db.constraints.operations.select.get_column_constraints will break if columns have been deleted, this comment explains the why attnums are robust identifiers

Proposed solution

PR #658 introduces the Column model which uses attnum, a robust alternate to column index. Hence column model id should be used at the service layer and attnum should be used in the place of column index at the data layer(db module)

We need to make at least the following changes:

Additional context

@silentninja silentninja added type: enhancement New feature or request status: triage needs: unblocking Blocked by other work work: backend Related to Python, Django, and simple SQL and removed status: triage labels Nov 23, 2021
@kgodey kgodey added this to the [08] Initial Data Types milestone Nov 23, 2021
@mathemancer
Copy link
Contributor

I think the db layer identifier for the column should be the attnum, and the mathesar service layer should map between the column id used in the API and the attnum used by the database.

@kgodey kgodey added ready Ready for implementation and removed needs: unblocking Blocked by other work labels Nov 24, 2021
@kgodey
Copy link
Contributor

kgodey commented Nov 24, 2021

Marking as ready since the prerequisite work has been merged.

@silentninja silentninja self-assigned this Dec 24, 2021
@silentninja silentninja removed their assignment Jan 18, 2022
@mathemancer mathemancer added the type: meta List of related issues. Not meant to be worked on directly. label Jan 20, 2022
@silentninja
Copy link
Contributor Author

@kgodey Should these be part of the Initial data type milestone or improvements as the broken down issues seem to be part of improvements?

@kgodey
Copy link
Contributor

kgodey commented Jan 25, 2022

@silentninja nice catch, I moved the broken down issues to Initial Data Types as well. I think it makes sense there since we're expanding the usage of columns through display options in that milestone.

@kgodey
Copy link
Contributor

kgodey commented Jun 2, 2022

This is done.

@kgodey kgodey closed this as completed Jun 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-status: revision A PR awaiting follow-up work from its author after review type: enhancement New feature or request type: meta List of related issues. Not meant to be worked on directly. work: backend Related to Python, Django, and simple SQL
Projects
No open projects
Development

No branches or pull requests

3 participants