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

Project fix #185

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Project fix #185

wants to merge 3 commits into from

Conversation

JustusAdam
Copy link
Collaborator

This fixes what I think is a bug in the implementation of query_through in the project operator. The implementation assumed that columns in emit were monotonically increasing, but I did not read about this as an explicit invariant. As a result the implementation would not permute columns if that was requested in emit. The following pseudo-rust illustrates the inconsistency.

let p = Project::new(emit=[0,2,1]); // <- permutation requested

let v = [1,2,3];
p.on_input([v]) // returns [1,3,2] <- correctly permuted
ancestor_state.insert(v);
p.query_through(..., states={key=1, states={..., ancestor_state }); // returns [1,2,3] <- not permuted

The new version should be as efficient as before but correctly permutes in query_through though I have not benchmarked it.

I added safety assertions for debug mode and added a test case.

The new implementation is less pretty and maybe less simple? So I added some comments explaining what it does.

Lmk if there are any questions or issues with the PR.

Justus Adam and others added 3 commits June 4, 2021 16:34
During `query_through` the ordering of columns in `emit` was not maintained, now
it is.
Added a test case
Added safety assertions
Added some explanation comments to the code
Passing the correct length to `reserve` (turns the length passed to `reserve` is for *additional* elements)
@JustusAdam
Copy link
Collaborator Author

I am a bit confused by the output from the automatic checks, because they originate from compiler warnings turned errors in parts of the code this PR does not touch.

I'd prefer not to fix them, as those changes would have nothing to do with the actual purpose of this PR.

@jonhoo
Copy link
Contributor

jonhoo commented Jun 8, 2021

Thanks for this Justus! I'm not actively maintaining this project any more, but always good to have these fixes in a public place for anyone else who might come along and want to run the code. Thanks for sending it back upstream! Tagging @ms705 too so he's aware of this.

@somehowchris
Copy link

somehowchris commented Oct 27, 2021

@jonhoo @ms705 I couldn't find any notice but just PRs and tickets like these. It seems that this project is not only currently not supported but was fully dropped, am I wrong? I would love to be wrong and help out but if so please note that somewhere and put the repository in read only mode to signal it

@jonhoo
Copy link
Contributor

jonhoo commented Nov 6, 2021

@somehowchris I responded over in #179 (comment) — let's keep this PR for discussion of the PR :)

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.

3 participants