-
Notifications
You must be signed in to change notification settings - Fork 409
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
Conversation
The flag is a bit awkward, the end goal is to have two flags, Edit: flag is removed! |
@dnwpark, my PRs are merged, SQL now uses inheritance CTEs |
06705af
to
79ec726
Compare
For example, given:
The query 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 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 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 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" |
ca822b4
to
8f4b050
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great!
edb/edgeql/compiler/options.py
Outdated
#: 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
edb/pgsql/compiler/context.py
Outdated
# 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] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
edb/pgsql/compiler/relctx.py
Outdated
|
||
# Don't use CTEs if there is no inheritance. (ie. There is only a | ||
# single material type) | ||
or len(typeref.children) == 0 |
There was a problem hiding this comment.
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
edb/pgsql/compiler/relctx.py
Outdated
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'} |
There was a problem hiding this comment.
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
edb/pgsql/compiler/relctx.py
Outdated
path_id=path_id, | ||
ctx=ctx, | ||
) | ||
descentant_ops = [ |
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Is there a reason to turn it off ever?
edb/pgsql/compiler/relctx.py
Outdated
and not for_mutation | ||
and typeref.name_hint.module in {'cfg', 'sys', 'schema'} | ||
): | ||
aspect = 'inhview' |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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.
There was a problem hiding this 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!
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