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

Cypher25 APOC proc func migration #4208

Merged
merged 10 commits into from
Oct 28, 2024
Merged

Conversation

gem-neo4j
Copy link
Contributor

@gem-neo4j gem-neo4j commented Oct 16, 2024

Migrating procedures to APOC for next major (from APOC core).

Note: This also

  • Fixes the build, so it works with latest unpublished Neo
  • Updates gradle
  • Fixes the kotlin
  • Updates some dependencies including testContainers which was the timeout bug

There is another PR to come with docs for this, but that one can't be merged until we get a new branch etc next year

@gem-neo4j gem-neo4j added team-cypher-surface Cypher Surface team should review the PR dev labels Oct 16, 2024
@gem-neo4j gem-neo4j force-pushed the cypher25_apoc_proc_func_migration branch from 4ab3499 to e106f2c Compare October 17, 2024 06:08
@gem-neo4j gem-neo4j force-pushed the cypher25_apoc_proc_func_migration branch 4 times, most recently from 8a39c12 to e31058e Compare October 17, 2024 06:39
@gem-neo4j gem-neo4j force-pushed the cypher25_apoc_proc_func_migration branch 2 times, most recently from 5aad47f to 114bc23 Compare October 17, 2024 08:53
@gem-neo4j gem-neo4j force-pushed the cypher25_apoc_proc_func_migration branch 2 times, most recently from fbcf1f5 to 0a2ae7a Compare October 17, 2024 10:02
@gem-neo4j gem-neo4j force-pushed the cypher25_apoc_proc_func_migration branch from 0a2ae7a to c732e92 Compare October 17, 2024 10:51
@gem-neo4j gem-neo4j force-pushed the cypher25_apoc_proc_func_migration branch 2 times, most recently from b1b51d4 to 6ddece8 Compare October 18, 2024 11:20
@gem-neo4j gem-neo4j force-pushed the cypher25_apoc_proc_func_migration branch from 6ddece8 to d234960 Compare October 18, 2024 11:40
Copy link
Collaborator

@vga91 vga91 left a comment

Choose a reason for hiding this comment

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

Nice work! :)
Just 2 little suggestions and one question and it's OK

@@ -18,7 +18,7 @@ All the following procedures can have the following APOC config, i.e. in `apoc.c
| apoc.ml.openai.url | the OpenAI endpoint base url | `https://api.openai.com/v1` by default,
`https://api.anthropic.com/v1` if `apoc.ml.openai.type=<ANTHROPIC>`,
or empty string if `apoc.ml.openai.type=<AZURE OR HUGGINGFACE>`
| apoc.ml.azure.api.version | in case of `apoc.ml.openai.type=AZURE`, indicates the `api-version` to be passed after the `?api-version=` url
| apoc.ml.azure.api.version | in case of `apoc.ml.openai.type=AZURE`, indicates the `api-version` to be passed after the `?api-version=` url |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe I would add something like <empty string> or "" after the final |?

Comment on lines 50 to 52
private static void assertParquetFails(String query) {
assertFails(query, PARQUET_MISSING_DEPS_ERROR);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this method is not used anymore

Comment on lines +63 to +66
testResult(db, "CYPHER 25 CALL apoc.log.stream('debug.log')", res -> {
final String wholeFile =
Iterators.stream(res.<String>columnAs("line")).collect(Collectors.joining(""));
assertTrue(wholeFile.contains("apoc.import.file.enabled=false"));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just to understand how it works.

Would this test (and the others too) work without CYPHER 25?
And what is the difference between the test without the prefix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They don't work without Cypher 25 at the moment because we haven't got Cypher Versions 100% done in Neo4j yet, but soon we want to be able to change the default version, and when the default version is Cypher 25, then the test wouldn't need the prefix. Basically, these moved procedures only exist on Cypher 25 and not Cypher 5 for extended, and they only exist on Cypher 5 and not 25 for Core :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah ok, got it thanks :)

Copy link
Collaborator

@vga91 vga91 left a comment

Choose a reason for hiding this comment

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

LGTM!

@gem-neo4j gem-neo4j merged commit 9121a81 into dev Oct 28, 2024
5 checks passed
@gem-neo4j gem-neo4j deleted the cypher25_apoc_proc_func_migration branch October 28, 2024 08:03
RobertoSannino pushed a commit that referenced this pull request Nov 5, 2024
…dependency conflict errors (#4215)

* Change Docker download based on branch name

* change BRANCH_NAME var

* rechange BRANCH_NAME var

* added fi statement

* added CODEARTIFACT_DOWNLOAD_URL if-else

* added if CODEARTIFACT_DOWNLOAD_URL then teamcity-repository.gradle

* removed -SNAPSHOT

* Partially reverted 'Cypher25 APOC proc func migration (#4208)'

* Revert "Provisionally ignored failing docker tests in 5.20 (#4085)"

This reverts commit 05bb7ff.

* try resetting ci.yaml

* try with if-else

* decommented BRANCH_NAME

* code clean ci.yaml

* changed extra deps to remove AbstractMethodError with hadoop deps

* excluded deps from xls and hadoop

* removed -community suffix from ci.yaml

* Update CI.yaml

* Update CI.yaml
vga91 added a commit that referenced this pull request Nov 5, 2024
…dependency conflict errors (#4215)

* Change Docker download based on branch name

* change BRANCH_NAME var

* rechange BRANCH_NAME var

* added fi statement

* added CODEARTIFACT_DOWNLOAD_URL if-else

* added if CODEARTIFACT_DOWNLOAD_URL then teamcity-repository.gradle

* removed -SNAPSHOT

* Partially reverted 'Cypher25 APOC proc func migration (#4208)'

* Revert "Provisionally ignored failing docker tests in 5.20 (#4085)"

This reverts commit 05bb7ff.

* try resetting ci.yaml

* try with if-else

* decommented BRANCH_NAME

* code clean ci.yaml

* changed extra deps to remove AbstractMethodError with hadoop deps

* excluded deps from xls and hadoop

* removed -community suffix from ci.yaml

* Update CI.yaml

* Update CI.yaml
gmarcostam pushed a commit to gmarcostam/neo4j-apoc-procedures that referenced this pull request Nov 18, 2024
…dependency conflict errors (neo4j-contrib#4215)

* Change Docker download based on branch name

* change BRANCH_NAME var

* rechange BRANCH_NAME var

* added fi statement

* added CODEARTIFACT_DOWNLOAD_URL if-else

* added if CODEARTIFACT_DOWNLOAD_URL then teamcity-repository.gradle

* removed -SNAPSHOT

* Partially reverted 'Cypher25 APOC proc func migration (neo4j-contrib#4208)'

* Revert "Provisionally ignored failing docker tests in 5.20 (neo4j-contrib#4085)"

This reverts commit 05bb7ff.

* try resetting ci.yaml

* try with if-else

* decommented BRANCH_NAME

* code clean ci.yaml

* changed extra deps to remove AbstractMethodError with hadoop deps

* excluded deps from xls and hadoop

* removed -community suffix from ci.yaml

* Update CI.yaml

* Update CI.yaml
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev team-cypher-surface Cypher Surface team should review the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants