Skip to content

Commit 8139f83

Browse files
obi1kenobiobi1kenobi
authored
Fix TypeError due to None sub-expression in BinaryComposition (kensho-technologies#146)
* Add reproducing testcase for TypeError on None sub-expression. * Update input data, add test for correct IR generation. * Fix lowering error due to missing return statement. * Delint.
1 parent 7dcddb0 commit 8139f83

File tree

6 files changed

+229
-7
lines changed

6 files changed

+229
-7
lines changed

‎graphql_compiler/compiler/expressions.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -684,8 +684,8 @@ def validate(self):
684684
_validate_operator_name(self.operator, BinaryComposition.SUPPORTED_OPERATORS)
685685

686686
if not isinstance(self.left, Expression):
687-
raise TypeError(u'Expected Expression left, got: {} {}'.format(
688-
type(self.left).__name__, self.left))
687+
raise TypeError(u'Expected Expression left, got: {} {} {}'.format(
688+
type(self.left).__name__, self.left, self))
689689

690690
if not isinstance(self.right, Expression):
691691
raise TypeError(u'Expected Expression right, got: {} {}'.format(

‎graphql_compiler/compiler/ir_lowering_match/__init__.py

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -117,8 +117,7 @@ def lower_ir(ir_blocks, query_metadata_table, type_equivalence_hints=None):
117117
match_query, complex_optional_roots, location_to_optional_roots)
118118
compound_match_query = prune_non_existent_outputs(compound_match_query)
119119
compound_match_query = collect_filters_to_first_location_occurrence(compound_match_query)
120-
compound_match_query = lower_context_field_expressions(
121-
compound_match_query)
120+
compound_match_query = lower_context_field_expressions(compound_match_query)
122121

123122
compound_match_query = truncate_repeated_single_step_traversals_in_sub_queries(
124123
compound_match_query)

‎graphql_compiler/compiler/ir_lowering_match/optional_traversal.py

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -497,14 +497,15 @@ def _update_context_field_expression(present_locations, expression):
497497
if isinstance(lower_bound, ContextField) or isinstance(upper_bound, ContextField):
498498
raise AssertionError(u'Found BetweenClause with ContextFields as lower/upper bounds. '
499499
u'This should never happen: {}'.format(expression))
500+
return expression
500501
elif isinstance(expression, (OutputContextField, FoldedOutputContextField)):
501502
raise AssertionError(u'Found unexpected expression of type {}. This should never happen: '
502503
u'{}'.format(type(expression).__name__, expression))
503504
elif isinstance(expression, no_op_blocks):
504505
return expression
505-
else:
506-
raise AssertionError(u'Found unexpected expression of type {}. This should never happen: '
507-
u'{}'.format(type(expression).__name__, expression))
506+
507+
raise AssertionError(u'Found unhandled expression of type {}. This should never happen: '
508+
u'{}'.format(type(expression).__name__, expression))
508509

509510

510511
def _lower_non_existent_context_field_filters(match_traversals, visitor_fn):

‎graphql_compiler/tests/test_compiler.py

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2203,6 +2203,106 @@ def test_has_edge_degree_op_filter_with_optional(self):
22032203

22042204
check_test_data(self, test_data, expected_match, expected_gremlin)
22052205

2206+
def test_has_edge_degree_op_filter_with_optional_and_between(self):
2207+
test_data = test_input_data.has_edge_degree_op_filter_with_optional_and_between()
2208+
2209+
expected_match = '''
2210+
SELECT EXPAND($result)
2211+
LET
2212+
$optional__0 = (
2213+
SELECT
2214+
Animal___1.name AS `animal_name`
2215+
FROM (
2216+
MATCH {{
2217+
class: Animal,
2218+
where: ((
2219+
(
2220+
(uuid BETWEEN {uuid_lower_bound} AND {uuid_upper_bound}) AND
2221+
(
2222+
(
2223+
({number_of_edges} = 0) AND
2224+
(in_Animal_ParentOf IS null)
2225+
) OR (
2226+
(in_Animal_ParentOf IS NOT null) AND
2227+
(in_Animal_ParentOf.size() = {number_of_edges})
2228+
)
2229+
)
2230+
) AND (
2231+
(in_Animal_ParentOf IS null) OR
2232+
(in_Animal_ParentOf.size() = 0)
2233+
)
2234+
)),
2235+
as: Animal___1
2236+
}}
2237+
RETURN $matches
2238+
)
2239+
),
2240+
$optional__1 = (
2241+
SELECT
2242+
Animal___1.name AS `animal_name`,
2243+
Animal__in_Animal_ParentOf__out_Entity_Related___1.name AS `related_event`
2244+
FROM (
2245+
MATCH {{
2246+
class: Animal,
2247+
where: ((
2248+
(uuid BETWEEN {uuid_lower_bound} AND {uuid_upper_bound}) AND
2249+
(
2250+
(
2251+
({number_of_edges} = 0) AND
2252+
(in_Animal_ParentOf IS null)
2253+
) OR (
2254+
(in_Animal_ParentOf IS NOT null) AND
2255+
(in_Animal_ParentOf.size() = {number_of_edges})
2256+
)
2257+
)
2258+
)),
2259+
as: Animal___1
2260+
}}.in('Animal_ParentOf') {{
2261+
as: Animal__in_Animal_ParentOf___1
2262+
}}.out('Entity_Related') {{
2263+
where: ((@this INSTANCEOF 'Event')),
2264+
as: Animal__in_Animal_ParentOf__out_Entity_Related___1
2265+
}} RETURN $matches
2266+
)
2267+
),
2268+
$result = UNIONALL($optional__0, $optional__1)
2269+
'''
2270+
expected_gremlin = '''
2271+
g.V('@class', 'Animal')
2272+
.filter{it, m -> (
2273+
(
2274+
(
2275+
($number_of_edges == 0) &&
2276+
(it.in_Animal_ParentOf == null)
2277+
) || (
2278+
(it.in_Animal_ParentOf != null) &&
2279+
(it.in_Animal_ParentOf.count() == $number_of_edges)
2280+
)
2281+
) && (
2282+
(it.uuid >= $uuid_lower_bound) && (it.uuid <= $uuid_upper_bound)
2283+
)
2284+
)}
2285+
.as('Animal___1')
2286+
.ifThenElse{it.in_Animal_ParentOf == null}{null}{it.in('Animal_ParentOf')}
2287+
.as('Animal__in_Animal_ParentOf___1')
2288+
.ifThenElse{it == null}{null}{it.out('Entity_Related')}
2289+
.filter{it, m -> ((it == null) || ['Event'].contains(it['@class']))}
2290+
.as('Animal__in_Animal_ParentOf__out_Entity_Related___1')
2291+
.back('Animal__in_Animal_ParentOf___1')
2292+
.optional('Animal___1')
2293+
.as('Animal___2')
2294+
.transform{it, m -> new com.orientechnologies.orient.core.record.impl.ODocument([
2295+
animal_name: m.Animal___1.name,
2296+
related_event: (
2297+
(m.Animal__in_Animal_ParentOf__out_Entity_Related___1 != null) ?
2298+
m.Animal__in_Animal_ParentOf__out_Entity_Related___1.name :
2299+
null
2300+
)
2301+
])}
2302+
'''
2303+
2304+
check_test_data(self, test_data, expected_match, expected_gremlin)
2305+
22062306
def test_has_edge_degree_op_filter_with_fold(self):
22072307
test_data = test_input_data.has_edge_degree_op_filter_with_fold()
22082308

‎graphql_compiler/tests/test_input_data.py

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1253,6 +1253,40 @@ def has_edge_degree_op_filter_with_optional():
12531253
type_equivalence_hints=None)
12541254

12551255

1256+
def has_edge_degree_op_filter_with_optional_and_between():
1257+
graphql_input = '''{
1258+
Animal {
1259+
name @output(out_name: "animal_name")
1260+
uuid @filter(op_name: "between", value: ["$uuid_lower_bound","$uuid_upper_bound"])
1261+
1262+
in_Animal_ParentOf @optional
1263+
@filter(op_name: "has_edge_degree", value: ["$number_of_edges"]) {
1264+
out_Entity_Related {
1265+
... on Event {
1266+
name @output(out_name: "related_event")
1267+
}
1268+
}
1269+
}
1270+
}
1271+
}
1272+
'''
1273+
expected_output_metadata = {
1274+
'animal_name': OutputMetadata(type=GraphQLString, optional=False),
1275+
'related_event': OutputMetadata(type=GraphQLString, optional=True),
1276+
}
1277+
expected_input_metadata = {
1278+
'uuid_lower_bound': GraphQLID,
1279+
'uuid_upper_bound': GraphQLID,
1280+
'number_of_edges': GraphQLInt,
1281+
}
1282+
1283+
return CommonTestData(
1284+
graphql_input=graphql_input,
1285+
expected_output_metadata=expected_output_metadata,
1286+
expected_input_metadata=expected_input_metadata,
1287+
type_equivalence_hints=None)
1288+
1289+
12561290
def has_edge_degree_op_filter_with_fold():
12571291
graphql_input = '''{
12581292
Species {

‎graphql_compiler/tests/test_ir_generation.py

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2238,6 +2238,94 @@ def test_has_edge_degree_op_filter_with_optional(self):
22382238

22392239
check_test_data(self, test_data, expected_blocks, expected_location_types)
22402240

2241+
def test_has_edge_degree_op_filter_with_optional_and_between(self):
2242+
test_data = test_input_data.has_edge_degree_op_filter_with_optional_and_between()
2243+
2244+
base_location = helpers.Location(('Animal',))
2245+
parent_location = base_location.navigate_to_subpath('in_Animal_ParentOf')
2246+
related_location = parent_location.navigate_to_subpath('out_Entity_Related')
2247+
revisited_base_location = base_location.revisit()
2248+
2249+
expected_blocks = [
2250+
blocks.QueryRoot({'Animal'}),
2251+
blocks.Filter(
2252+
expressions.BinaryComposition(
2253+
u'||',
2254+
expressions.BinaryComposition(
2255+
u'&&',
2256+
expressions.BinaryComposition(
2257+
u'=',
2258+
expressions.Variable('$number_of_edges', GraphQLInt),
2259+
expressions.ZeroLiteral
2260+
),
2261+
expressions.BinaryComposition(
2262+
u'=',
2263+
expressions.LocalField('in_Animal_ParentOf'),
2264+
expressions.NullLiteral
2265+
)
2266+
),
2267+
expressions.BinaryComposition(
2268+
u'&&',
2269+
expressions.BinaryComposition(
2270+
u'!=',
2271+
expressions.LocalField('in_Animal_ParentOf'),
2272+
expressions.NullLiteral
2273+
),
2274+
expressions.BinaryComposition(
2275+
u'=',
2276+
expressions.UnaryTransformation(
2277+
u'size',
2278+
expressions.LocalField('in_Animal_ParentOf')
2279+
),
2280+
expressions.Variable('$number_of_edges', GraphQLInt)
2281+
)
2282+
)
2283+
)
2284+
),
2285+
blocks.Filter(
2286+
expressions.BinaryComposition(
2287+
u'&&',
2288+
expressions.BinaryComposition(
2289+
u'>=',
2290+
expressions.LocalField('uuid'),
2291+
expressions.Variable('$uuid_lower_bound', GraphQLID)
2292+
),
2293+
expressions.BinaryComposition(
2294+
u'<=',
2295+
expressions.LocalField('uuid'),
2296+
expressions.Variable('$uuid_upper_bound', GraphQLID)
2297+
)
2298+
)
2299+
),
2300+
blocks.MarkLocation(base_location),
2301+
blocks.Traverse('in', 'Animal_ParentOf', optional=True),
2302+
blocks.MarkLocation(parent_location),
2303+
blocks.Traverse('out', 'Entity_Related', within_optional_scope=True),
2304+
blocks.CoerceType({'Event'}),
2305+
blocks.MarkLocation(related_location),
2306+
blocks.Backtrack(parent_location),
2307+
blocks.EndOptional(),
2308+
blocks.Backtrack(base_location, optional=True),
2309+
blocks.MarkLocation(revisited_base_location),
2310+
blocks.ConstructResult({
2311+
'animal_name': expressions.OutputContextField(
2312+
base_location.navigate_to_field('name'), GraphQLString),
2313+
'related_event': expressions.TernaryConditional(
2314+
expressions.ContextFieldExistence(related_location),
2315+
expressions.OutputContextField(
2316+
related_location.navigate_to_field('name'), GraphQLString),
2317+
expressions.NullLiteral),
2318+
}),
2319+
]
2320+
expected_location_types = {
2321+
base_location: 'Animal',
2322+
parent_location: 'Animal',
2323+
related_location: 'Event',
2324+
revisited_base_location: 'Animal',
2325+
}
2326+
2327+
check_test_data(self, test_data, expected_blocks, expected_location_types)
2328+
22412329
def test_has_edge_degree_op_filter_with_fold(self):
22422330
test_data = test_input_data.has_edge_degree_op_filter_with_fold()
22432331

0 commit comments

Comments
 (0)