On Fri, Jan 24, 2025 at 11:46 PM Junwang Zhao <zhjw...@gmail.com> wrote: > > On Fri, Jan 24, 2025 at 9:31 PM Ashutosh Bapat > <ashutosh.bapat....@gmail.com> wrote: > > > > On Sun, Jan 19, 2025 at 6:45 PM Junwang Zhao <zhjw...@gmail.com> wrote: > > > > > > Hi Ashutosh, > > > > > > On Wed, Jan 1, 2025 at 5:02 PM Ashutosh Bapat > > > <ashutosh.bapat....@gmail.com> wrote: > > > > > > > > On Wed, Jan 1, 2025 at 2:22 PM Ashutosh Bapat > > > > <ashutosh.bapat....@gmail.com> wrote: > > > > > > > > > > On Sat, Dec 21, 2024 at 6:21 PM Junwang Zhao <zhjw...@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.
Cirrus meson FreeBSD build has this setting, the regression test rowsecurity failed the same as my local machine, see [0], so I think this is an issue we have to resolve. Cirrus meson Bookworm build failed 002_pg_upgrade.pl and 027_stream_regress.pl, see [1] [0] https://cirrus-ci.com/task/5702052891852800 [1] https://cirrus-ci.com/task/6265002845274112 > > > > > 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 -- Regards Junwang Zhao
