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 subqueries with parameters #1303

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
2 participants
@cpcloud
Member

cpcloud commented Jan 25, 2018

closes #1300
closes #1331

This was a somewhat invasive change, but nets a lot fewer places where the params argument has to be used outside of the compile and execute functions and methods on clients. Now, params are part of the context and context is never None so it's easier to reason about where parameters are coming from.

@cpcloud cpcloud self-assigned this Jan 25, 2018

@cpcloud cpcloud requested a review from wesm Jan 25, 2018

@cpcloud cpcloud added the bug label Jan 25, 2018

@cpcloud cpcloud added this to To do in Refactoring via automation Jan 25, 2018

@cpcloud cpcloud added this to the 0.13 milestone Jan 25, 2018

@cpcloud cpcloud force-pushed the cpcloud:fix-scalar-params branch 3 times, most recently from b607536 to 94bb3e2 Jan 25, 2018

@cpcloud cpcloud changed the title from BUG: Fix subqueries with parameters to WIP/BUG: Fix subqueries with parameters Jan 27, 2018

@cpcloud cpcloud force-pushed the cpcloud:fix-scalar-params branch from 10ca2ef to 61c699d Jan 28, 2018

@cpcloud cpcloud added the bigquery label Jan 28, 2018

@cpcloud cpcloud force-pushed the cpcloud:fix-scalar-params branch 2 times, most recently from 3909737 to 6c4aee3 Jan 28, 2018

@cpcloud

This comment has been minimized.

Member

cpcloud commented Jan 30, 2018

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

@kszucs

A short summary on top of ibis/sql/compiler.py would be great, a general overview of the classes and the relationships between them.

It was pretty hard to understand the abstractions here when I wrote the clickhouse backend.

@@ -46,14 +55,30 @@ def __exit__(self, exc_type, exc_value, traceback):
class BigQuery(Query):
def __init__(self, *args, **kwargs):

This comment has been minimized.

@kszucs

kszucs Jan 30, 2018

Member

How about explicit args?

def __init__(self, client, ddl, params=None):
    super(BigQuery, self).__init__(client, ddl)
    self.query_parameters = params or []

This comment has been minimized.

@cpcloud

cpcloud Feb 7, 2018

Member

Good idea, will change. This originally was just so that I could do the default initialization and be robust to signature changes in the parent.

def __init__(self, *args, **kwargs):
query_parameters = kwargs.pop('query_parameters', [])
super(BigQuery, self).__init__(*args, **kwargs)
self.query_parameters = query_parameters

This comment has been minimized.

@kszucs

kszucs Jan 30, 2018

Member

I might just hate typing but ibis.bigquery.BigQuery.query_parameters is a bit verbose to me.

This comment has been minimized.

@cpcloud

cpcloud Feb 7, 2018

Member

Users don't interact with ibis at this level, and this is an instance variable anyway so the most you'd ever type is self.query_parameters

@@ -260,6 +245,13 @@ class BigQueryExprTranslator(impala_compiler.ImpalaExprTranslator):
_registry = _operation_registry
_rewrites = impala_compiler.ImpalaExprTranslator._rewrites.copy()
context_class = BigQueryContext

This comment has been minimized.

@kszucs

kszucs Jan 30, 2018

Member

This is inconsistent with the BigQuerySelect.translator which is not postfixed with _class

This comment has been minimized.

@cpcloud

cpcloud Feb 7, 2018

Member

There's context_class which is a class variable, and context which is an instance of context_class, so they are consistent, unless I'm misunderstanding you here.

This comment has been minimized.

@kszucs

kszucs Feb 7, 2018

Member

I mean BigQuerySelect.translator is holding a class too: BigQueryExprTranslator.

This comment has been minimized.

@cpcloud

cpcloud Feb 7, 2018

Member

I'll make a follow up issue to fix the inconsistency. In the meantime, I'm going to merge this next.

This comment has been minimized.

@kszucs

kszucs Feb 7, 2018

Member

Great! :) You might check #1315 next

@@ -243,7 +234,8 @@ def execute(self, expr, params=None, limit='default', async=False):
def _execute_query(self, ddl, async=False):
klass = self.async_query if async else self.sync_query
return klass(self, ddl).execute()
inst = klass(self, ddl)
return inst.execute()

This comment has been minimized.

@kszucs

kszucs Jan 30, 2018

Member

The previous one was nicer :)

This comment has been minimized.

@cpcloud

cpcloud Feb 7, 2018

Member

Yeah, but this is +epsilon easier to debug.

context = self.context
@property
def translator(self):
return self.context.dialect.translator

This comment has been minimized.

@kszucs

kszucs Jan 30, 2018

Member

It's quite hard to distinguish between Context, Dialect and Translator. Too many indirections for me.
What is the relationship between them?

This comment has been minimized.

@cpcloud

cpcloud Feb 7, 2018

Member

I'll add some documentation for this in ibis/sql/compiler.py.

This comment has been minimized.

@kszucs

kszucs Feb 7, 2018

Member

Thanks :)

@cpcloud cpcloud force-pushed the cpcloud:fix-scalar-params branch 4 times, most recently from ff16f0b to 0d5247e Jan 31, 2018

param = ibis.param('int64')
expr = alltypes[alltypes.string_col.cast('int64') == param]
string_value = 0

This comment has been minimized.

@cpcloud

cpcloud Feb 5, 2018

Member

I need to change these names to reflect the actual values.

@cpcloud cpcloud force-pushed the cpcloud:fix-scalar-params branch 4 times, most recently from f8c7261 to 3330226 Feb 5, 2018

@@ -57,6 +57,14 @@ def batting(self):
def awards_players(self):
return self.connection.database().awards_players
@classmethod
def make_context(cls, params=None):
module_name = cls.module

This comment has been minimized.

@kszucs

kszucs Feb 7, 2018

Member

There is a property for getting the module's name: self.name
We should use either explicit module variables or a method which converts the classname.

This comment has been minimized.

@cpcloud

cpcloud Feb 7, 2018

Member

We should use either explicit module variables or a method which converts the classname.

I'm not sure I follow this. Isn't self.name enough?

@cpcloud cpcloud force-pushed the cpcloud:fix-scalar-params branch from d02cca5 to 13f2f8d Feb 7, 2018

@cpcloud cpcloud changed the title from WIP/BUG: Fix subqueries with parameters to BUG: Fix subqueries with parameters Feb 7, 2018

@cpcloud cpcloud force-pushed the cpcloud:fix-scalar-params branch from f5f4042 to eb285de Feb 7, 2018

@cpcloud

This comment has been minimized.

Member

cpcloud commented Feb 7, 2018

merging on green

@cpcloud cpcloud closed this in 780084d Feb 7, 2018

Refactoring automation moved this from To do to Done Feb 7, 2018

@cpcloud cpcloud deleted the cpcloud:fix-scalar-params branch Feb 7, 2018

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