Adding support for Filter blocks to SQL backend #166
Conversation
| u'has_substring': Operator(u'contains', Cardinality.UNARY), | ||
| } | ||
|
|
||
| UNSUPPORTED_OPERATOR_NAMES = { |
There was a problem hiding this comment.
Any particular reason to specify this set in addition to SUPPORTED_OPERATORS? Seems kind of error-prone -- we'll always need to remember to add every new filtering operator to one of these constants here, or we'll get a random KeyError on line 205 of emit_sql.py. I'd suggest defining SUPPORTED_OPERATORS and then if the operator in the BinaryComposition isn't there then saying it's unsupported.
There was a problem hiding this comment.
Nope, thanks fixed that.
|
|
||
|
|
||
| SUPPORTED_OPERATORS = { | ||
| u'contains': Operator(u'in_', Cardinality.LIST_VALUED), |
There was a problem hiding this comment.
Are the specific names (Operator.name values) here important? Are they used to look up things in SQLAlchemy? A comment might be helpful to explain why the existing compiler operator names are being remapped to new names.
There was a problem hiding this comment.
Yes they are important for the getattr calls. For example, if we are resolving column_name @filter(op_name: "=", value: ["$variable"])
The corresponding python ends up being getattr(Column(column_name), '__eq__')(BindParameter('variable_name') which programattically gets us to Column('column_name') == BindParameter('variable_name'). I'll add this to a comment.
| Returns: | ||
| Expression, SQLAlchemy expression. | ||
| """ | ||
| variable_name = expression.variable_name |
There was a problem hiding this comment.
expression is meant to be of type Variable, no? If so, then line 260 should be an assertion -- variable names are required to start with $.
There was a problem hiding this comment.
Good to know, this is an assertion now.
| left = _expression_to_sql(expression.left, node, context) | ||
| right = _expression_to_sql(expression.right, node, context) | ||
| if sql_operator.cardinality == constants.Cardinality.UNARY: | ||
| # ensure the operator is grabbed from the Column object |
There was a problem hiding this comment.
Seems like there should be an assertion here? The assumption appears to be that after this block, left is an instance of Column.
| left, right = right, left | ||
| if not isinstance(right, BindParameter): | ||
| raise AssertionError( | ||
| u'List valued operator expects column as left side of expression') |
There was a problem hiding this comment.
Always include the value that caused the error in the error message. Imagine debugging this problem -- the error message tells you that something went wrong, but you don't know precisely in what way it went wrong since the exact value of right isn't returned. So the first step afterward is to change the error message to give you more info so that you can actually start looking into it. Let's just skip that first step and make the error message include the data we'd need to start diagnosing the problem.
How about List valued operator {} expects BindParameter as the right side of the expression, but instead got {} or something to that effect?
There was a problem hiding this comment.
This should be fixed now, the check is the same for UNARY and LIST_VALUED things, so they both call the same method with a more detailed error now.
| u'List valued operator expects column as left side of expression') | ||
| if not isinstance(left, Column): | ||
| raise AssertionError( | ||
| u'List valued operator expects bind parameter as right side of expression') |
There was a problem hiding this comment.
I think the error messages and checks are backwards relative to each other. Also, let's refer to types by their actual names: BindParameter, Column, etc.
| right.expanding = True | ||
| clause = getattr(left, sql_operator.name)(right) | ||
| return clause | ||
| else: |
There was a problem hiding this comment.
nit: At one point there was a subtle bug in the compiler where an if-elif tree like this one, where each branch was meant to return, actually had a code path that didn't return. With the raise AssertionError in the else branch, that meant that no error was raised (since the else wasn't taken, one of other the branches was taken but didn't return), and instead the function simply returned None to the caller ... which promptly exploded elsewhere.
So if all branches are meant to return, I'd pull the AssertionError out of the else, and make it say Unreachable state reached (+ as much contextual info as possible) instead of necessarily blaming the cardinality.
There was a problem hiding this comment.
That makes sense. I remember that error, it is very hard to track down.
graphql_compiler/tests/integration_tests/integration_test_helpers.py
Outdated
Show resolved
Hide resolved
| def compile_and_run_match_query(schema, graphql_query, parameters, graph_client): | ||
| """Compiles and runs a MATCH query against the supplied graph client.""" | ||
| compilation_result = graphql_to_match(schema, graphql_query, parameters) | ||
| # OrientDB expects decimals to be passed as their string representation |
There was a problem hiding this comment.
nit: This is slightly misleading. The MATCH code the compiler emits expects Decimals to come in as strings, since they are wrapped in a statement that causes OrientDB to parse them into Decimal types before using them.
There was a problem hiding this comment.
Corrected, thanks for the explanation.
No description provided.