Re: SQL Property Graph Queries (SQL/PGQ)

On Fri, Jan 24, 2025 at 9:31 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> On Sun, Jan 19, 2025 at 6:45 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
> >
> > Hi Ashutosh,
> >
> > On Wed, Jan 1, 2025 at 5:02 PM Ashutosh Bapat
> > <ashutosh.bapat.oss@gmail.com> wrote:
> > >
> > > On Wed, Jan 1, 2025 at 2:22 PM Ashutosh Bapat
> > > <ashutosh.bapat.oss@gmail.com> wrote:
> > > >
> > > > On Sat, Dec 21, 2024 at 6:21 PM Junwang Zhao <zhjwpku@gmail.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.

I figured out it's because I have `-DWRITE_READ_PARSE_PLAN_TREES` this option,
removing this option clears the warning, but I'm not sure if this is a
hidden issue.

>
> 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.

I have some changes based on the latest patch set. I attached two patches with
the following summary.

0001:
- doc: some of the query in ddl.sgml can not be executed
- after path factor was introduced, some comments doesn't apply

0002:
- add a get_propgraph_element_alias_name function and do some trivial refactor

> --
> Best Wishes,
> Ashutosh Bapat



-- 
Regards
Junwang Zhao