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

Reusing Statement with different property values #869

Open
zakjan opened this issue Nov 30, 2023 · 5 comments
Open

Reusing Statement with different property values #869

zakjan opened this issue Nov 30, 2023 · 5 comments
Assignees

Comments

@zakjan
Copy link

zakjan commented Nov 30, 2023

Hi,

Constructing Cypher DSL statements has a performance overhead which makes it unusable in cycles. SDN suffers from the same issue, promotes creating a new Cypher DSL statement for every call. https://docs.spring.io/spring-data/neo4j/docs/7.1.5/reference/html/#sdn-mixins.using-cypher-dsl-statements

(I know, database queries shouldn't be used in cycles to avoid N+1 query problem, but in my case it's caused by legacy Neo4j usage patterns which originate from the embedded mode. To be refactored separately. 😮‍💨)

For constructing Cypher DSL statement just once and reusing with different parameter values, currently a wrapper class is necessary:

record CachedStatement(String cypher, Map<String, Object> parameters) {}

Usage:

class CarStatements {

    private static String NAME_KEY = "name";

    private static GET_CARS_STATEMENT = getCarsStatement();

    private static Statement getCarsStatement() {
        Parameter<?> nameParameter = Cypher.parameter(NAME_KEY);

        Node car = Cypher.node("Car").named("car");

        return Cypher.match(car).where(car.property("name").eq(nameParameter)).returning(car).build();
    }

    public static CachedStatement getCars(String name) {
        return new CachedStatement(GET_CARS_STATEMENT.getCypher(), Map.of(NAME_KEY, name));
    }
}

Is it feasible to add a method to Statement for creating a new instance with copied already rendered Cypher string and binding new parameter values, so that Cypher is reused? Potential usage:

    public static Statement getCars(String name) {
        return GET_CARS_STATEMENT.withParameters(Map.of(NAME_KEY, name));
    }

or

    private static Parameter<?> NAME_PARAMETER = Cypher.parameter("name");

    public static Statement getCars(String name) {
        return GET_CARS_STATEMENT.withParameters(Map.of(NAME_PARAMETER, name));
    }

Depends on how to share the parameters between query construction and binding. I wonder what could be the most idiomatic way.

@michael-simons
Copy link
Collaborator

Interesting question.
We do cache statements, but that doesn't solve the parameter issue.

@zakjan
Copy link
Author

zakjan commented Dec 4, 2023

Exactly. What's the cached query used for currently?

@michael-simons
Copy link
Collaborator

if you render the statement a second time (you get the cached string)

https://github.com/neo4j-contrib/cypher-dsl/blob/f278f67e8f0fa2d5fad2f46d0f5eaf94461d7993/neo4j-cypher-dsl/src/main/java/org/neo4j/cypherdsl/core/renderer/ConfigurableRenderer.java#L90-L96

You could just incorporate the parameters here, but to have them, you would need to visit the AST first, which kinda defeats the purpose.

Need to think about this a bit, it's at least 50% already there… If you have a good idea, happy to work on a PR.

@michael-simons
Copy link
Collaborator

michael-simons commented Dec 4, 2023

But wait… For what you are doing, wouldn't that work already?

@Test
void withArgs() {

	var movie = Cypher.node("Movie").named("n");
	var statement = Cypher.match(movie.where(
		movie.property("title").eq(Cypher.parameter("whatever")).or(movie.property("year").gte(Cypher.anonParameter(1999))))).returning(movie).build();

	var result1 = Renderer.getDefaultRenderer().render(statement);
	assertThat(result1).isSameAs(Renderer.getDefaultRenderer().render(statement));
	assertThat(result1).isEqualTo("MATCH (n:`Movie` WHERE (n.title = $whatever OR n.year >= $pcdsl01)) RETURN n");

	try(var driver = GraphDatabase.driver("neo4j://localhost", AuthTokens.none())) {
		for(String value : List.of("value1", "value2")) {
			// Get all the anonymous parameters and their values
			var parameters = new HashMap<>(statement.getCatalog().getParameters());
			// add all named
			parameters.put("whatever", value);
			// parameters are now {whatever=value1, pcdsl01=1999}
			driver.executableQuery(result1)
				.withParameters(parameters)
				.execute();
		}
	}
}

You just actually need to use the parameters for your properties :)

@zakjan
Copy link
Author

zakjan commented Dec 8, 2023

When it's inlined like this, yes. But consider that the part creating and running the DSL query is in a separate repository. Either a custom repository implementation or SDN.

for(String value : List.of("value1", "value2")) {
    movieRepository.getMovie(value);
}

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

No branches or pull requests

2 participants