Skip to content

Conversation

polyglot-k
Copy link
Contributor

@polyglot-k polyglot-k commented Sep 25, 2025

This pull request refactors the logic for determining whether a given ParserRuleContext represents a subquery in the JpqlQueryRenderer class. The main change is a simplification and clarification of the isSubquery method's traversal logic.

Refactoring and logic simplification:

Refactored the isSubquery method to use an explicit while loop for traversing parent contexts, making the logic easier to follow and avoiding recursive calls.

The method now returns immediately when encountering a SubqueryContext, Update_statementContext, or Delete_statementContext, and only continues traversing if none of these are found.

By replacing recursion with an iterative approach, the method avoids continuously adding frames to the call stack, improving efficiency and stack safety for deeply nested contexts.

  • You have read the Spring Data contribution guidelines.
  • You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
  • You submit test cases (unit or integration tests) that back your changes.
  • You added yourself as author in the headers of the classes you touched. Amend the date range in the Apache license header if needed. For new types, add the license header (copy from another file and set the current year only).

@mp911de
Copy link
Member

mp911de commented Sep 26, 2025

We have similar code in EqlQueryRenderer and HqlQueryRenderer. Care to address these as well?

@mp911de mp911de added type: task A general task and removed status: waiting-for-triage An issue we've not yet triaged labels Sep 26, 2025
@mp911de mp911de self-assigned this Sep 26, 2025
@polyglot-k
Copy link
Contributor Author

@mp911de of course ! i can do that

@polyglot-k
Copy link
Contributor Author

@mp911de
I have fixed all the places where this structure was needed.

I have one question: in the HqlQueryRenderer part, the isSetQuery method is always used as !isSetQuery. Was this maintained for the sake of consistency, or would it be more intuitive to include the negation in the method name itself?

@mp911de
Copy link
Member

mp911de commented Sep 26, 2025

Care to use tabs for indents instead of spaces? Other than that, the pull request looks ready for merge.

@polyglot-k
Copy link
Contributor Author

@mp911de I reflected it!

… JpqlQueryRenderer for improved readability

Signed-off-by: KNU-K <[email protected]>
@polyglot-k polyglot-k requested a review from mp911de September 26, 2025 10:16
…lQueryRenderer for improved readability

Signed-off-by: KNU-K <[email protected]>
@mp911de
Copy link
Member

mp911de commented Sep 26, 2025

I have one question: in the HqlQueryRenderer part, the isSetQuery method is always used as !isSetQuery. Was this maintained for the sake of consistency, or would it be more intuitive to include the negation in the method name itself?

That's the best name we were able to come up. isNotSetQuery is worse to read than !isSetQuery. We're always happy to get suggestions how to improve.

@polyglot-k
Copy link
Contributor Author

I have one question: in the HqlQueryRenderer part, the isSetQuery method is always used as !isSetQuery. Was this maintained for the sake of consistency, or would it be more intuitive to include the negation in the method name itself?

That's the best name we were able to come up. isNotSetQuery is worse to read than !isSetQuery. We're always happy to get suggestions how to improve.

Yes, I understand that !
Will it merge if I leave it as it is?
@mp911de

@mp911de mp911de changed the title Refactor subquery detection logic in JpqlQueryRenderer Replace recursion in QueryRenderer.isSubquery(…) with loop Sep 29, 2025
@mp911de mp911de closed this in 1334008 Sep 29, 2025
mp911de added a commit that referenced this pull request Sep 29, 2025
Reformat code and reorder author tags.

See #4025
mp911de pushed a commit that referenced this pull request Sep 29, 2025
mp911de added a commit that referenced this pull request Sep 29, 2025
Reformat code and reorder author tags.

See #4025
@mp911de mp911de added this to the 3.5.5 (2025.0.5) milestone Sep 29, 2025
@mp911de
Copy link
Member

mp911de commented Sep 29, 2025

Thank you for your contribution. That's merged, polished, and backported now.

@polyglot-k
Copy link
Contributor Author

@mp911de oh! thank u

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

Successfully merging this pull request may close these issues.

3 participants