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.

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

Use CTE instead of inheritance views when generating ranges for types #7455

Merged
merged 26 commits into from
Jul 11, 2024

Conversation

dnwpark
Copy link
Contributor

@dnwpark dnwpark commented Jun 12, 2024

If a type or pointer has children and the compiler is not using EXPLAIN, use a CTE instead of the current inheritance views.

Also remove the expand_inhview debug flag.

Related #7410

@dnwpark
Copy link
Contributor Author

dnwpark commented Jun 12, 2024

The flag is a bit awkward, the end goal is to have two flags, is_explain and use_type_inheritance_ctes while removing the current expand_inhviews.

Edit: flag is removed!

Base automatically changed from inhview-expand-dp to master June 13, 2024 19:13
@dnwpark dnwpark marked this pull request as ready for review June 24, 2024 19:17
@aljazerzen
Copy link
Contributor

@dnwpark, my PRs are merged, SQL now uses inheritance CTEs

@dnwpark dnwpark force-pushed the type-range-cte branch 3 times, most recently from 06705af to 79ec726 Compare July 4, 2024 14:40
@dnwpark
Copy link
Contributor Author

dnwpark commented Jul 5, 2024

For example, given:

  type Foo {
    n -> int64;
  }
  type Foo2 extending Foo;
  type Bar {
    foo -> Foo {
      n -> str
    }
  }
  type Bar2 extending Bar;

The query select Foo { n } produces the sql:

WITH 
"t_default::Foo~1" AS NOT MATERIALIZED 
    ((
    (
        FROM edgedbpub."default::Foo" AS "Foo~2"
        SELECT/*<pg.SelectStmt at 0x7fe6befd7aa0>*/
            "Foo~2".id AS "Foo_value~1",
            "Foo~2"."n" AS "n_serialized~1"
    ) union ALL (
        FROM edgedbpub."default::Foo2" AS "Foo2~1"
        SELECT/*<pg.SelectStmt at 0x7fe6be4e7ad0>*/
            "Foo2~1".id AS "Foo_value~2",
            "Foo2~1"."n" AS "n_serialized~2"
    )
    ))
FROM (
    FROM "t_default::Foo~1" AS "t_default::Foo~2"
    SELECT/*<pg.SelectStmt at 0x7fe6be4e7260>*/
        "t_default::Foo~2"."Foo_value~1" AS "Foo_value~3",
        "t_default::Foo~2"."n_serialized~1" AS "n_serialized~3"
) AS "q~1"
SELECT/*<pg.SelectStmt at 0x7fe6be0354f0>*/
    ROW((
        SELECT/*<pg.SelectStmt at 0x7fe6be4e6420>*/
            "q~1"."n_serialized~3" AS "n_serialized~4"
    )) AS "Foo_serialized~1"

The query select Foo2 { n } produces the sql:

FROM edgedbpub."default::Foo2" AS "Foo2~2"
SELECT/*<pg.SelectStmt at 0x7fe6bd8bee70>*/
    ROW((
        SELECT/*<pg.SelectStmt at 0x7fe6be4e66c0>*/
            "Foo2~2"."n" AS "n_serialized~1"
    )) AS "Foo2_serialized~1"

The query select Bar { foo : { n, @n } } produces the sql:

WITH 
"t_default::Bar~1" AS NOT MATERIALIZED 
    ((
    (
        FROM edgedbpub."default::Bar" AS "Bar~2"
        SELECT/*<pg.SelectStmt at 0x7fe6bd8be630>*/
            "Bar~2".id AS "Bar_value~1",
            "Bar~2"."foo" AS "foo_identity~1"
    ) union ALL (
        FROM edgedbpub."default::Bar2" AS "Bar2~1"
        SELECT/*<pg.SelectStmt at 0x7fe6be4e61b0>*/
            "Bar2~1".id AS "Bar_value~2",
            "Bar2~1"."foo" AS "foo_identity~2"
    )
    )),
"t_default::Foo~1" AS NOT MATERIALIZED 
    ((
    (
        FROM edgedbpub."default::Foo" AS "Foo~1"
        SELECT/*<pg.SelectStmt at 0x7fe6be201fa0>*/
            "Foo~1".id AS "Foo_identity~1",
            "Foo~1"."n" AS "n_serialized~1"
    ) union ALL (
        FROM edgedbpub."default::Foo2" AS "Foo2~1"
        SELECT/*<pg.SelectStmt at 0x7fe6be201550>*/
            "Foo2~1".id AS "Foo_identity~2",
            "Foo2~1"."n" AS "n_serialized~2"
    )
    )),
"t_default::__|foo@default|Bar~1" AS NOT MATERIALIZED 
    ((
    (
        FROM edgedbpub."foo" AS "foo~1"
        SELECT/*<pg.SelectStmt at 0x7fe6be202600>*/
            "foo~1".source AS source,
            "foo~1".target AS target,
            "foo~1"."@n" AS "n_serialized~5"
    ) union ALL (
        FROM edgedbpub."foo" AS "foo~2"
        SELECT/*<pg.SelectStmt at 0x7fe6be200650>*/
            "foo~2".source AS source,
            "foo~2".target AS target,
            "foo~2"."@n" AS "n_serialized~6"
    )
    ))
FROM (
    FROM "t_default::Bar~1" AS "t_default::Bar~2"
    SELECT/*<pg.SelectStmt at 0x7fe6be4e5d30>*/
        "t_default::Bar~2"."Bar_value~1" AS "Bar_value~3",
        "t_default::Bar~2"."foo_identity~1" AS "foo_identity~3"
) AS "q~1"
SELECT/*<pg.SelectStmt at 0x7fe6bd72c890>*/
    ROW((
        FROM LATERAL (
            FROM LATERAL (
                SELECT/*<pg.SelectStmt at 0x7fe6beb596d0>*/
                    "q~1"."foo_identity~3" AS "foo_identity~4"
            ) AS "q~2"
            JOIN LATERAL (
                FROM "t_default::Foo~1" AS "t_default::Foo~2"
                SELECT/*<pg.SelectStmt at 0x7fe6be201d90>*/
                    "t_default::Foo~2"."Foo_identity~1" AS "Foo_identity~3",
                    "t_default::Foo~2"."n_serialized~1" AS "n_serialized~3"
            ) AS "q~3"
                ON ("q~2"."foo_identity~4" = "q~3"."Foo_identity~3")
            SELECT/*<pg.SelectStmt at 0x7fe6be4e4830>*/
                "q~3"."Foo_identity~3" AS "foo_value~1",
                ((
                    SELECT/*<pg.SelectStmt at 0x7fe6be2023f0>*/
                        "q~3"."n_serialized~3" AS "n_serialized~4"
                ), (
                    FROM (
                        FROM "t_default::__|foo@default|Bar~1" AS "t_default::__|foo@default|Bar~2"
                        SELECT/*<pg.SelectStmt at 0x7fe6be2037a0>*/
                            "t_default::__|foo@default|Bar~2".source AS source,
                            "t_default::__|foo@default|Bar~2".target AS target,
                            "t_default::__|foo@default|Bar~2"."n_serialized~5" AS "n_serialized~7"
                    ) AS "q~4"
                    SELECT/*<pg.SelectStmt at 0x7fe6be201070>*/
                        "q~4"."n_serialized~7" AS "n_serialized~8"
                    WHERE
                        (("q~1"."Bar_value~3" = "q~4".source) AND 
                         ("q~2"."foo_identity~4" = "q~4".target))
                )) AS "foo_serialized~1"
        ) AS "Foo_foo~3"
        SELECT/*<pg.SelectStmt at 0x7fe6be4e75c0>*/
            "Foo_foo~3"."foo_serialized~1" AS "foo_serialized~2"
    )) AS "Bar_serialized~1"

The query select Bar2 { foo : { n, @n } } produces the sql:

WITH 
"t_default::Foo~1" AS NOT MATERIALIZED 
    ((
    (
        FROM edgedbpub."default::Foo" AS "Foo~1"
        SELECT/*<pg.SelectStmt at 0x7fe6bd6a8110>*/
            "Foo~1".id AS "Foo_identity~1",
            "Foo~1"."n" AS "n_serialized~1"
    ) union ALL (
        FROM edgedbpub."default::Foo2" AS "Foo2~1"
        SELECT/*<pg.SelectStmt at 0x7fe6bd6a9fd0>*/
            "Foo2~1".id AS "Foo_identity~2",
            "Foo2~1"."n" AS "n_serialized~2"
    )
    ))
FROM edgedbpub."default::Bar2" AS "Bar2~2"
SELECT/*<pg.SelectStmt at 0x7fe6be4e45f0>*/
    ROW((
        FROM LATERAL (
            FROM LATERAL (
                SELECT/*<pg.SelectStmt at 0x7fe6becb5a60>*/
                    "Bar2~2"."foo" AS "foo_identity~1"
            ) AS "q~1"
            JOIN LATERAL (
                FROM "t_default::Foo~1" AS "t_default::Foo~2"
                SELECT/*<pg.SelectStmt at 0x7fe6bd6a8920>*/
                    "t_default::Foo~2"."Foo_identity~1" AS "Foo_identity~3",
                    "t_default::Foo~2"."n_serialized~1" AS "n_serialized~3"
            ) AS "q~2"
                ON ("q~1"."foo_identity~1" = "q~2"."Foo_identity~3")
            SELECT/*<pg.SelectStmt at 0x7fe6bec1b9b0>*/
                "q~2"."Foo_identity~3" AS "foo_value~1",
                ((
                    SELECT/*<pg.SelectStmt at 0x7fe6bd6a8b00>*/
                        "q~2"."n_serialized~3" AS "n_serialized~4"
                ), (
                    FROM edgedbpub."foo" AS "foo~1"
                    SELECT/*<pg.SelectStmt at 0x7fe6bd6aaa80>*/
                        "foo~1"."@n" AS "n_serialized~5"
                    WHERE
                        (("Bar2~2".id = "foo~1".source) AND 
                         ("q~1"."foo_identity~1" = "foo~1".target))
                )) AS "foo_serialized~1"
        ) AS "Foo_foo~3"
        SELECT/*<pg.SelectStmt at 0x7fe6bec190a0>*/
            "Foo_foo~3"."foo_serialized~1" AS "foo_serialized~2"
    )) AS "Bar2_serialized~1"

@dnwpark dnwpark force-pushed the type-range-cte branch 2 times, most recently from ca822b4 to 8f4b050 Compare July 8, 2024 18:33
Copy link
Member

@msullivan msullivan left a comment

Choose a reason for hiding this comment

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

This looks great!

Comment on lines 85 to 88
#: Should type inheritance be expanded using CTEs.
#: When not explaining CTEs can be used to provide access to a type and its
#: descendents.
use_inheritance_ctes: bool = True
Copy link
Member

Choose a reason for hiding this comment

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

When is this false now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In server/compiler/compiler.py, this flag is set to false for explain, bootstrapping and schema reflection.

Copy link
Member

Choose a reason for hiding this comment

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

Does it need to be set to false for bootstrapping and schema reflection? We need to stop using inheritance views for those too

Comment on lines 261 to 263
# Type and type inheriance CTEs in creation order. This ensures type CTEs
# referring to other CTEs are in the correct order.
ordered_inheritance_ctes: list[pgast.CommonTableExpr]
Copy link
Member

Choose a reason for hiding this comment

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

Python dictionaries are ordered by insertion order already; can we take advantage of that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, the two kinds of type ctes (rewrites and inheritance) can depend on each other and may have an interleaved order. The pointers ctes are entirely separate though, and I've separated them now.


# Don't use CTEs if there is no inheritance. (ie. There is only a
# single material type)
or len(typeref.children) == 0
Copy link
Member

Choose a reason for hiding this comment

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

I think the == 0 case should maybe be folded into the above condition, so we don't add an extra select around it for no reason

and include_descendants
and not for_mutation
include_descendants = (
not ptrref.union_is_exhaustive

# HACK: This is a workaround for #4491
and ptrref.out_source.name_hint.module not in {'sys', 'cfg'}
Copy link
Member

Choose a reason for hiding this comment

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

We should drop these checks

path_id=path_id,
ctx=ctx,
)
descentant_ops = [
Copy link
Member

Choose a reason for hiding this comment

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

spelling

@@ -1740,34 +1739,6 @@ def range_for_material_objtype(
rvar = rvar_for_rel(
sctx.rel, lateral=lateral, typeref=typeref, ctx=sctx)

else:
assert not typeref.is_view, "attempting to generate range from view"
Copy link
Member

Choose a reason for hiding this comment

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

Could we preserve this assert in the new path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I put it in _table_from_typeref but I think it's reasonable to have it more ahead in the process.

and not for_mutation
and typeref.name_hint.module in {'cfg', 'sys', 'schema'}
):
aspect = 'inhview'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@msullivan Thoughts on this? I would like to add a comment here, but I'm not entirely sure how best to explain this branch beyond something vague like, "these modules use views to implement inheritance".

)
and isinstance(t, s_objtypes.ObjectType)
)
or isinstance(t, s_objtypes.ObjectType)
Copy link
Member

Choose a reason for hiding this comment

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

We can collapse out the expr_type conditions--those can only hold if the type is an object anyway.

Copy link
Member

@msullivan msullivan left a comment

Choose a reason for hiding this comment

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

Great work on this!

@dnwpark dnwpark merged commit 9fa9d84 into master Jul 11, 2024
23 checks passed
@dnwpark dnwpark deleted the type-range-cte branch July 11, 2024 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants