Re: SQL Property Graph Queries (SQL/PGQ)

On Sat, Dec 21, 2024 at 6:21 PM Junwang Zhao <zhjwpku(at)gmail(dot)com> wrote:
Thanks Junwang for your review.

> Here are some review opinions for 0001, I haven't seen the other
> patches, forgive me if some of the opinions have already been
> addressed.
>
> 1. In propgraph_edge_get_ref_keys, when finding a matching foreign key,
> the fk pointer will always point to last ForeignKeyCacheInfo of
> the edge relation, which is wrong. We should add another pointer
> that remembers the matched ForeignKeyCacheInfo to ref_rel. See
> attached 0001.

Nice catch. I fixed it in a slightly different way reducing overall
code by using foreach_node(). I have merged this as part of 0004 which
has fixes for several other issues. Interestingly there was a SQL that
had revealed the problem in create_property_graph.sql. But we had
accepted a wrong output. Corrected that as well.

>
> 2. Some of the TODOs seem easy to address, attached 0002 does this.

From a cursory glance those changes look useful and mostly correct. It
will be good if you can provide a SQL test for those, covering that
part of the code. Please post the whole patch-set with your changes as
a separate commit/patch.

>
> 3. Since property name and label name are unique in property graph
> scope, some of the wording are not accurate. See attached 0003.

<para>
- For each property graph element, all properties with the same name must
- have the same expression for each label. For example, this would be
+ For each property graph, all properties with the same name must
+ have the same expression. For example, this would be

Each property graph element may have a property with the same name
associated with multiple labels. But each of those properties should
have the same expression. You may see that as the same property being
defined multiple times once in each label it is associated with OR
multiple properties with the same name. Current wording uses the
latter notion, so it looks fine to me. The changes made to error
messages are not needed with this notion.

--
Best Wishes,
Ashutosh Bapat