Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Master PG12 TEST #1012

Closed
wants to merge 98 commits into from
Closed

Master PG12 TEST #1012

wants to merge 98 commits into from

Conversation

jrgemignani
Copy link
Contributor

DO NOT MERGE!!!
DO NOT MERGE!!!

This PR is a test, for bringing the master up to PG12, to allow others to review the work.

muhammadshoaib and others added 30 commits August 24, 2022 08:35
This is an initial version of AGE for PG12.

Co-authored-by: Alex Kwak <[email protected]>
Co-authored-by: Ibrar Ahmad <[email protected]>
Co-authored-by: Josh Innis <[email protected]>
Co-authored-by: John Gemignani <[email protected]>
This is a patch to remove the word incubate from AGE for PG12
Updated the README and RELEASE files.
Added the ability to pass parameters to the cypher() function via
a function called age_prepare_cypher().

This extra function is necessary because the cypher() function itself
isn't actually executed - it is instead transformed and replaced. This
means that it needs to have its input parameters resolved prior to
that transform. However, parameters aren't resolved until the
execution phase. So, another command to resolve them needs to run
prior to the cypher() function call.

This mainly impacts the drivers, which will need to be updated.

Additionally, modified the golang driver as an example of this new
usage.

Added regression tests.
Modified the python driver to pass parameters to the cypher() function
via age_prepare_cypher()

Modified README for the python driver
Temporary update to show diffs.
Temporary change to view regression errors.
Temporary installcheck test
Temporary installcheck for debugging
On regression test failure, dump the failure to the log.
Fixed compare_agtype_scalar_values returned result. It used a
function, varstr_cmp, that wasn't guaranteed to return 1, 0, or -1.
It was only guaranteed to return >0, 0, or <0. This caused some
builds to fail during the regression tests, as 1, 0, or -1 were
expected.
This change will implement running of the go driver unit tests
upon every pull commit made to the `drivers/golang` code.

It uses the `paths` directive to ensure the github action workflow
only runs when the go driver code is changed.

The docker-compose file is required to instaniate a postgres
instance, needed for unit testing.
* Update to go version 1.19, as 1.16 is EOL

The current version of the driver is 1.16 which went EOL Feb 2021

This upgrades to 1.19 and also configures CI to retrospectively
test back to 1.18 (1.17 is EOL). This will give test coverage
for all actively supported go versions.

* Also update readme go version
* Fixed a bug where python driver throws error when build from source

* Moved required packages to requirements.txt

* Update Readme

Rearranged instructions and added instructions to install from requirements.txt

* Fixed a typo in exceptions.py file

Fixed a typo 'SqlExcution' on line 62 in exceptions.py file
    modified:   Makefile
    modified:   README.md
    modified:   RELEASE
    renamed:    age--1.1.0.sql -> age--1.1.1.sql
    modified:   age.control
    modified:   NOTICE
    modified:   RELEASE
    new file:   age--1.1.0--1.1.1.sql
Queries can now call functions using the form CALL ... YIELD.

CALL ... YIELD can be used in some of the following forms:

    Individual:
    CALL
    CALL YIELD
    CALL YIELD WHERE UPDATE/RETURN

    Subqueries:
    READING_CLAUSE CALL YIELD UPDATE/RETURN
    CALL YIELD READING_CLAUSE UPDATE/RETURN

In the future, CALL YIELD support for record returning functions and
multiple variable output functions can be added.

Known Issue with WHERE clause where a WHERE in a MATCH + CALL subquery
does not filter results is known.

Co-authored-by: Dehowe Feng <[email protected]>
Conflicts:
	README.md

Co-authored-by: John Gemignani <[email protected]>
Typo fix where an if statement check was causing a logic error that
caused the where statement to be ignored.

Regression tests added to address this and thoroughly check WHERE
clauses as well.

Co-authored by: John Gemignani <[email protected]>
Fixed the EXPLAIN command to allow for nested cypher commands within
more complex SQL queries.
Fixed the delete_global_graphs function. It was not keeping track of
the previous graph global context pointer.

This was causing a memory leak for multiple graph contexts if
individual graphs were deleted.
Updated the behavior of invalid labels to return NULL rather than
throw an error.

Added additional regression tests as well.
Added license header to the following 3 files -

   regress/sql/age_global_graph.sql
   regress/sql/age_load.sql
   regress/sql/index.sql

Adjusted regression tests for these 3.
markgomer and others added 17 commits March 24, 2023 12:54
* Fixed bug where the system was crashing
when graph_name was passed as NULL.

* added checks for possible NULL arguments before
calling create_complete_graph.

* Added regress tests for Barbell graph generation

Conflicts:
	src/backend/utils/graph_generation.c

Co-authored-by: John Gemignani <[email protected]>
Fixed issue apache#718 which was due to a lack of input checks for an
edge case and added regression tests.

Addition of the driver parameter patch caused 2 additional edge
cases that needed to be checked -

    SELECT * FROM cypher() AS (result agtype);
    SELECT * FROM cypher(NULL) AS (result agtype);
…pache#721)

- Updated assert to pass EXPR_KIND_SELECT_TARGET.
- Added regression tests.
- Used a small dataset for regression tests.
…pache#734)

- A vertex and  an edge cannot have the same label within the same
  graph, so added a check for that.

- insert_simple_vertex() and insert_simple_edge() functions were not
  checking for the kind of label. Since a vertex cannot be
  inserted into a label relation of edge and vice versa, this was
  causing the server termination. Added a check for that.

- Added regression tests.

Conflicts:
	src/backend/utils/graph_generation.c
	src/include/catalog/ag_label.h

Co-authored-by: John Gemignani <[email protected]>
Added logic to prevent match from following optional match. This
syntax is considered invalid, add logic to throw an error when
this happens.

Also added regression tests to test this logic.
…CH clauses(apache#701) (apache#747)

Fixed an issue where an already resolved variable, when used for
a property constraint in a second MATCH clause, errored out.
See apache#701 and apache#724  for more details. This is part of a series of
PRs done to refactor MATCH clause behavior.

This also fixed a regression test that was erroring out, but was
overlooked.

Adjusted and added additional regression tests.

Co-authored-by: Dehowe Feng <[email protected]>
…che#751)

Fixed an issue where an already resolved variable, when used for
a property constraint, errored out. See apache#724 for more details.

This is the second part of the fix for the match property constraint
bug regarding variable reuse. The prior fix addressed clause-to-clause,
this fix addresses within a clause.

Adjusted and added additional regression tests.

Co-authored-by: Dehowe Feng <[email protected]>
Fixed a segmentation fault edge case regarding invalid labels
being used in a repeated variable.
- Replaced estate->es_output_cid with getCurrentCommandId(true) in
  update_entity_tuple to use the correct CommandId for error checks.

- Added regression tests.
- Added regression tests for unwind.
* Implement CI test for jdbc driver

- Modify jdbc driver to pull correct AGE image from docker hub.
- Also added logger to display the result of tests i.e. passing,
  failing etc.

* Implement CI test for nodejs driver

- Modified connection params to connect with running docker instance.
- Fixed failing tests by modifying the expected outcomes.
- Include asf header as key/value to avoid EJSONPARSE error.
  "//" key will never be used by npm for any purpose,
  and is reserved for comments.

* Update docker-compose to pull age image from corresponding pg version tag
- Removed external links from readme.
- Updated version to 1.0.0
    modified:   Makefile
    modified:   README.md
    modified:   RELEASE
    renamed:    age--1.1.1.sql -> age--1.3.0.sql
    modified:   age.control
Corrected an entry in the RELEASE notes for release.
Corrected the RELEASE notes. One entry was pointing to an incorrect
pull request.
Conflicts:
	.github/workflows/go-driver.yml
	.github/workflows/jdbc-driver.yaml
	.github/workflows/nodejs-driver.yaml
	.github/workflows/python-driver.yaml
	.gitignore
	CONTRIBUTING.md
	Dockerfile
	NOTICE
	README.md
	RELEASE
	age--1.3.0.sql
	drivers/README
	drivers/docker-compose.yml
	drivers/golang/README.md
	drivers/golang/age/builder.go
	drivers/golang/age/mapper.go
	drivers/golang/go.mod
	drivers/golang/go.sum
	drivers/golang/parser/README.md
	drivers/jdbc/README.md
	drivers/jdbc/lib/build.gradle.kts
	drivers/jdbc/lib/src/test/java/org/apache/age/jdbc/BaseDockerizedTest.java
	drivers/nodejs/README.md
	drivers/python/README.md
	drivers/python/age/age.py
	drivers/python/age/exceptions.py
	drivers/python/setup.py
	drivers/python/test_age_py.py
	regress/expected/age_global_graph.out
	regress/expected/catalog.out
	regress/expected/cypher_call.out
	regress/expected/cypher_match.out
	regress/expected/cypher_merge.out
	regress/expected/cypher_set.out
	regress/sql/age_global_graph.sql
	regress/sql/cypher_call.sql
	regress/sql/cypher_match.sql
	regress/sql/cypher_set.sql
	src/backend/catalog/ag_label.c
	src/backend/commands/label_commands.c
	src/backend/executor/cypher_create.c
	src/backend/executor/cypher_merge.c
	src/backend/executor/cypher_set.c
	src/backend/parser/cypher_analyze.c
	src/backend/parser/cypher_clause.c
	src/backend/parser/cypher_gram.y
	src/backend/utils/adt/agtype.c
	src/backend/utils/cache/ag_cache.c
	src/backend/utils/graph_generation.c
	src/backend/utils/load/age_load.c
	src/include/catalog/ag_label.h
	src/include/commands/label_commands.h
@jrgemignani jrgemignani changed the title Master pg12 Master PG12 TEST Jul 3, 2023
@jrgemignani jrgemignani closed this Jul 3, 2023
@jrgemignani
Copy link
Contributor Author

jrgemignani commented Jul 3, 2023

@MuhammadTahaNaveed @dehowef @rafsun42 @muhammadshoaib @eyab
If you can review this when you have time. This way I can correct any oversights or mistakes before going forward.

@jrgemignani jrgemignani requested a review from eyab July 3, 2023 22:38
On branch master_PG12 modified the following files -

    modified:   src/backend/executor/cypher_set.c
    modified:   src/backend/parser/cypher_clause.c
    modified:   src/backend/utils/adt/agtype.c
    modified:   src/backend/utils/adt/graphid.c
    modified:   age--1.3.0.sql
    modified:   regress/expected/catalog.out
    modified:   regress/sql/catalog.sql
@jrgemignani jrgemignani reopened this Jul 5, 2023
@jrgemignani jrgemignani closed this Jul 5, 2023
@jrgemignani
Copy link
Contributor Author

I have updated the PR contents; some files will have changed.

Copy link
Member

@dehowef dehowef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an unfinished review-- The rest of my comments will be posted on #1015.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 109 introduced a typo. Aquire should be acquire.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 120, same correction as mentioned above -- aquire-->acquire

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think John mentioned this too, but I will add my concern. Should there be a consistency in the READ_FIELD function for oid? Right now there are two changed instances of READ_OID_FIELD --> READ_INT_FIELD and one of READ_OID_FIELD to READ_UINT_FIELD.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lack of EOL in this file. Maybe needs one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.