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, e557f49, c3bb671, 223528a, f26b32c, 896bd02, 9121fb2, d0cb598]

› 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?

[yrodiere]

Steve is on PTO, let's try to figure it out ourselves.

@NathanQingyangXu I understand that, with this PR, we now have an integration of ParameterMarkerStrategy in native queries, but that integration will fail in some cases when some SQL is inserted before the parameters.

My question is this: could this possibly make the behavior worse in some cases?
Obviously in cases where the parameter marker strategy is needed, it can't be worse: at worst it will just fail a different way.
But in cases where the parameter marker strategy is not needed (the JDBC driver expects? markers)... could this PR trigger a failure?

The best way to be sure would probably be to test this using one of the examples Steve gave?

[NathanQingyangXu]

The situation can't be worse. In vast majority of cases, it works as expected. Currently only dialects that have such native quuey parameter marker customization are restricted to H2 and PG. The super special LimitHandlers that require inserting SQL at the beginning invariably don't require native parameter marker strategy at all. They only accept ? JDBC marker. There are edge cases wherein error still shows up, when we need to insert LimiHandler SQL to the end (case 1 in my last message). This could be solved completely by another simple commit, but I didn't do that for the simple reason that it only solves case 1, not case 2 or inserting SQL at the beginning. But as I said, case 2 doesn't show up in reality for now. Hope I explained clearly. I think we can merge this PR (Christian used to review its logic and didn't find fault). For the extremely rare case 1, we could solve it when user complains.

…

On Mon, Jun 30, 2025, 7:19 a.m. Yoann Rodière ***@***.***> wrote: *yrodiere* left a comment (hibernate/hibernate-orm#8798) <#8798 (comment)> Steve is on PTO, let's try to figure it out ourselves. @NathanQingyangXu <https://github.com/NathanQingyangXu> I understand that, with this PR, we now have an integration of ParameterMarkerStrategy in native queries, but that integration will fail in some cases when some SQL is inserted before the parameters. My question is this: could this possibly make the behavior *worse* in some cases? Obviously in cases where the parameter marker strategy is needed, it can't be worse: at worst it will just fail a different way. But in cases where the parameter marker strategy is not needed (the JDBC driver expects? markers)... could this PR trigger a failure? The best way to be sure would probably be to test this using one of the examples Steve gave? — Reply to this email directly, view it on GitHub <#8798 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB6UYAXKMY6CIKAP4NDZNM33GEMMHAVCNFSM6AAAAAB7YT7IMCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTAMJYG43DSOJUHA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[NathanQingyangXu]

I think I can add a simple commit to solve case 1 and given case 2 doesn't need to be solved (only a few dialects belong to this group, but they have no native marker customization requirement at all!). If you have no objection I would do that. It would make for a perfect solution without any compromise. Needless to say, I will add compelling testing case to prove. On Mon, Jun 30, 2025, 6:04 p.m. Nathan Xu ***@***.***> wrote:

…

The situation can't be worse. In vast majority of cases, it works as expected. Currently only dialects that have such native quuey parameter marker customization are restricted to H2 and PG. The super special LimitHandlers that require inserting SQL at the beginning invariably don't require native parameter marker strategy at all. They only accept ? JDBC marker. There are edge cases wherein error still shows up, when we need to insert LimiHandler SQL to the end (case 1 in my last message). This could be solved completely by another simple commit, but I didn't do that for the simple reason that it only solves case 1, not case 2 or inserting SQL at the beginning. But as I said, case 2 doesn't show up in reality for now. Hope I explained clearly. I think we can merge this PR (Christian used to review its logic and didn't find fault). For the extremely rare case 1, we could solve it when user complains. On Mon, Jun 30, 2025, 7:19 a.m. Yoann Rodière ***@***.***> wrote: > *yrodiere* left a comment (hibernate/hibernate-orm#8798) > <#8798 (comment)> > > Steve is on PTO, let's try to figure it out ourselves. > > @NathanQingyangXu <https://github.com/NathanQingyangXu> I understand > that, with this PR, we now have an integration of ParameterMarkerStrategy > in native queries, but that integration will fail in some cases when some > SQL is inserted before the parameters. > > My question is this: could this possibly make the behavior *worse* in > some cases? > Obviously in cases where the parameter marker strategy is needed, it > can't be worse: at worst it will just fail a different way. > But in cases where the parameter marker strategy is not needed (the JDBC > driver expects? markers)... could this PR trigger a failure? > > The best way to be sure would probably be to test this using one of the > examples Steve gave? > > — > Reply to this email directly, view it on GitHub > <#8798 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AB6UYAXKMY6CIKAP4NDZNM33GEMMHAVCNFSM6AAAAAB7YT7IMCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTAMJYG43DSOJUHA> > . > You are receiving this because you were mentioned.Message ID: > ***@***.***> >

[yrodiere]

If you have no objection I would do that. It would make for a perfect solution without any compromise. Needless to say, I will add compelling testing case to prove.

If it doesn't involve breaking SPI, and it's tested, I'm all for it. Thanks.

case 2 doesn't
need to be solved (only a few dialects belong to this group, but they have
no native marker customization requirement at all!).

Note SQL Server requires custom markers in Hibernate Reactive: https://github.com/hibernate/hibernate-reactive/blob/e257189f643346799703eab7805de6c4b74a8b33/hibernate-reactive-core/src/main/java/org/hibernate/reactive/provider/service/NativeParametersHandling.java#L57-L59

I imagine that doesn't change anything to your assessment?

[NathanQingyangXu]

Note SQL Server requires custom markers in Hibernate Reactive: https://github.com/hibernate/hibernate-reactive/blob/e257189f643346799703eab7805de6c4b74a8b33/hibernate-reactive-core/src/main/java/org/hibernate/reactive/provider/service/NativeParametersHandling.java#L57-L59

I imagine that doesn't change anything to your assessment?

SQL Server's top(?) feature implies the corresponding LimitHandler will insert it at the beginning of SQL. That made me very sad.

However, close inspection shows that in Hibernate Reactive it is SQLServerDialect that is used, whose LimitHandler is OffsetFetchLimitHandler which has no such concern (SQLServerLegacyDialect would have such concern but it is not used).

So yeah, we can solve all the following dialects used in Hibernate Reactive: (all of the below dialects have OffsetLimitHandler which belongs to case 1 or appending limit jdbc parameters to the end of SQL).

• Postgresql
• Cockroach
• SQLServer

by adding a new default method to LimitHandler as below:

default String processSql(String sql, int jdbcParameterBindingsCnt, ParameterMarkerStrategy parameterMarkerStrategy, Limit limit, QueryOptions queryOptions) {
    return processSql( sql, limit, queryOptions );
}	

Is such change breaking spi? The new method will be only used in DeferredResultSetAccess class which is internal.
Besides we need to change OffsetFetchLimitHandler without touching other LimitHandler for up to now only OffsetFetchLimitHandler is relevant to native query marker customization.

[beikov]

Is such change breaking spi?

You're only adding default methods to the SPI, not changing existing methods, so this is fine.

[yrodiere]

by adding a new default method to LimitHandler as below:

default String processSql(String sql, int jdbcParameterCnt, ParameterMarkerStrategy parameterMarkerStrategy, Limit limit, QueryOptions queryOptions) {
    return processSql( sql, limit, queryOptions );
}	

It's fine, but people will still be forced to implement the old processSql, which is bad. Please see my suggestion above: #8798 (comment)

[NathanQingyangXu]

@Deprecated // Never called directly by Hibernate ORM
-	

Yeah. Forgive an old man's bad memory and poor eyesights.
I just pushed a new commit.
I didn't change the SQLServerDialect (and maybe Cockroach), but I confirmed the code changes in this PR should apply to both. You could change on top of this PR for I am not sure about the potential implications.

Feel free to take over or change the code logic in whatever way. I created a new separate NativeParameterMarkerTests class to showcase the effectiveness; currently it covers both H2 and Postgresql.