Allowing nested @optional` tags #120

amartyashankha · 2018-08-19T00:18:31Z

Most of the action happens in OptionalTraversalTrie (in utils.py).
The most recent test is not passing. This is the one with a 250 line output. I'll put it in a separate file later.

This will allow them to be tracked in the QueryMetadataTable.

Also fixes a bug where Backtrack blocks were incorrectly generated right before an Unfold block, pointing to invalid, unused, and unusable Locations.

…cation.

obi1kenobi · 2018-08-22T15:31:18Z

graphql_compiler/compiler/ir_lowering_match/utils.py

@@ -219,3 +220,104 @@ def construct_where_filter_predicate(simple_optional_root_info):
 # A CompoundMatchQuery is a representation of several MatchQuery objects containing
 #   - match_queries: a list MatchQuery objects
 CompoundMatchQuery = namedtuple('CompoundMatchQuery', ('match_queries'))
+
+
+class OptionalTraversalTrie(dict):


Let's discuss this in person, and figure out whether and how it might relate to the QueryMetadataTable work that has been happening in parallel.

…phql-compiler into nested_optionals

obi1kenobi · 2018-08-30T20:13:04Z

graphql_compiler/compiler/helpers.py

@@ -347,6 +349,26 @@ def __ne__(self, other):
        """Check another object for non-equality against this one."""
        return not self.__eq__(other)

+    def __lt__(self, other):
+        """Check another object for non-equality against this one."""


copy-pasta docstring

obi1kenobi · 2018-08-30T20:18:10Z

graphql_compiler/compiler/helpers.py

@@ -347,6 +349,26 @@ def __ne__(self, other):
        """Check another object for non-equality against this one."""
        return not self.__eq__(other)

+    def __lt__(self, other):


other could realistically be a FoldScopeLocation, we'll have to handle that case.

Also, FoldScopeLocation objects have to be comparable as well, which introduces some code duplication concerns. How about this:

Implement __eq__ and __lt__ on BaseLocation by specifying the rules for which heterogeneous types compare to each other. For equality, they aren't equal, and for __lt__ they can compare in lexicographic order of the type's name.

Set @total_ordering on BaseLocation.

If the two objects being compared are the same type, dispatch to a special private comparator method that is marked as an @abstractmethod in BaseLocation. In Location, this method specifies the logic for comparing Location objects to each other, and in FoldScopeLocation it specifies that object's comparison logic.

Should this be a separate PR? I don't think we need it for nested optionals.

obi1kenobi · 2018-08-30T20:19:39Z

graphql_compiler/compiler/helpers.py

+        if self.visit_counter != other.visit_counter:
+            return self.visit_counter < other.visit_counter
+
+        if self.field is None:


This doesn't seem right. If other.field is also None, the objects are equal, which means that __lt__ must return False. With this implementation, x < x returns True if x is a Location at a vertex field.

This means you need to add more tests.

Should I just explicitly test that Location s objects are compared correctly?

obi1kenobi · 2018-08-30T20:22:26Z

graphql_compiler/compiler/ir_lowering_match/utils.py

+                # Simple optionals are ignored.
+                # There should be no complex optionals after a simple optional.
+                encountered_simple_optional = True
+


remove blank line at the end of block

obi1kenobi · 2018-08-30T20:23:39Z

graphql_compiler/compiler/ir_lowering_match/utils.py

+
+        # Recursively find all rooted subtrees of each of the children of the current node.
+        location_to_list_of_subtrees = {
+            location: [


Is this not just location: list(self.get_all_rooted_subtrees_as_lists(location))? I don't think you need the nested comprehension.

obi1kenobi · 2018-08-30T21:51:53Z

Yeah probably a good idea to write some test cases for comparisons. You can make the comparison logic a separate PR, I'll review it tomorrow AM and then update this one with that PR's stuff.

…

On Thu, Aug 30, 2018, 5:06 PM Amartya Shankha Biswas < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In graphql_compiler/compiler/helpers.py <#120 (comment)> : > @@ -347,6 +349,26 @@ def __ne__(self, other): """Check another object for non-equality against this one.""" return not self.__eq__(other) + def __lt__(self, other): + """Check another object for non-equality against this one.""" + if len(self.query_path) != len(other.query_path): + return len(self.query_path) < len(other.query_path) + + for self_vertex_field, other_vertex_field in zip(self.query_path, other.query_path): + if self_vertex_field != other_vertex_field: + return self_vertex_field < other_vertex_field + + if self.visit_counter != other.visit_counter: + return self.visit_counter < other.visit_counter + + if self.field is None: Should I just explicitly test that Location s objects are compared correctly? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#120 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACPWSof80xzrCUDjT8sRLa2JhLsAv8aHks5uWFPYgaJpZM4WCxSX> .

obi1kenobi · 2018-08-31T00:06:52Z

graphql_compiler/compiler/helpers.py

+    def __eq__(self, other):
+        """Return True if the other object is smaller than self in the total ordering."""
+        if isinstance(self, Location) and isinstance(other, Location):
+            return self == other


I think you can leave __eq__ as an abstractmethod. Otherwise, I think that this line will cause infinite recursion.

obi1kenobi · 2018-08-31T00:06:59Z

graphql_compiler/compiler/helpers.py

+        raise NotImplementedError()
+
+    def __eq__(self, other):
+        """Return True if the other object is smaller than self in the total ordering."""


obi1kenobi · 2018-08-31T00:07:48Z

graphql_compiler/compiler/helpers.py

@@ -237,6 +237,45 @@ def get_location_name(self):
        """Return a tuple of a unique name of the location, and the current field name (or None)."""
        raise NotImplementedError()

+    @abstractmethod
+    def _compare(self, other):


How about _check_if_object_of_same_type_is_smaller or something similarly more descriptive than _compare? Currently, it doesn't communicate that the other object is required to be of the same type as self.

obi1kenobi · 2018-08-31T00:12:16Z

graphql_compiler/compiler/helpers.py

+        if len(self.fold_path) != len(other.fold_path):
+            return len(self.fold_path) < len(other.fold_path)
+
+        for self_edge_tuple, other_edge_tuple in zip(self.fold_path, other.fold_path):


Does if self.fold_path != other.fold_path: return self.fold_path < other.fold_path not work? If I remember correctly, it's a tuple-of-tuples, which should sort correctly.

obi1kenobi · 2018-08-31T00:13:15Z

Also Github seems to think there are conflicts and that this can't be merged right now.

coveralls · 2018-08-31T19:54:01Z

Coverage increased (+0.3%) to 95.282% when pulling 65f6822 on nested_optionals into 719341e on master.

…phql-compiler into nested_optionals

obi1kenobi · 2018-09-01T16:29:28Z

graphql_compiler/tests/test_helpers.py

@@ -38,6 +38,8 @@ def compare_ir_blocks(test_case, expected_blocks, received_blocks):
    for i in six.moves.xrange(len(expected_blocks)):
        expected = expected_blocks[i]
        received = received_blocks[i]
+        if expected != received:
+            __import__('pdb').set_trace()


You should probably remove this before we merge into master.

Woops, fixed

I don't think it is, actually -- the if expected != received: line is still there and it's causing a syntax error.

My bad. Hopefully actually fixed now.

obi1kenobi and others added 24 commits August 8, 2018 18:51

Add basic metadata table, update compiler frontend to fill it.

863147a

Make FoldScopeLocations into first-class location objects.

b9f3a97

This will allow them to be tracked in the QueryMetadataTable.

Pass around FoldScopeLocation objects when navigating a @fold scope.

9c584f4

Also fixes a bug where Backtrack blocks were incorrectly generated right before an Unfold block, pointing to invalid, unused, and unusable Locations.

Ensure that the type name is recorded in the metadata.

ed18de9

Update folded output context fields to make proper use of FoldScopeLo…

778051e

…cation.

Add test case for prohibited folded output of list types.

ea5d1c3

Record and report folded location types.

4c6ddd8

Rename "is_folded" to "is_within_fold".

189c01f

Removing directive check.

8d56483

Maintaining optional location stack during extraction.

0497f6b

Maintaining optional location stack during extraction.

c4c421d

Adding code to construct tree.

335ca26

Success.

e659498

Success.

0e99a82

Fixing python3 compare.

5b12d4d

working version

6154e43

working version

db9724a

Removed comparison for Location.

7cabcc1

Gremlin expected fix.

92c7081

rename variable

bf4073c

Adding a big ass test case.

7a0ff3c

Cleaning up subtree code.

e2d0feb

Cleaning up

97bdc0f

Merging master.

c68a3d9

obi1kenobi reviewed Aug 22, 2018

View reviewed changes

Amartya Shankha Biswas and others added 5 commits August 23, 2018 13:59

Updated ir generation errors.

0a6c9da

Merge branch 'nested_optionals' of github.com:kensho-technologies/gra…

06f16c8

…phql-compiler into nested_optionals

Using flat representation for Trie.

1d7ca8c

Fixing complex nested optionals output.

e292219

Fixed lint errors.

0c8dc34

Amartya Shankha Biswas added 3 commits August 28, 2018 11:40

Fixing outputs.

c850bb5

Adding comparison to Location.

2b007e2

Fixing pylint error.

5c31bdb

obi1kenobi reviewed Aug 30, 2018

View reviewed changes

Amartya Shankha Biswas added 7 commits August 30, 2018 18:01

Addressed comments

2c6a082

Adding comparisons for FoldScopeLocation.

629c343

Adding comparisons for FoldScopeLocation.

f3015dd

Adding Location comparison tests.

a0f6714

Adding equals comparison.

a542dba

Moving to locations file.

02dfc54

Fixing lint.

1478ba3

obi1kenobi reviewed Aug 31, 2018

View reviewed changes

Addressing comments.

cf181d0

amartyashankha force-pushed the nested_optionals branch from 9136e6f to cf181d0 Compare August 31, 2018 19:41

Merge branch 'master' into nested_optionals

1aee8a3

Amartya Shankha Biswas added 6 commits August 31, 2018 16:07

Addressing comments.

ff5489c

Merge branch 'nested_optionals' of github.com:kensho-technologies/gra…

fee6576

…phql-compiler into nested_optionals

Addressing comments.

98b7957

Addressing comments.

41e80cb

Removingf pylint disable.

55b5dd0

Using negation of >= as <.

e5e08a8

obi1kenobi reviewed Sep 1, 2018

View reviewed changes

Amartya Shankha Biswas added 2 commits September 1, 2018 13:31

Removed pdb.

ecb13e4

Fixing error

65f6822

obi1kenobi approved these changes Sep 4, 2018

View reviewed changes

obi1kenobi merged commit 1609a4f into master Sep 4, 2018

obi1kenobi deleted the nested_optionals branch September 4, 2018 17:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allowing nested @optional` tags #120

Allowing nested @optional` tags #120

amartyashankha commented Aug 19, 2018

obi1kenobi Aug 22, 2018

obi1kenobi Aug 30, 2018

obi1kenobi Aug 30, 2018

amartyashankha Aug 30, 2018

obi1kenobi Aug 30, 2018

amartyashankha Aug 30, 2018

obi1kenobi Aug 30, 2018

obi1kenobi Aug 30, 2018

obi1kenobi commented Aug 30, 2018 via email

obi1kenobi Aug 31, 2018

obi1kenobi Aug 31, 2018

obi1kenobi Aug 31, 2018

obi1kenobi Aug 31, 2018

obi1kenobi commented Aug 31, 2018

coveralls commented Aug 31, 2018 •

edited

obi1kenobi Sep 1, 2018

amartyashankha Sep 1, 2018

obi1kenobi Sep 2, 2018

amartyashankha Sep 3, 2018

Allowing nested @optional` tags #120

Allowing nested @optional` tags #120

Conversation

amartyashankha commented Aug 19, 2018

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 commented Aug 30, 2018 via email

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 commented Aug 31, 2018

coveralls commented Aug 31, 2018 • edited

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

coveralls commented Aug 31, 2018 •

edited