Skip to content
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.

Sign up for GitHub

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

Join recursive cte to base #880

Merged
merged 18 commits into from
Jul 16, 2020
Merged

Join recursive cte to base #880

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
fa9ac77
Join recursive cte to base
Jul 14, 2020
a974eaa
Fix comment
Jul 14, 2020
dbc993a
Add recurse inside optional test
Jul 15, 2020
dd24b1a
Remove TODO
Jul 15, 2020
c687575
Lint
Jul 15, 2020
e200eea
Remove todo
Jul 15, 2020
75521ce
Add comment
Jul 15, 2020
29f8ee3
Fix mypy hint
Jul 15, 2020
c34be90
Update graphql_compiler/tests/integration_tests/test_backends_integra…
bojanserafimov Jul 15, 2020
50a1f51
Update graphql_compiler/tests/integration_tests/test_backends_integra…
bojanserafimov Jul 15, 2020
38867e6
Remove autogenerated comment
Jul 15, 2020
27f37d3
Merge branch 'cte_join' of github.com:kensho-technologies/graphql-com…
Jul 15, 2020
5b7170f
Add cautionary comment
Jul 16, 2020
859feae
Offset test comments
Jul 16, 2020
edc88c4
Add another integration test
Jul 16, 2020
b912341
Lint
Jul 16, 2020
66be23e
Re-indent test
Jul 16, 2020
fbe8878
Update graphql_compiler/compiler/emit_sql.py
bojanserafimov Jul 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 22 additions & 12 deletions graphql_compiler/compiler/emit_sql.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -690,7 +690,7 @@ def generate_subquery(self) -> str:
return alias


@dataclass
@dataclass(eq=False) # Setting eq=False allows the class to inherit id-based hash from object
class ColumnRouter:
"""Container for columns selected from a variety of selectables.

Expand Down Expand Up @@ -743,7 +743,7 @@ def __init__(self, sql_schema_info: SQLAlchemySchemaInfo, ir: IrAndMetadata):
self._relocate(ir.query_metadata_table.root_location)

# Mapping aliases to the column used to join into them.
self._came_from: Dict[Alias, Column] = {}
self._came_from: Dict[Union[Alias, ColumnRouter], Column] = {}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mention of aliases in the docstring is outdated. So is the entire _aliases field, since it doesn't only contain aliases anymore. I'm working on fixing this in the next PR, and I created an issue #882


self._recurse_needs_cte: bool = False

Expand Down Expand Up @@ -830,6 +830,10 @@ def _join_to_parent_location(
# approach is incorrect because a mandatory edge traversal miss inside an optional
# scope is supposed to invalidate the whole result. However, with this solution the
# result will still appear.
# 3. You might think it's safe to INNER JOIN on recursive traversals in optional
# scope, since the recursion always contains the 0-th level, which is the vertex
# itself. However, when you join a selectable to itself, any rows with NULL value
# at the join key are dropped.
self._filters.append(
sqlalchemy.or_(
self._came_from[self._current_alias].isnot(None),
Expand Down Expand Up @@ -913,10 +917,13 @@ def _wrap_into_cte(self) -> None:
if self._recurse_needs_cte:
self._current_alias = self.get_query(extra_outputs).cte(recursive=False)
self._from_clause = self._current_alias

self._filters = [] # The filters are already included in the cte
self._aliases = {
alias_key: ColumnRouter(

# After creating this CTE, accessing a field of any of the currently marked
# locations should redirect to the column of the CTE that exposes that field.
# Here we replace all aliases with ColumnRouters pointing to the CTE.
old_to_new_alias = {
alias_value: ColumnRouter(
{
external_name: self._current_alias.c[internal_name]
for external_name, internal_name in column_mappings.get(
Expand All @@ -926,6 +933,14 @@ def _wrap_into_cte(self) -> None:
)
for alias_key, alias_value in self._aliases.items()
}
self._came_from = {
old_to_new_alias[alias]: old_to_new_alias[alias].c[came_from.name]
for alias, came_from in self._came_from.items()
}
self._aliases = {
alias_key: old_to_new_alias[alias] for alias_key, alias in self._aliases.items()
}

if not isinstance(self._current_location, Location):
raise AssertionError(
f"Attempted to wrap to CTE while the _current_location of was type "
Expand Down Expand Up @@ -1020,13 +1035,8 @@ def recurse(self, vertex_field: str, depth: int) -> None:
.where(base.c[CTE_DEPTH_NAME] < literal_depth)
)

# Instead of joining to the current _from_clause, we make this alias the _from_clause.
# If the existing _from_clause had any information in it, then the _current_alias would
# be a cte that contains all that information.
# TODO(bojanserafimov): If the existing _from_clause had no filters or traversals, but
# had an output, the output needs to be moved into the cte or
# we need to join to the _from_clause.
self._from_clause = self._current_alias
# TODO(bojanserafimov): This creates an unused alias if there's no tags or outputs so far
self._join_to_parent_location(previous_alias, primary_key, CTE_KEY_NAME, False)

def start_global_operations(self) -> None:
"""Execute a GlobalOperationsStart block."""
Expand Down
217 changes: 167 additions & 50 deletions graphql_compiler/tests/integration_tests/test_backends_integration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,29 +289,29 @@ def test_recurse(self) -> None:

# (query, expected_results) pairs. All of them running with the same parameters.
# The queries are ran in the order specified here.
queries = [
queries: List[Tuple[str, List[Dict[str, Any]]]] = [
# Query 1: Just the root
(
"""
{
Animal {
name @filter(op_name: "=", value: ["$starting_animal_name"])
@output(out_name: "root_name")
}
}""",
{
Animal {
name @filter(op_name: "=", value: ["$starting_animal_name"])
@output(out_name: "root_name")
}
}""",
[{"root_name": "Animal 1"}],
),
# Query 2: Immediate children
(
"""
{
Animal {
name @filter(op_name: "=", value: ["$starting_animal_name"])
out_Animal_ParentOf {
name @output(out_name: "descendant_name")
{
Animal {
name @filter(op_name: "=", value: ["$starting_animal_name"])
out_Animal_ParentOf {
name @output(out_name: "descendant_name")
}
}
}
}""",
}""",
[
{"descendant_name": "Animal 1"},
{"descendant_name": "Animal 2"},
Expand All @@ -321,16 +321,16 @@ def test_recurse(self) -> None:
# Query 3: Grandchildren
(
"""
{
Animal {
name @filter(op_name: "=", value: ["$starting_animal_name"])
out_Animal_ParentOf {
{
Animal {
name @filter(op_name: "=", value: ["$starting_animal_name"])
out_Animal_ParentOf {
name @output(out_name: "descendant_name")
out_Animal_ParentOf {
name @output(out_name: "descendant_name")
}
}
}
}
}""",
}""",
[
{"descendant_name": "Animal 1"},
{"descendant_name": "Animal 2"},
Expand All @@ -341,18 +341,18 @@ def test_recurse(self) -> None:
# Query 4: Grand-grandchildren
(
"""
{
Animal {
name @filter(op_name: "=", value: ["$starting_animal_name"])
out_Animal_ParentOf {
{
Animal {
name @filter(op_name: "=", value: ["$starting_animal_name"])
out_Animal_ParentOf {
out_Animal_ParentOf {
name @output(out_name: "descendant_name")
out_Animal_ParentOf {
name @output(out_name: "descendant_name")
}
}
}
}
}
}""",
}""",
[
{"descendant_name": "Animal 1"},
{"descendant_name": "Animal 2"},
Expand All @@ -363,14 +363,14 @@ def test_recurse(self) -> None:
# Query 5: Recurse depth 1
(
"""
{
Animal {
name @filter(op_name: "=", value: ["$starting_animal_name"])
out_Animal_ParentOf @recurse(depth: 1){
name @output(out_name: "descendant_name")
{
Animal {
name @filter(op_name: "=", value: ["$starting_animal_name"])
out_Animal_ParentOf @recurse(depth: 1){
name @output(out_name: "descendant_name")
}
}
}
}""",
}""",
[
{"descendant_name": "Animal 1"}, # depth 0 match
{"descendant_name": "Animal 1"}, # depth 1 match
Expand All @@ -381,14 +381,14 @@ def test_recurse(self) -> None:
# Query 6: Recurse depth 2
(
"""
{
Animal {
name @filter(op_name: "=", value: ["$starting_animal_name"])
out_Animal_ParentOf @recurse(depth: 2){
name @output(out_name: "descendant_name")
{
Animal {
name @filter(op_name: "=", value: ["$starting_animal_name"])
out_Animal_ParentOf @recurse(depth: 2){
name @output(out_name: "descendant_name")
}
}
}
}""",
}""",
[
{"descendant_name": "Animal 1"}, # depth 0 match
{"descendant_name": "Animal 1"}, # depth 1 match
Expand All @@ -403,14 +403,14 @@ def test_recurse(self) -> None:
# Query 7: Recurse depth 3
(
"""
{
Animal {
name @filter(op_name: "=", value: ["$starting_animal_name"])
out_Animal_ParentOf @recurse(depth: 3){
name @output(out_name: "descendant_name")
{
Animal {
name @filter(op_name: "=", value: ["$starting_animal_name"])
out_Animal_ParentOf @recurse(depth: 3){
name @output(out_name: "descendant_name")
}
}
}
}""",
}""",
[
{"descendant_name": "Animal 1"}, # depth 0 match
{"descendant_name": "Animal 1"}, # depth 1 match
Expand All @@ -426,6 +426,123 @@ def test_recurse(self) -> None:
{"descendant_name": "Animal 4"}, # depth 3 match
],
),
# Query 8: Skip depth 0
(
"""
{
Animal {
name @filter(op_name: "=", value: ["$starting_animal_name"])
out_Animal_ParentOf {
out_Animal_ParentOf @recurse(depth: 2) {
name @output(out_name: "descendant_name")
}
}
}
}""",
[
{"descendant_name": "Animal 1"}, # depth 0 match
{"descendant_name": "Animal 2"}, # depth 0 match
{"descendant_name": "Animal 3"}, # depth 0 match
{"descendant_name": "Animal 1"}, # depth 1 match
{"descendant_name": "Animal 2"}, # depth 1 match
{"descendant_name": "Animal 3"}, # depth 1 match
{"descendant_name": "Animal 4"}, # depth 1 match
{"descendant_name": "Animal 1"}, # depth 2 match
{"descendant_name": "Animal 2"}, # depth 2 match
{"descendant_name": "Animal 3"}, # depth 2 match
{"descendant_name": "Animal 4"}, # depth 2 match
],
),
# Query 9: Output child name
(
"""
{
Animal {
name @filter(op_name: "=", value: ["$starting_animal_name"])
out_Animal_ParentOf {
name @output(out_name: "child_name")
out_Animal_ParentOf @recurse(depth: 2) {
name @output(out_name: "descendant_name")
}
}
}
}""",
[
{"child_name": "Animal 1", "descendant_name": "Animal 1"}, # depth 0 match
{"child_name": "Animal 2", "descendant_name": "Animal 2"}, # depth 0 match
{"child_name": "Animal 3", "descendant_name": "Animal 3"}, # depth 0 match
{"child_name": "Animal 1", "descendant_name": "Animal 1"}, # depth 1 match
{"child_name": "Animal 1", "descendant_name": "Animal 2"}, # depth 1 match
{"child_name": "Animal 1", "descendant_name": "Animal 3"}, # depth 1 match
{"child_name": "Animal 3", "descendant_name": "Animal 4"}, # depth 1 match
{"child_name": "Animal 1", "descendant_name": "Animal 1"}, # depth 2 match
{"child_name": "Animal 1", "descendant_name": "Animal 2"}, # depth 2 match
{"child_name": "Animal 1", "descendant_name": "Animal 3"}, # depth 2 match
{"child_name": "Animal 1", "descendant_name": "Animal 4"}, # depth 2 match
],
),
# Query 10: Recurse within optional scope. Animal_1 has no grandchildren from its
# child Animal_2, but since we use an @optional edge, Animal_2 should
# still appear in the result as a child of Animal_1. Here we are testing
# that the use of @recurse does not interfere with @optional semantics.
(
"""
{
Animal {
name @filter(op_name: "=", value: ["$starting_animal_name"])
out_Animal_ParentOf {
name @output(out_name: "child_name")
out_Animal_ParentOf @optional {
out_Animal_ParentOf @recurse(depth: 1){
name @output(out_name: "descendant_name")
}
}
}
}
}""",
[
{"child_name": "Animal 1", "descendant_name": "Animal 1"}, # depth 0 match
{"child_name": "Animal 1", "descendant_name": "Animal 2"}, # depth 0 match
{"child_name": "Animal 1", "descendant_name": "Animal 3"}, # depth 0 match
{"child_name": "Animal 2", "descendant_name": None}, # depth 0 match
{"child_name": "Animal 3", "descendant_name": "Animal 4"}, # depth 0 match
{"child_name": "Animal 1", "descendant_name": "Animal 1"}, # depth 1 match
{"child_name": "Animal 1", "descendant_name": "Animal 2"}, # depth 1 match
{"child_name": "Animal 1", "descendant_name": "Animal 3"}, # depth 1 match
{"child_name": "Animal 1", "descendant_name": "Animal 4"}, # depth 1 match
],
),
# Query 11: Same as query 10, but with additional traversal inside the @recurse.
(
"""
{
Animal {
name @filter(op_name: "=", value: ["$starting_animal_name"])
out_Animal_ParentOf {
name @output(out_name: "child")
out_Animal_ParentOf @optional {
out_Animal_ParentOf @recurse(depth: 1){
name @output(out_name: "descendant")
out_Animal_ParentOf {
name @output(out_name: "tiny_child")
}
}
}
}
}
}""",
[
{"child": "Animal 1", "descendant": "Animal 1", "tiny_child": "Animal 1"},
{"child": "Animal 1", "descendant": "Animal 1", "tiny_child": "Animal 2"},
{"child": "Animal 1", "descendant": "Animal 1", "tiny_child": "Animal 3"},
{"child": "Animal 1", "descendant": "Animal 3", "tiny_child": "Animal 4"},
{"child": "Animal 2", "descendant": None, "tiny_child": None},
{"child": "Animal 1", "descendant": "Animal 1", "tiny_child": "Animal 1"},
{"child": "Animal 1", "descendant": "Animal 1", "tiny_child": "Animal 2"},
{"child": "Animal 1", "descendant": "Animal 1", "tiny_child": "Animal 3"},
{"child": "Animal 1", "descendant": "Animal 3", "tiny_child": "Animal 4"},
],
),
]

# TODO(bojanserafimov): Only testing in MSSQL because none of our backends agree on recurse
Expand Down Expand Up @@ -713,7 +830,7 @@ def test_snapshot_graphql_schema_from_orientdb_schema(self):
"UniquelyIdentifiable": {"uuid": GraphQLID}
}
schema, _ = generate_schema(
self.orientdb_client, # type: ignore # from fixture
self.orientdb_client,
class_to_field_type_overrides=class_to_field_type_overrides,
hidden_classes={ORIENTDB_BASE_VERTEX_CLASS_NAME},
)
Expand Down
Loading