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

BUG: Fix sort_by codegen and repeated compilation issue #1181

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@cpcloud
Member

cpcloud commented Oct 21, 2017

Closes #1179
Closes #1180

@cpcloud cpcloud self-assigned this Oct 21, 2017

@cpcloud cpcloud added this to the 0.12 milestone Oct 21, 2017

@cpcloud cpcloud force-pushed the cpcloud:fix-order-by-count branch 3 times, most recently from a307b22 to 074e6c9 Oct 22, 2017

@cpcloud cpcloud added the bigquery label Oct 23, 2017

@cpcloud

This comment has been minimized.

Member

cpcloud commented Oct 24, 2017

@wesm can you review this when you get a chance?

@cpcloud cpcloud requested a review from wesm Oct 24, 2017

@wesm

This comment has been minimized.

Member

wesm commented Oct 24, 2017

will do

@wesm

wesm approved these changes Oct 24, 2017

expr.compile().compile(compile_kwargs={'literal_binds': True})
)
expected = """\
SELECT count('*') AS count

This comment has been minimized.

@wesm

wesm Oct 24, 2017

Member

This is the count-star syntax for sqlite, I guess (did not know that)?

This comment has been minimized.

@cpcloud

cpcloud Oct 24, 2017

Member

Hm, actually this will have the same behavior as count(*) except in the case of an empty table. I need to fix this.

This comment has been minimized.

@cpcloud

cpcloud Oct 24, 2017

Member

Actually, nevermind. This is the form suggested by the SQLAlchemy docs.

@cpcloud cpcloud force-pushed the cpcloud:fix-order-by-count branch from 074e6c9 to 1157257 Oct 24, 2017

@cpcloud cpcloud force-pushed the cpcloud:fix-order-by-count branch from 1157257 to f1e2ebe Oct 24, 2017

@cpcloud cpcloud closed this in a0581b2 Oct 25, 2017

@cpcloud cpcloud deleted the cpcloud:fix-order-by-count branch Oct 25, 2017

@cpcloud

This comment has been minimized.

Member

cpcloud commented Oct 25, 2017

Thanks for the review @wesm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment