-
Notifications
You must be signed in to change notification settings - Fork 74
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
Don't re-index relations just to add an empty tuple #178
base: master
Are you sure you want to change the base?
Don't re-index relations just to add an empty tuple #178
Conversation
Now we only have the `Relation` version, so we could combine Rules 7 and 8 and leapjoin instead of doing two separate joins.
The reason why rules 7 and 8 can't be merged yet is another interesting datafrog limitation that could be of interest to you :) It's about leapjoins' well-formedness and there are details in the comment about why we used to need both relation and variable versions of the liveness facts. |
Apart from the above point (which we could keep as a comment for the time being, and fix some day in the future): needless to say this looks good to me once we have a datafrog release with the required PR. |
Hmm, looking at this again, it seems like it's more of an indexing issue? In any case, I'll just remove the FIXME from this PR. It was more of a note-to-self. |
Merging rules 7 and 8 would be something like errors(Loan, Point) :-
loan_invalidated_at(Loan, Point),
origin_contains_loan_on_entry(Origin, Loan, Point),
origin_live_on_entry(Origin, Point). If this were implemented as filter leapers on This criterion manifests as runtime errors rather than compile-time errors: IIRC related to the MIN/MAX sentinel indices in the leapjoins, like an assertion that at least one leaper returned a valid index. I think the errors can be triggered on some of the in-repo input tests, with the two rules merged; at least they used to when we tried this 3y ago. And there are some discussions with Frank about this on Zulip where he mentioned a way to fix it in datafrog (it was some kind of passthrough leaper just for that purpose). I can find these discussions if you want. |
No need to dig through the chat logs. That's pretty clear to me. I've always been a bit confused by the difference between "extend" and "filter" (though more the "anti" variants). "extend" is for when you have terms in the leaper that are not bound by the |
Exactly. There's a bit more info in the leapjoin part of Frank's initial post about datafrog, and maybe we should incorporate more of that into the docs ? (To clarify what I meant: digging through the logs would be to find the proposed fix :) |
Depends on rust-lang/datafrog#36.
Removes some indexes, which should help memory usage by a small amount and makes things easier to read.