Orientdb @optional bugfix #95

amartyashankha · 2018-06-01T16:13:13Z

Expected behavior:
Returned results for a query that has a @filter inside an @optional scope must either:

not have the optional edge, or
have the optional edge while satisfying the filtering condition.

Actual behavior:
If the optional edge exists, but the filtering condition inside the @optional is not satisfied, the result was still returned as if the optional edge did not exist.

This PR fixes the above.

coveralls · 2018-06-18T22:02:35Z

Coverage increased (+0.1%) to 94.706% when pulling 9446860 on amartyashankha:orientdb_optional_bugfix into 1caafae on kensho-technologies:master.

obi1kenobi · 2018-06-19T15:51:03Z

graphql_compiler/compiler/ir_lowering_match/utils.py

+            inner_location_name_to_where_filter[inner_location_name] = optional_edge_where_filter
+
+        # Sort expressions by inner_location_name to obtain deterministic order
+        where_filter_expressions = [inner_location_name_to_where_filter[key]


please reformat:

where_filter_expressions = [ inner_location_name_to_where_filter[key] for key in sorted(inner_location_name_to_where_filter.keys()) ]

obi1kenobi

Overall looks like a fine approach, some generally minor tweaks left.

obi1kenobi · 2018-06-19T15:51:52Z

graphql_compiler/compiler/emit_match.py

@@ -156,6 +157,14 @@ def _construct_output_to_match(output_block):
    return u'SELECT %s FROM' % (u', '.join(selections),)


+def _construct_where_to_match(where_block):


This seems like the wrong place to handle this, why can't we lower it away before it gets to this point?

This is being handled elsewhere now. The existing code might still be useful if the a TrueLiteral is explicitly used as a predicate for the Filter?

obi1kenobi · 2018-06-19T15:52:46Z

graphql_compiler/compiler/emit_match.py

@@ -197,6 +206,9 @@ def emit_code_from_single_match_query(match_query):
    # Represent and add the SELECT clauses with the proper output data.
    query_data.appendleft(_construct_output_to_match(match_query.output_block))

+    # Represent and add the WHERE clause with the proper filters.
+    query_data.append(_construct_where_to_match(match_query.where_block))


We should be able to lower it away and set match_query.where_block to None if no filtering is required, so we'll have to check if it's None here.

obi1kenobi · 2018-06-22T13:56:14Z

graphql_compiler/compiler/expressions.py

+                                 .format(self.location))
+
+    def to_match(self):
+        """Return a unicode object with the MATCH representation of this ContextField."""


replace ContextField with SelectEdgeContextField or rephrase -- this object is not a subclass of ContextField

obi1kenobi · 2018-06-22T13:57:02Z

graphql_compiler/compiler/ir_lowering_common.py

+    Returns:
+        dict mapping from simple_optional_root_location -> dict containing keys
+         - 'inner_location_name': Location object correspoding to the unique MarkLocation present
+                                  within a simple optional (one that does not expands vertex fields)


typo: does not expands

obi1kenobi · 2018-06-22T13:57:26Z

graphql_compiler/compiler/ir_lowering_common.py

+                                an @optional traverse) that expand vertex fields
+        location_to_optional_root: dict mapping from location -> optional_location
+                                   where location is within @optional (not necessarily one that
+                                   expands vertex fields) and optional_location is the location


optional_location -> optional_root?

obi1kenobi · 2018-06-22T14:05:59Z

graphql_compiler/compiler/ir_lowering_match/utils.py

+    inner_local_field = LocalField(inner_location_name)
+    inner_location_existence = BinaryComposition(u'!=', inner_local_field, NullLiteral)
+
+    edge_field_location = Location(root_location_path, field=edge_field)


We're passing both of these values separately as function args, then constructing a Location from them. Why not pass a Location then read the two elements from it? Seems more robust and cleaner.

obi1kenobi · 2018-06-22T14:07:10Z

graphql_compiler/compiler/ir_lowering_match/utils.py

+        ]
+
+        predicate = expression_list_to_conjunction(where_filter_expressions)
+        super(SelectWhereFilter, self).__init__(predicate)


Let's make this take a predicate expression instead, and move the logic elsewhere. I'm not a fan of having tons of complex logic in a constructor like this.

obi1kenobi · 2018-06-22T14:08:34Z

graphql_compiler/compiler/match_query.py

+                             u'{} {}'.format(where_block, ir_blocks))
+    ir_except_where = ir_blocks[:-1]
+
+    output_block = ir_except_where[-1]
    if not isinstance(output_block, ConstructResult):
        raise AssertionError(u'Expected last IR block to be ConstructResult, found: '


This isn't the last IR block any more -- the message should be updated if we stick with this representation.

obi1kenobi · 2018-06-22T14:09:28Z

graphql_compiler/tests/test_compiler.py

@@ -443,6 +463,16 @@ def test_filter_on_optional_variable_name_or_alias(self):
                }}
                RETURN $matches
            )
+            WHERE
+                (


Elsewhere, the first ( is on the same line as the keyword that starts it. Let's keep the formatting like that: WHERE ( then de-indent everything below by one.

obi1kenobi · 2018-06-22T14:11:38Z

graphql_compiler/tests/test_emit_output.py

@@ -29,7 +29,8 @@ def test_simple_immediate_output(self):
            MarkLocation(base_location),
            ConstructResult({
                'foo_name': OutputContextField(base_name_location, GraphQLString),
-            })
+            }),
+            SelectWhereFilter({}),


I don't think we want this block added if it isn't doing anything. Also, regardless of what the MATCH query looks like, it's strange from an IR perspective to construct results before filtering them again.

When this block appears, let's move it to right before ConstructResult instead of right after. That way, ConstructResult is always the last IR block, and that simplifies the error-checking code I commented on above.

obi1kenobi

Some minor nitpicks remaining, otherwise looks good.

obi1kenobi · 2018-06-27T17:48:05Z

graphql_compiler/compiler/expressions.py

+
+    def to_gremlin(self):
+        """Not implemented, should not be used."""
+        raise AssertionError(u'SelectEdgeContextField is only used for the WHERE statement in MATCH'


nit: you need a . at the end of MATCH, otherwise the string gets joined into: ... in MATCHThis function should not ...

obi1kenobi · 2018-06-27T17:50:53Z

graphql_compiler/compiler/ir_lowering_match/__init__.py

@@ -17,12 +19,13 @@
                                 lower_context_field_expressions, prune_non_existent_outputs)
 from ..match_query import convert_to_match_query
 from ..workarounds import orientdb_class_with_while, orientdb_eval_scheduling
-
+from .utils import _construct_where_filter_predicate


Underscored functions should be private and not imported (except maybe in tests). Remove the underscore from the function name.

obi1kenobi · 2018-06-27T17:54:38Z

graphql_compiler/compiler/match_query.py

@@ -135,6 +136,34 @@ def _split_match_steps_into_match_traversals(match_steps):
    return output


+def _extract_global_operations(ir_blocks):
+    """Extract all global operation blocks (all blocks following GlobalOperationsStart).


So is ConstructResult considered part of the global operations and returned in global_operation_blocks, or is it returned in remaining_ir_blocks?

From the use of this function below, it seems like ConstructResult shouldn't be part of ir_blocks at all -- in which case the docstring must reflect that, and there should be an assertion that ensures that.

amartyashankha requested a review from obi1kenobi June 1, 2018 16:42

obi1kenobi reviewed Jun 19, 2018

View reviewed changes

obi1kenobi reviewed Jun 22, 2018

View reviewed changes

Amartya Shankha Biswas added 14 commits June 25, 2018 12:30

Extracting simple optional info

d9c8103

Adding WHERE clause

c3adb4b

Continuing WhereBlock, now using Filter

8f44ea0

Added SelectEdgeContextField and SelectWhereFilter

a6f6583

Added docstrings

11742ac

Adding docstrings

220c82f

Updating MATCH in test_compiler

e1a4fe8

Fixing all test cases

7d87550

Adding comments and fixing lint errors

683bc49

Expanding docstring and removing default argument

9917069

Removing TODO

c725a5d

Deterministic order for WHERE filters

3aa7e58

reformatting

64c1bda

Removing SelectWhereFilter and addressing comments.

8126f7e

amartyashankha force-pushed the orientdb_optional_bugfix branch from e493ee1 to 8126f7e Compare June 25, 2018 19:48

Amartya Shankha Biswas added 5 commits June 25, 2018 16:01

Addressing comments.

1f9d5c2

Renaming input for _split_ir_into_match_steps to pruned_ir_blocks

01f0fad

Minor changes.

f8aad9d

Minor changes.

9446860

Addressing comments.

91a9d23

obi1kenobi reviewed Jun 27, 2018

View reviewed changes

Amartya Shankha Biswas added 2 commits June 27, 2018 14:57

Addressing comments.

37e4dd4

Addressing comments.

8d3418e

obi1kenobi approved these changes Jun 27, 2018

View reviewed changes

obi1kenobi merged commit 7b9e4b9 into kensho-technologies:master Jun 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Orientdb @optional bugfix #95

Orientdb @optional bugfix #95

amartyashankha commented Jun 1, 2018 •

edited by obi1kenobi

Loading

coveralls commented Jun 18, 2018 •

edited

Loading

obi1kenobi Jun 19, 2018

obi1kenobi left a comment

obi1kenobi Jun 19, 2018

amartyashankha Jun 25, 2018 •

edited

Loading

obi1kenobi Jun 19, 2018

obi1kenobi Jun 22, 2018

obi1kenobi Jun 22, 2018

obi1kenobi Jun 22, 2018

obi1kenobi Jun 22, 2018

obi1kenobi Jun 22, 2018

obi1kenobi Jun 22, 2018

obi1kenobi Jun 22, 2018

obi1kenobi Jun 22, 2018

obi1kenobi left a comment

obi1kenobi Jun 27, 2018

obi1kenobi Jun 27, 2018

obi1kenobi Jun 27, 2018

		@@ -156,6 +157,14 @@ def _construct_output_to_match(output_block):
		return u'SELECT %s FROM' % (u', '.join(selections),)


		def _construct_where_to_match(where_block):

Orientdb @optional bugfix #95

Orientdb @optional bugfix #95

Conversation

amartyashankha commented Jun 1, 2018 • edited by obi1kenobi Loading

coveralls commented Jun 18, 2018 • edited Loading

Choose a reason for hiding this comment

obi1kenobi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amartyashankha Jun 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obi1kenobi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amartyashankha commented Jun 1, 2018 •

edited by obi1kenobi

Loading

coveralls commented Jun 18, 2018 •

edited

Loading

amartyashankha Jun 25, 2018 •

edited

Loading