[aYPLiTQG] Remove isSchema check to be more performant #4150
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gem-neo4j
added
4.4
team-cypher-surface
gem-neo4j
force-pushed
the
4.4_run_files_updates
branch
from
693a83a to
7ad6b42
Compare
loveleif
reviewed
Jul 29, 2024
| } else { | ||
| // Periodic operations cannot be schema operations, so no need to check that here (will fail as invalid | ||
| // query) | ||
| if (isPeriodicOperation(stmt)) { |
There was a problem hiding this comment.
This check is not fool proof (it's another query string regex). Could we remove this branch?
There was a problem hiding this comment.
This is 4.4 extended, so I was just leaving the behaviour as it was to not break anything
| Util.inTx(db, pools, threadTx -> { | ||
| try (Result result = threadTx.execute(stmt, params)) { | ||
| return consumeResult(result, queue, addStatistics, tx, fileName); | ||
| } catch (Exception e) { | ||
| collectError(queue, reportError, e, fileName); | ||
| // APOC historically skips schema operations | ||
| if (!e.getMessage().contains("Schema operations on database 'neo4j' are not allowed")) { |
There was a problem hiding this comment.
It would be nice if we could use the error code here instead of parsing the message, but I assume it's not possible?
There was a problem hiding this comment.
The error code was just for "forbidden" things, which felt kinda broad, I guess once the GQL status codes start we will be able to do that more :D
| Util.inTx(db, pools, threadTx -> { | ||
| try (Result result = threadTx.execute(stmt, params)) { | ||
| return consumeResult(result, queue, addStatistics, tx, fileName); | ||
| } catch (Exception e) { | ||
| collectError(queue, reportError, e, fileName); | ||
| // APOC historically skips schema operations | ||
| if (!e.getMessage().contains("Schema operations on database 'neo4j' are not allowed")) { |
There was a problem hiding this comment.
on database 'neo4j', what about databases that has another name?
gem-neo4j
force-pushed
the
4.4_run_files_updates
branch
from
7ad6b42 to
b6d57a9
Compare
|
cc @loveleif :) |
loveleif
approved these changes
Aug 22, 2024
Comment on lines
+275
to
+276
| if (!(e.getMessage().contains("Schema operations on database") | ||
| && e.getMessage().contains("are not allowed"))) { |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
RunFile is not allowed to run schema operations, in 4.4.0.9 the validation check for this was a regex that was very basic, then in 4.4.0.11 this was updated to be more accurate, what happens is that every query gets run through an explain which checks if the query is a Schema operation, and if it is, it will ignore it.
The problem I see with this is that it will now run this potentially time consuming check for every query in the file as well as actually running the query.
This PR removes this check and instead checks the errors thrown, if a schema error is thrown then it will handle this there to keep old behaviour.