Hi Imran, On Sun, Aug 4, 2024 at 12:32 PM Imran Zaheer <imran.z...@gmail.com> wrote: > > Hi > I am attaching a new patch for a minor feature addition. > > - Adding support for 'Labels and properties: EXCEPT list'
Do you intend to support EXCEPT in the label expression as well or just properties? > > Please let me know if something is missing. I think the code changes are in the right place. I didn't review the patch thoroughly. But here are some comments and some advice. Please do not top-post on hackers. Always sent the whole patchset. Otherwise, CI bot gets confused. It doesn't pick up patchset from the previous emails. About the functionality: It's not clear to me whether an EXCEPT should be applicable only at the time of property graph creation or it should be applicable always. I.e. when a property graph is dumped, should it have EXCEPT in it or have a list of columns surviving except list? What if a column in except list is dropped after creating a property graph? Some comments on the code 1. You could use list_member() in insert_property_records() to check whether a given column is in the list of exceptions after you have enveloped in String node. 2. The SELECT with GRAPH_TABLE queries are tested in graph_table.sql. We don't include those in create_property_graph.sql 3. Instead of creating a new property graph in the test, you may modify one of the existing property graphs to have a label with except list and then query it. We are aiming a minimal set of features in the first version. I will let Peter E. decide whether to consider this as minimal set feature or not. The feature looks useful to me. -- Best Wishes, Ashutosh Bapat
