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

ENH: Scalar parameters #1075

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@cpcloud
Member

cpcloud commented Jul 21, 2017

Closes #53

@cpcloud cpcloud added the enhancement label Jul 21, 2017

@cpcloud cpcloud added this to the 0.11.3 milestone Jul 21, 2017

@cpcloud cpcloud self-assigned this Jul 21, 2017

@cpcloud cpcloud force-pushed the cpcloud:bound-params branch 3 times, most recently from 87676c4 to 77eb251 Jul 27, 2017

@cpcloud cpcloud force-pushed the cpcloud:bound-params branch 3 times, most recently from 923bd61 to 34c332a Aug 9, 2017

@cpcloud cpcloud changed the title from WIP: Scalar parameters to Scalar parameters Aug 15, 2017

@cpcloud cpcloud force-pushed the cpcloud:bound-params branch 2 times, most recently from 9cdebaf to f60a101 Aug 15, 2017

@jreback

This comment has been minimized.

Contributor

jreback commented Aug 18, 2017

this ready to merge?

@cpcloud

This comment has been minimized.

Member

cpcloud commented Aug 18, 2017

@wesm can you review when you get a chance?

@wesm

This comment has been minimized.

Member

wesm commented Aug 18, 2017

Looking

@wesm

Looks good; main comments are around reducing the number of points of contact with the params object

Returns
-------
Expr

This comment has been minimized.

@wesm

wesm Aug 18, 2017

Member

ScalarExpr?

# ORDER BY and LIMIT
order_frag = self.format_postamble()
order_frag = self.format_postamble(params=params)

This comment has been minimized.

@wesm

wesm Aug 18, 2017

Member

This seems like a lot of parameter "threading". Any reason to have this as an argument versus a self.params when the query is being compiled?

@@ -774,7 +774,7 @@ def __init__(self, database, like=None, aggregate=False):
self.like = like
self.aggregate = aggregate
def compile(self):
def compile(self, params=None):

This comment has been minimized.

@wesm

wesm Aug 18, 2017

Member

It seems like these parameters should all be encapsulated in the DDL object, so passing params into ibis.impala.compiler.build_ast is all that's needed.

@@ -87,6 +89,9 @@ def execute_with_scope(expr, scope, **kwargs):
if op in scope:
return scope[op]
if context is None:
context = ctx.Summarize()

This comment has been minimized.

@wesm

wesm Aug 18, 2017

Member

We might try to make a more clear name for "context". This is more of an aggregation style than anything

def test_scalar_parameter(t, df, raw_value):
value = ibis.param(dt.double)
expr = t.float64_with_zeros == value
result = expr.execute(params={value: raw_value})

This comment has been minimized.

@wesm

wesm Aug 18, 2017

Member

What do you think about named parameters (versus having to use the param object as a key)? Can be follow up work

This comment has been minimized.

@cpcloud

cpcloud Aug 18, 2017

Member

I think we can support both. I think we can implement the named version as a special case of unnamed. Implemented the dict API version first because it's more general since it doesn't care about the name of the value.

This comment has been minimized.

@wesm

wesm Aug 18, 2017

Member

Makes sense.

@@ -879,26 +880,26 @@ def __init__(self, *args, **kwargs):
self.exists = kwargs.pop('exists', False)
super(AlchemySelect, self).__init__(*args, **kwargs)
def compile(self):
def compile(self, params=None):

This comment has been minimized.

@wesm

wesm Aug 18, 2017

Member

self.params instead? It might be simpler to have

bound_ddl = ddl.with_params(new_params) then having the parameter threaded everywhere

@cpcloud cpcloud force-pushed the cpcloud:bound-params branch from 7d9fc63 to 28b1bc6 Aug 18, 2017

@cpcloud

This comment has been minimized.

Member

cpcloud commented Aug 18, 2017

@wesm I've refactored to pass params to build_ast and the parameter threading surface is much smaller.

@cpcloud cpcloud force-pushed the cpcloud:bound-params branch from 572ac93 to a62f4f8 Aug 18, 2017

@wesm wesm changed the title from Scalar parameters to ENH: Scalar parameters Aug 21, 2017

@wesm

wesm approved these changes Aug 21, 2017

LGTM. thanks!

@cpcloud cpcloud force-pushed the cpcloud:bound-params branch from a62f4f8 to e39205c Aug 21, 2017

@cpcloud cpcloud closed this in 8a98408 Aug 22, 2017

@cpcloud cpcloud deleted the cpcloud:bound-params branch Aug 22, 2017

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