-
Notifications
You must be signed in to change notification settings - Fork 175
fix for ops that return nil #289
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
base: develop
Are you sure you want to change the base?
Conversation
|
Works for me. Can we also test that no tuples are returned if you use |
|
Yup, I've added that test and it failed, and I've come up with my version of a fix. It's a bit gross though so let me know if there's a "more correct" way to do it. Now if I can just figure out how to get gh to add the amended commit... |
|
Oh, it did it automatically |
|
Yeah, I don't really like that - the nil filtering should just happen as a matter of course, since the presence of ? variables should insert a nil filter. Wonder why that's not happening. |
|
Oh cool, I assumed it would have. wonder why it's not firing... |
|
@sritchie could you give me a pointer as to where/how the nil filtering should happen? I've been walking through this code today and am struggling. |
|
@bfabry hey, sorry, was out of town helping out at a running race. This is the piece of code that does the filtering: https://github.com/nathanmarz/cascalog/blob/develop/cascalog-core/src/clj/cascalog/cascading/operations.clj#L732 you can see in this search where that gets called:
If you search Let me know if that helps! |
|
@sritchie those are both part of the cascading platform, but the bug is with the in-memory platform? |
|
@bfabry yeah, you're right - that code snippet doesn't really mesh with my comment. I had forgotten that that code was at the Cascading level, then lost context when pasting the link. I think if we can modify this pull request to act like that Cascading code, then we can apply it to map, mapcat and filter, which should fix the tests. |
In memory platform was blowing up for an op that returned a single nil value, due to (jackknife.seq/collectify nil) => []