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

SpyglassMixinPart rejects restrictions of self on delete #1163

Closed
CBroz1 opened this issue Oct 11, 2024 · 2 comments · Fixed by #1192
Closed

SpyglassMixinPart rejects restrictions of self on delete #1163

CBroz1 opened this issue Oct 11, 2024 · 2 comments · Fixed by #1192

Comments

@CBroz1
Copy link
Member

CBroz1 commented Oct 11, 2024

Problem

SpyglassMixinPart.delete was designed to allow propagation of deletes through parts with fk refs, but, in doing so, removes the ability to restrict the part itself by a secondary key.

We could decide that this is working as intended - one should not target the part itself for delete. Or, we can say this is counter intuitive and adjust the delete to accept either restriction of part or master

Example tables

@schema
class MyMaster(SpyglassMixin, dj.Whatever):
    description = """
    master_pk: int
    ---
    master_sk: int
    """

    class MyPart(SpyglassMixinPart):
    description = """
    --> master
    part_pk: int
    ---
    part_sk: int
    """

Example usage

(MyMaster.MyPart & 'master_pk = 1').delete() # works
(MyMaster.MyPart & 'master_sk = 1').delete() # does not work
(MyMaster.MyPart & 'part_pk = 1').delete() # does not work
(MyMaster.MyPart & 'part_sk = 1').delete() # does not work

Should all of these work?

Solutions

  1. Long distance restricting the master in this delete step would do that (i.e., self.master >> restriction) while also opening the door to any deletes downstream and maybe being more overhead than we'd like
  2. A try/except restriction of the part or direct cascade of the restriction to the master would be less costly
@edeno
Copy link
Collaborator

edeno commented Oct 11, 2024

Are there downsides to making the last three cases work?

@CBroz1
Copy link
Member Author

CBroz1 commented Oct 11, 2024

Depending on the implementation, computation time or consistency/violating expectations. Currently, relatively few of the part tables in the package inherit this class. Arguably, they all should

CBroz1 added a commit to CBroz1/spyglass that referenced this issue Nov 21, 2024
@CBroz1 CBroz1 linked a pull request Nov 21, 2024 that will close this issue
7 tasks
@CBroz1 CBroz1 mentioned this issue Nov 27, 2024
7 tasks
edeno pushed a commit that referenced this issue Dec 5, 2024
* #1175

* #1185

* #1183

* Fix circular import

* #1163

* #1105

* Fix failing tests, close download subprocesses

* WIP: fix decode changes spikesort tests

* Fix fickle test

* Revert typo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants