Re: Disallow outer-level and WHERE-clause aggregates in GRAPH_TABLE

Hi Ashutosh,

Thanks for the review.

> > > So what I'm thinking right now is
> > > that this is a fundamental design error, and that we should nuke
> > > GraphPropertyRef altogether in favor of using a Var that references
> > > the appropriate column of the RTE_GRAPH_TABLE relation.
> >
> > I spent time today on this and I think the direction is right. In what
> > I have running
> > locally, a property reference is emitted as a plain Var over the
> > RTE_GRAPH_TABLE relation,
> > so the Var handles leveling and the rest for free. GraphPropertyRef
> > does not go away
> > entirely, though. It moves onto a side list on the RTE that the
> > rewriter uses to resolve
> > each property Var back to its graph property.
> >
>
> I share the concern raised by Tom about bugs due to missing
> GraphPropertyRef handling. However, I don't think we can use Var
> instead of GraphPropertyRef. It represents a reference to a property
> of all the graph elements that are bound to an element pattern in a
> GRAPH_TABLE. The element pattern is not represented by a range table
> and Var node does not have a field to represent an element pattern
> variable. Further the same GraphPropertyRef may be rewritten as
> different expressions depending on the element table it gets
> associated with when generating a subquery for a given path.

You are right. A Var does not seem correct here after looking at it a
bit more. GraphPropertyRef is not a column of an existing RTE. Also,
the same reference can resolve differently in different generated path
subqueries depending on which element table that path uses. So keeping a
separate placeholder seems more natural.

> this with Var will be quite complex, if not impossible. The question
> is whether we can find all the places where we need to treat
> GraphPropertyRef as a Var. Given that the GraphPropertyRefs are
> generated during transformation and vanish after rewrite, the places
> where we need to handle them are limited, as noted by Tom already.
> Many of those are already covered by the patch. Going through all the
> places, rather painfully, where we use levelsup for Var-like nodes,
> looking at var.c, I see that most of that code is applicable to
> planning, optimization and execution. The additional places which I
> think we need to handle are:
> a. locate_var_of_level_walker() to handle GraphPropertyRef so that the
> correct error location can be reported. We have separate
> locate_aggref_of_level_walker(). So first I thought that it would be
> appropriate to add a separate
> locate_graphpropertyref_of_level_walker() function. But we want to
> report an error from a place where the distinction between Var and
> GraphPropertyRef is lost. So a separate function doesn't make sense.
> b. contain_vars_of_level() should not see GraphPropertyRef since the
> latter lives within GRAPH_TABLE only. But the function is used during
> query transformation, so it will be good to add a GraphPropertyRef
> case and Assert()/throw error if it is seen. We should add a comment
> there to expect GraphPropertyRef there once we start supporting nested
> subqueries in GRAPH_TABLE.
> c. IncrementVarSublevelsUp() will need to handle GraphPropertyRef once
> we support nested subqueries in GRAPH_TABLE. For now we may want to
> add a GraphPropertyRef case and Assert()/throw error if it is seen. We
> should add a comment there to expect GraphPropertyRef there once we
> start supporting nested subqueries in GRAPH_TABLE. Please note, I
> don't expect ChangeVarNodes to handle GraphPropertyRef since there is
> no rtindex in GraphPropertyRef.

Right. The places where GraphPropertyRef needs Var-like treatment seem
fairly limited. In v7, I handled the walkers that need query level
information, such as locate_var_of_level_walker() and
check_agg_arguments_walker(). I also added a GraphPropertyRef case to
contain_vars_of_level(), since that can be reached during query
transformation before rewrite removes GraphPropertyRef.

IncrementVarSublevelsUp() is different. There I added a
GraphPropertyRef case that leaves it alone and asserts
gprlevelsup == 0, since we do not yet support subqueries inside
GRAPH_TABLE. Once we do, we will need to adjust the levels there as
well.

> Here are some comments on v6 patches.

> @@ -769,9 +775,17 @@ check_agg_arguments_walker(Node *node,
> {
> if (node == NULL)
> return false;
> - if (IsA(node, Var))
> + if (IsA(node, Var) || IsA(node, GraphPropertyRef))
> {

> I would rather add a separate GraphPropertyRef case instead of
> combining it with Var. When we start supporting nested subqueries in
> GRAPH_TABLE, we will need to add levelsup to GraphPropertyRef and
> handle it appropriately. I am leaning towards doing that now and
> setting it to 0. Then use here instead of hardcoded 0.

Done in v7. I added gprlevelsup to carry the level.

> Also pass it on
> to the Vars in the expression that replaces the GraphPropertyRef by
> calling ChangeVarNodes() with sublevelsup = GraphPropertyRef::levelsup
> instead of 0. Once subquery support is added, we will set
> GraphPropertyRef::levelsup to the appropriate value and the
> ChangeVarNodes() will propagate it to the Vars in the replacement
> expression. If we go about it this way, we should add an Assert in
> replace_property_refs() to make sure that GraphPropertyRef::levelsup
> == 0. But I am also fine with implicitly treating GraphPropertyRef as
> level 0 with a comment that we should pass on the levelsup to the Vars
> in the replacement expression once we support nested subqueries in
> GRAPH_TABLE.

... replace_property_refs() now passes gprlevelsup through to
ChangeVarNodes() instead of hardcoding 0. I also added
gprlevelsup == 0 asserts in both IncrementVarSublevelsUp_walker() and
replace_property_refs_mutator(), since subqueries inside GRAPH_TABLE
are not supported yet.

> @@ -1845,6 +1847,10 @@ transformSubLink(ParseState *pstate, SubLink
*sublink)
> case EXPR_KIND_CYCLE_MARK:
> /* okay */
> break;
> + case EXPR_KIND_GRAPH_TABLE_COLUMNS:
> + case EXPR_KIND_GRAPH_TABLE_WHERE:
> + err = _("subqueries within GRAPH_TABLE reference are not supported");
>
>
> I would use "cannot use subquery in GRAPH_TABLE reference" to be
> consistent with the other error messages in this function.

Done in v7.

> I think we could make this function lean by letting
> IncrementVarSublevelsUp() do the levelsup adjustment before the
> GraphPropertyRef is replaced. It will require traversing the
> expression tree twice, but it will eliminate the possibility of
> replace_property_refs_mutator() missing a node type that has levelsup.

Done in v7. I removed the special Var-levelsup handling from
replace_property_refs_mutator() and let IncrementVarSublevelsUp()
adjust the copied expression in generate_query_for_graph_path() before
GraphPropertyRef is replaced.

> @@ -84,6 +84,8 @@ typedef enum ParseExprKind
> EXPR_KIND_GENERATED_COLUMN, /* generation expression for a column */
> EXPR_KIND_CYCLE_MARK, /* cycle mark value */
> EXPR_KIND_PROPGRAPH_PROPERTY, /* derived property expression */
> + EXPR_KIND_GRAPH_TABLE_COLUMNS, /* GRAPH_TABLE COLUMNS list item */
> + EXPR_KIND_GRAPH_TABLE_WHERE, /* WHERE in a GRAPH_TABLE pattern */
>
>
> Nit. I would just write WHERE in a GRAPH_TABLE since GRAPH_TABLE is
> not a pattern, but a table function.

Done in v7.

Also, with these changes, I did see what it would take to support
subqueries and
I have something that I can show in a different thread. I think the
aub-query support
will be a v20 item, while v7- should be targeted for v19.

--
Sami Imseih
Amazon Web Services (AWS)