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

Fix selections that have inner inputs. #1275

Merged
merged 9 commits into from Nov 21, 2015

Conversation

@llllllllll
Copy link
Member

commented Oct 14, 2015

Things like a[a.a == b.a] would fail because it wouldn't compute the b.a.

return compute_up(expr, data, predicate, scope=scope, **kwargs)


@dispatch(Selection, sa.sql.ColumnElement, ClauseElement)

This comment has been minimized.

Copy link
@cpcloud

cpcloud Oct 14, 2015

Member

Should ClauseElement be sa.sql.expression.BinaryExpression?

This comment has been minimized.

Copy link
@cpcloud

cpcloud Oct 14, 2015

Member

ClauseElement is very general and would let basically any sqlalchemy expression in.

This comment has been minimized.

Copy link
@llllllllll

llllllllll Oct 14, 2015

Author Member

Is BinaryExpression too restrictive here? Are there any valid predicate functions that don't fall into this? Is there any ClauseElement that we would not treat as a valid predicate?

This comment has been minimized.

Copy link
@cpcloud

cpcloud Nov 20, 2015

Member

Yes, Selectable is a subclass of ClauseElement and we don't support non-column boolean expressions.

This comment has been minimized.

Copy link
@llllllllll

llllllllll Nov 20, 2015

Author Member

I tried making this BinaryExpression but it looks like there are some cases where we are getting a BooleanClauseList. The first common parrent is ColumnElement, does that seem restrictive enough?

This comment has been minimized.

Copy link
@cpcloud

cpcloud Nov 20, 2015

Member

Yeah, ColumnElement is fine

broadcast_collect(i, broadcastable, want_to_broadcast, no_recurse)
for i in expr._inputs
)
return expr._subs({e: c for e, c in zip(expr._inputs, children)})

This comment has been minimized.

Copy link
@cpcloud

cpcloud Oct 14, 2015

Member

You couldn't resist :D

This comment has been minimized.

Copy link
@llllllllll

llllllllll Oct 14, 2015

Author Member

I saw my chance to take advantage of dropping 2.6 and I went for it.

@llllllllll

This comment has been minimized.

Copy link
Member Author

commented Oct 14, 2015

looks like I broke everything. I will clean the other backends up and ping you when its passing.

@llllllllll llllllllll added the wip label Oct 14, 2015

@llllllllll llllllllll changed the title BUG: Fix selections that had inner inputs. WIP: Fix selections that had inner inputs. Oct 14, 2015

@llllllllll llllllllll force-pushed the quantopian:select-inputs branch from aef8bec to 4d2707f Oct 16, 2015

@llllllllll

This comment has been minimized.

Copy link
Member Author

commented Nov 2, 2015

related #1230

I need to get back to this. I haven't had much time to work on blaze recently ;_;

@llllllllll llllllllll force-pushed the quantopian:select-inputs branch 3 times, most recently from e4e8914 to f692fec Nov 3, 2015

@llllllllll

This comment has been minimized.

Copy link
Member Author

commented Nov 4, 2015

Hoping the tests pass. This should be ready to review so I am removing the wip tag.

@llllllllll llllllllll removed the wip label Nov 4, 2015

@llllllllll llllllllll changed the title WIP: Fix selections that had inner inputs. Fix selections that have inner inputs. Nov 4, 2015

@llllllllll

This comment has been minimized.

Copy link
Member Author

commented Nov 12, 2015

@cpcloud any thoughts on this? Currently using this for our deploy so it would be good to get this merged.

@llllllllll llllllllll force-pushed the quantopian:select-inputs branch from 5eec84c to 573bcfb Nov 20, 2015

@llllllllll llllllllll force-pushed the quantopian:select-inputs branch from 573bcfb to afb3b18 Nov 20, 2015

ENH: Restrict selection predicates to be ColumnElements
This is a tighter restriction because we don't want selects for the predicate.
@cpcloud

This comment has been minimized.

Copy link

commented on blaze/compute/sql.py in 34131bc Nov 20, 2015

Wouldn't you want compute(expr, {expr._child: data, expr.predicate: predicate}, **kwargs)?

This comment has been minimized.

Copy link
Member Author

replied Nov 20, 2015

Do we need to optimize / compute_down on this compute_up step here?

This comment has been minimized.

Copy link

replied Nov 20, 2015

I think potentially we might. Our predicate may contain things that can be optimized

This comment has been minimized.

Copy link
Member Author

replied Nov 20, 2015

kk, I will use this in the (Selection, Selectable)` case also

@cpcloud

This comment has been minimized.

Copy link

commented on blaze/compute/sql.py in 34131bc Nov 20, 2015

i'd just inline this, since you're only using it in one placd

This comment has been minimized.

Copy link
Member Author

replied Nov 20, 2015

sounds good. pulling it out was dev cruft

@cpcloud

This comment has been minimized.

Copy link

commented on blaze/compute/numexpr.py in adcfccf Nov 20, 2015

nice catch

@cpcloud

This comment has been minimized.

Copy link

commented on blaze/expr/optimize.py in 2f0b5ae Nov 20, 2015

why are these necessary?

This comment has been minimized.

Copy link
Member Author

replied Nov 20, 2015

There are some backends that depended on the old behavior of the predicate not being an input and I was not sure how to properly implement the fix for the backend. I tried with all of the backends I know about. I sort of see this as a shim until we can properly fix this in the backends that are using this. I think this is used by spark, mongo, and bcolz. For bcolz, the real fix imo is to use dask.array

@llllllllll

This comment has been minimized.

Copy link
Member Author

commented Nov 20, 2015

Not sure if the tests are still failing with a 500 from binstar, any idea what caused (or is still causing) this?

edit: seems fine now

@cpcloud

This comment has been minimized.

Copy link

commented on blaze/compute/spark.py in 592af16 Nov 20, 2015

yeah ulitimately i think this backend has low value

@cpcloud

This comment has been minimized.

Copy link
Member

commented Nov 20, 2015

+1 here

@llllllllll

This comment has been minimized.

Copy link
Member Author

commented Nov 21, 2015

+1

good to merge?

cpcloud added a commit that referenced this pull request Nov 21, 2015

Merge pull request #1275 from quantopian/select-inputs
Fix selections that have inner inputs.

@cpcloud cpcloud merged commit 9fc5e46 into blaze:master Nov 21, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.