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

Extend column API to support duplicating columns #452

Closed
kgodey opened this issue Jul 20, 2021 · 10 comments · Fixed by #533
Closed

Extend column API to support duplicating columns #452

kgodey opened this issue Jul 20, 2021 · 10 comments · Fixed by #533
Assignees
Labels
ready Ready for implementation type: enhancement New feature or request work: backend Related to Python, Django, and simple SQL

Comments

@kgodey
Copy link
Contributor

kgodey commented Jul 20, 2021

Problem

The design spec for Working with Columns calls for the ability to duplicate a column.

Proposed solution

The Column creation endpoint should be extended to accept a reference to a another column in the same table. If this parameter is passed in, it should create a new column that's a copy of the column that has been passed in. All data in the column should also be copied.

Additional context

@kgodey kgodey added the type: enhancement New feature or request label Jul 20, 2021
@kgodey kgodey added this to the 06. Working with Tables milestone Jul 20, 2021
@kgodey kgodey added ready Ready for implementation work: backend Related to Python, Django, and simple SQL work: database labels Jul 20, 2021
@mathemancer
Copy link
Contributor

We'll need to have the ability to create any column at a specified index to accomplish this. Currently, added columns are placed to the right of the currently-existing columns.

At the DB layer, the norm for viewing columns in a particular order is to use a view. I think we should avoid calling some views tables in the UI (and calling others views). This is quite non-trivial to implement, since column order isn't usually a "controlled" part of a table definition.

@kgodey
Copy link
Contributor Author

kgodey commented Jul 22, 2021

@mathemancer Thanks! I updated the issue to remove the reference to creating it at a specific index. We can figure out creating a column at a specific position later if that's something people want to do.

@eito-fis
Copy link
Contributor

Should this only copy data, or would we also like to preserve constraints? Also, what should the naming scheme look like? I'm imagining just col_name(1), col_name(2) ... col_name(n)

@kgodey
Copy link
Contributor Author

kgodey commented Jul 28, 2021

Good questions!
(1) We should preserve constraints
(2) That naming scheme sounds fine

@mathemancer
Copy link
Contributor

mathemancer commented Jul 30, 2021

W.r.t. preserving constraints: Should this include UNIQUE constraints involving the duplicated column? I think we could run into some weird issues there. For example, if there's a uniqueness constraint on the columns A, B, what does it mean when we copy B so we now have a table with A, B, B'? Should this tuple be required to be unique, or should each of A, B and A, B' be unique (separately):

  A  |  B  |  B' |
-----+-----+-----+
  1  |  0  |  0  | <-- This {B': 0} was copied with the column
  1  |  1  |  1  | <-- This {B': 1} was copied with the column
  1  |  1  |  0  | <-- Should this be allowed by the new constraint?

Should the last row above be prevented from insert by the new constraint?

{A: 1, B: 1} and {A: 1, B': 0} have both already appeared. On the other hand, the tuple (1, 1, 0) hasn't appeared anywhere. What does "preserved" actually mean in this case?

@kgodey
Copy link
Contributor Author

kgodey commented Jul 30, 2021

I think copying constraints means that A, B, and A, B' should be unique (separately). B' is a copy of B, B' should not have any other relationship to B.

Should the last row above be prevented from insert by the new constraint?

Yes, it should, because it violates A, B' being unique.

Separately, now that I think about it more, I think it would be good for the function to take an argument about copying constraints. That way, we can have an option in the UI that allows the user to copy constraints or not. I think copying data should also be toggled by an argument.

@eito-fis
Copy link
Contributor

What would not copying data look like? Set nullable to true and just copy constraints?

@kgodey
Copy link
Contributor Author

kgodey commented Jul 31, 2021

@eito-fis Yes, copy the data type and constraints and set nullable to true if needed. Once we have column-specific formatting/display options, those should copy over too.

@eito-fis
Copy link
Contributor

eito-fis commented Aug 7, 2021

I've started writing code for this, which has brought up some additional questions:

  • We can't duplicate primary key constraints, but we could get similar behavior with a unique and non-nullable constraint. Is this desirable? It feels like a primarily UX question.
  • Should I go ahead and create functions to create the other, non unique constraints? Or should I only copy unique constraints right now?

@kgodey
Copy link
Contributor Author

kgodey commented Aug 7, 2021

(1) I think we shouldn't try to get similar behavior by using other constraints since that's not duplicating the column. It's probably better to have a warning in the UI saying that we won't duplicate the PK constraint.

(2) No, just copy unique and not-null constraints for now.

@eito-fis eito-fis mentioned this issue Aug 10, 2021
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready Ready for implementation type: enhancement New feature or request work: backend Related to Python, Django, and simple SQL
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

3 participants