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

WIP: Remove ScalarSelect generation #1201

Merged
merged 14 commits into from Aug 11, 2015

Conversation

@cpcloud
Copy link
Member

commented Aug 11, 2015

No description provided.

@cpcloud cpcloud self-assigned this Aug 11, 2015

@cpcloud cpcloud added this to the 0.8.3 milestone Aug 11, 2015

@cpcloud cpcloud added the dependency label Aug 11, 2015

@cpcloud cpcloud changed the title Remove ScalarSelect generation WIP: Remove ScalarSelect generation Aug 11, 2015

@cpcloud cpcloud added enhancement sql and removed dependency labels Aug 11, 2015

@cpcloud

This comment has been minimized.

Copy link
Member Author

commented Aug 11, 2015

@llllllllll can you take a look at this when you get a chance

def compute_up(t, s, **kwargs):
return s.label(t.label)


@dispatch(Label, FromClause)
def compute_up(t, s, **kwargs):
assert len(s.c) == 1, \

This comment has been minimized.

Copy link
@llllllllll

llllllllll Aug 11, 2015

Member

do we ever expect this to be hit? If so, could we make this raise a ValueError. Otherwise I am fine with leaving the assertion.

This comment has been minimized.

Copy link
@cpcloud

cpcloud Aug 11, 2015

Author Member

No, this is me expressing an assumption about the code that follows it. If it fails, then I've hit a case that I didn't handle. The common case is that I've (re)labeled the output of a column that's from a more complex statement than just selecting the column

This comment has been minimized.

Copy link
@llllllllll

llllllllll Aug 11, 2015

Member

I understand, just wanted to make sure that this was not a normal case.

This comment has been minimized.

Copy link
@cpcloud

cpcloud Aug 11, 2015

Author Member

Yes it would be very strange to get to a label expression on something with more than a single column

This comment has been minimized.

Copy link
@cpcloud

cpcloud Aug 11, 2015

Author Member

Let me add a test that tries to fail that assertion in a simple case

This comment has been minimized.

Copy link
@llllllllll

llllllllll Aug 11, 2015

Member

kk, I wouldn't worry too much if it is a sufficiently rare case.

@llllllllll

This comment has been minimized.

Copy link
Member

commented Aug 11, 2015

This looks good, just needs a whatsnew entry

cpcloud added some commits Aug 11, 2015

cpcloud added a commit that referenced this pull request Aug 11, 2015

Merge pull request #1201 from cpcloud/fix-sql
WIP: Remove ScalarSelect generation

@cpcloud cpcloud merged commit 96ca784 into blaze:master Aug 11, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@cpcloud cpcloud deleted the cpcloud:fix-sql branch Aug 11, 2015

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.