HHH-16283 - Integrate ParameterMarkerStrategy into NativeQuery

[NathanQingyangXu]

https://hibernate.atlassian.net/browse/HHH-16283

A straightforward implementation. Two challenges stand out:

• native query parameter expanding
• LimitHandler needs to be refactored to make it possible to be parameterized

The second concern is tricky so a separatr ticket (https://hibernate.zulipchat.com/#narrow/stream/132094-hibernate-orm-dev/topic/Custom.20functions) was created, and this PR only focuses on the first goal.

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license
and can be relicensed under the terms of the LGPL v2.1 license in the future at the maintainers' discretion.
For more information on licensing, please check here.

---

[sebersole]

The "least messy" way to deal with LimitHandler will be adding a method to QueryOptions :

interface QueryOptions {
    ...
    ParameterMarkerStrategy getParameterMarkerStrategy();
}

I think ideally we'd add a ParameterMarkerStrategy argument to the LimitHandler#processSql method, but that contract is technically public and we should avoid changing these signatures. And that's a change that should definitely not be in any 6.x work. But since 7 is a new major, we could consider such a change there.

[NathanQingyangXu]

The "least messy" way to deal with LimitHandler will be adding a method to QueryOptions :

interface QueryOptions {
    ...
    ParameterMarkerStrategy getParameterMarkerStrategy();
}

I think ideally we'd add a ParameterMarkerStrategy argument to the LimitHandler#processSql method, but that contract is technically public and we should avoid changing these signatures. And that's a change that should definitely not be in any 6.x work. But since 7 is a new major, we could consider such a change there.

Fair enough. Another issue is during LimitHandler#processSql, we also need to know the starting position, which seems inappropriate to be put into QueryOptions?

[sebersole]

we also need to know the starting position

That's problematic though. Considering different databases put the limit clause in different places, starting position relative to what?

[NathanQingyangXu]

it is the starting position for any parameter marker created in LimitHandler. E.g. the following native query is issued prior to LimitHandler processing: *select * form book where author = ?1 and year = ?2* so after LimitHandler processing, the following example SQL should be generated: *select * form book where author = ?1 and year = ?2 offset ?3 limit ?4* *as opposed to the invalid* *select * form book where author = ?1 and year = ?2 offset ? limit ?* Then LimitHandler needs to know that the starting position is 3, for the strategy method requires two parameters: 1) position; 2) JdbcType I assume the JdbcType could be Integer in LimitHandler but the starting position is vital. Usually it could be obtained by JdbcParameterBindings. Currently I only use it on OffsetLimitHandler for now only H2 and Postgres needs customization and both dialects correspond to OffsetLimitHandler. It should be straightforward to change every LimitHandler but I guess it would hurt perf and unnecessary? Lots of gray areas for this ticket.

…

On Mon, Sep 16, 2024 at 9:22 AM Steve Ebersole ***@***.***> wrote: we also need to know the starting position That's problematic though. Considering different databases put the limit clause in different places, starting position relative to what? — Reply to this email directly, view it on GitHub <#8798 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB6UYAVDGAZ7UAKYD3DFBALZW3LQRAVCNFSM6AAAAABMYXHWDSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNJSHEYDMNJUGE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[sebersole]

...
select * form book where author = ?1 and year = ?2 offset ?3 limit ?4

My point was that for some databases the limit clause occurs in different positions within the SQL, such as skip/first, e.g.:

select first ?1 skip?2 * form book where author = ?3 and year = ?4

[NathanQingyangXu]

...
select * form book where author = ?1 and year = ?2 offset ?3 limit ?4

My point was that for some databases the limit clause occurs in different positions within the SQL, such as skip/first, e.g.:

select first ?1 skip?2 * form book where author = ?3 and year = ?4

Yeah, I agree. We have decided to refactor LimitHandler related stuff out of this PR or ticket (we created a new ticket assigned to you). This PR had been refactored to only focus on normal parameter processing and collection type parameter expansion.

[yrodiere]

This PR had been refactored to only focus on normal parameter processing and collection type parameter expansion.

Nice. There wasn't any comment since then though... I suspect I'm missing some information?

Hey @NathanQingyangXu @sebersole do you know what's the status of this PR? I mean I see it needs to be rebased, but are you aware of other problems that will prevent us from merging it in 7.1?

Asking because this would allow us to get rid of SQL parsing/rewriting in Hibernate Reactive, which is causing bugs (hibernate/hibernate-reactive#2012) and fragility (hibernate/hibernate-reactive#2315) in Hibernate Reactive.

I think ideally we'd add a ParameterMarkerStrategy argument to the LimitHandler#processSql method, but that contract is technically public and we should avoid changing these signatures. And that's a change that should definitely not be in any 6.x work. But since 7 is a new major, we could consider such a change there.

Looks like this went through the cracks... Do you think we could do this in 7.1?

+	@Deprecated // Never called directly by Hibernate ORM
-	String processSql(String sql, Limit limit);
+	default String processSql(String sql, Limit limit) {
+ 		throw new AssertionFailure(getClass().getName() + " is missing an implementation for processSql()");
+ 	}
 
+	@Deprecated // Never called directly by Hibernate ORM
 	default String processSql(String sql, Limit limit, QueryOptions queryOptions) {
 		return processSql( sql, limit );
 	}

+       // This is the one called directly by Hibernate ORM
+	default String processSql(String sql, Limit limit, ParameterMarkerStrategy parameterMarkerStrategy) {
+		return processSql(sql, limit, parameterMarkerStrategy);
+ 	}

[NathanQingyangXu]

Nothing needs to be done further except for syncing up with v7 series. I'll rebase and refactor as you proposed soon. Had thought this PR would be a dead one, :).

[hibernate-github-bot]

Thanks for your pull request!

This pull request does not follow the contribution rules. Could you have a look?

❌ All commit messages should start with a JIRA issue key matching pattern HHH-\d+
    ↳ Offending commits: [668ff21]

› This message was automatically generated.

[NathanQingyangXu]

@yrodiere , I synced up with main branch and resolved the conflict.
However, I just recollect that there is a tricky issue in LimitHandler in the sense that we need to know the start parameter position for limit markers if they are to be appended to the sql. More complex case shows up when the limit sql snippet is to be inserted at the beginning (e.g. after select), then all the existing markers in the sql need to be recomputed. See the previous discussion with Steve for some examples.

That is why a separate ticket was created at https://hibernate.atlassian.net/browse/HHH-18624, and this PR only focuses on the following two scenarios:

• normal marker customization whose parameter was assigned one value
• marker customization whose parameter was assigned multi-values

[yrodiere]

Okay, thanks for the summary.

Hey @sebersole, do I understand correctly this is still making a worthwhile improvement, as in "it's broken but less than it used to be"? Okay to merge on your end?