Skip to content

Implement self ref fk ordering#322

Draft
ozer550 wants to merge 7 commits intolearningequality:release-v0.9.xfrom
ozer550:implement-self-ref-fk-ordering
Draft

Implement self ref fk ordering#322
ozer550 wants to merge 7 commits intolearningequality:release-v0.9.xfrom
ozer550:implement-self-ref-fk-ordering

Conversation

@ozer550
Copy link
Copy Markdown
Member

@ozer550 ozer550 commented Apr 29, 2026

Summary

WIP
Short description

TODO

  • Have tests been written for the new code?
  • Has documentation been written/updated?
  • New dependencies (if any) added to requirements file

Reviewer guidance

If you PR has a significant size, give the reviewer some helpful remarks

Issues addressed

List the issues solved or partly solved by the PR

Documentation

If the PR has documentation, link the file here (either .rst in your repo or if built on Read The Docs)

@ozer550 ozer550 requested a review from bjester April 29, 2026 15:09
task.set_store(Store(**kwargs))

@staticmethod
def _compute_self_ref_order(self_ref_fk_value):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So from our discussion on slack and seeing this, I think some of the behaviors have been taken very literally, which is my fault for not making that clearer.

if _self_ref_fk is None and the model is self-referential, the order field should be 0
otherwise, it should query for the parent's order (the Store._self_ref_order for id=_self_ref_fk)

We might need to query the database for this, but I think it's worth thinking through how and when this should occur. This satisfies the criteria of the behavior, but the engineering choices are up to you. The behaviors are not meant to be taken literally that this functionality should explicitly be placed here.

I say this for a couple reasons:

  1. Look at how we manage database calls-- in StoreLookup we do bulk queries for the store records, and how StoreUpdate actually does none. There's room for improvement in managing these queries.
  2. The circumstances may not always work with the pipeline because it buffers records. On this note, I'm thinking in regards to this test case test_handle_store_create__self_ref_parent_not_in_store and whether that is an acceptable outcome.

More on (2): say we have our buffer batch size set to 2. In the first batch, we have a parent and a child. Since the batch size is two, the parent won't be saved yet when the child is processed, so this function should result in None. Then lets say batch #2 has two more children, of the first child (grandchildren). The function should find the store, but the store would have a None order, leading to None for the grandchildren. Thus we have a parent-child relationship which could get out of order because of None.

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 this pull request may close these issues.

2 participants