Re: SQL Property Graph Queries (SQL/PGQ)

On Sun, Jan 19, 2025 at 6:45 PM Junwang Zhao <zhjwpku(at)gmail(dot)com> wrote:
>
> Hi Ashutosh,
>
> On Wed, Jan 1, 2025 at 5:02 PM Ashutosh Bapat
> <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
> >
> > On Wed, Jan 1, 2025 at 2:22 PM Ashutosh Bapat
> > <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
> > >
> > > 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.
> >
> > My last email is held for moderation. It will arrive once moderators
> > release it. In the meantime trying to send the patches as a zip file
> > in a hope that it won't require moderation.
> >
> > --
> > Best Wishes,
> > Ashutosh Bapat
>
> 0007-RLS-tests-20250101.patch introduces regression test failure:
>
> +WARNING: outfuncs/readfuncs failed to produce an equal rewritten parse tree
>
> P.S. It seems the patch set doesn't apply to master.

Thanks for noticing it.

Here's rebased patch set on the current head with some minor conflicts
in various files fixed. I did not see the said failure on my laptop.

0001-0009 patches are the same as the previous set sans mergeconflict fixes.
0010 - is test for \dGx - to make sure that the extended format output
works with \dG. We may decide not to have this patch in the final
commit. But no harm in being there til that point.

--
Best Wishes,
Ashutosh Bapat