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

Handle <bind> correctly inside <foreach> #2760

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

harawata
Copy link
Member

An attempt at fixing #2754
Related to #206 and #575

When processing DynamicSqlSource, evaluate param values in scripting phase and store them in ParameterMapping.
This does not affect RawSqlSource.

Just to be clear, it still is not possible to invoke a method inside a parameter reference
like #{_parameter.mymethod(_parameter.value)}.
It might be possible to accept OGNL expression in a param reference
(e.g. #{${_parameter.mymethod(_parameter.value)}}), but I'm not sure if that's a good idea.

This should not break backward compatibility for normal usages, however, plugins that manipulate internal stuff might be affected.

@emacarron @mnesarco
If this does not seem like the right approach to you, please let me know!

…phase

- Evaluated param values are stored in `ParameterMapping` and later used in DefaultParameterHandler
- There is no change when processing RawSqlSource
- Removed unused `injectionFilter` from TextSqlNode (mybatisgh-117)

This should fix mybatis#2754 .

This might also fix mybatis#206 and mybatis#575 , but with this patch, it still is not possible to invoke a method with parameter inside a parameter reference like `#{_parameter.mymethod(_parameter.value)}`.
It might be possible to accept OGNL expression in a param reference (e.g. `#{${_parameter.mymethod(_parameter.value)}}`), but I'm not sure if that's a good idea.
Although it is not the goal, this could improve performance when there are many foreach iterations.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.002%) to 87.315% when pulling e7f7622 on harawata:bind-in-foreach into 35262b0 on mybatis:master.

@mnesarco
Copy link
Member

Hi @harawata , there has been passed too many years since the last time I used or contributed to mybatis. I am afraid I cannot provide any useful nor updated info about this issue. The two-phase processing of dynamic queries was and is a very confusing topic for most users. I remember that there was a branch that process everything in the first pass caching bindings for the second pass... but it was never merged. I am talking about a very ancient history here.

@harawata
Copy link
Member Author

Thank you for the reply, @mnesarco !
It's a shame that you no longer use MyBatis, but your contributions live on. 🚀
I try not to bother you from now on :)

It sounds like the branch took a similar approach to this PR, but you don't recall the reason why it wasn't merged, do you?
I hope I don't miss some undesirable side effect caused by this.
Happy holidays! 🎄

@mnesarco
Copy link
Member

It was long long time ago and I cannot recall the details, but I think it was not discussed enough and there were possible performance implications. I was trying to solve the same problem with bind tag. The basic idea was to implement bindings as function calls in the first phase that cache the output and assign a generated key to use at second phase, I did a POC but unfortunately I have no access to that code anymore.

I started using JPA because of work requirements some years ago and I ended up implementing an internal tool to support native and jpql dynamic queries similar to Mybatis :D ... JPA has evolved to support also Stored Procedures and very felxible mapping scenarios... But I still miss some mybatis tricks....

BTW, java17's Records are quite sexy but not directly supported by JPA. Does current mybatis supports Record mappings with all the usual magic?

Happy Holidays!

@harawata
Copy link
Member Author

Writing a custom tool for JPA sounds like you 😆

I couldn't think of any scenario that performs worse because of this change.
I actually expect the new <foreach> to perform even better when there are many iterations, but I should probably do some profiling.

Regarding the Records, SELECT should work the same way as classic immutable classes.
But assigning generated keys (i.e. useGeneratedKeys) is not supported. It seems technically difficult as the Records does not allow field value modification even with reflection.
With some DBs, as a workaround, it is possible to obtain new Record instance(s) with generated keys using syntax like INSERT ... RETURNING.

@mnesarco
Copy link
Member

Yeah generated keys are usually problematic. I am using a dedicated service for generated keys in many projects. And I am considering TSID[1] as a general approach. Vlad Mihalcea posted an interesting article about that[2].

[1] https://github.com/f4b6a3/tsid-creator
[2] https://vladmihalcea.com/uuid-database-primary-key/

But this is off topic. :D

@harawata
Copy link
Member Author

I did a quick profiling with JMH.

<insert id="insertMultiRow">
  insert into person (
    id, name1, name2, name3, name4, name5, name6, name7, name8, name9
  ) values
  <foreach item="item" collection="list" separator=",">
    (#{item.id}, #{item.name1}, #{item.name2}, #{item.name3}, #{item.name4},
     #{item.name5}, #{item.name6}, #{item.name7}, #{item.name8}, #{item.name9})
  </foreach>
</insert>

Only 500 items in the list.

// 3.5.11
Benchmark                                             Mode  Cnt         Score       Error   Units
BenchmarkMybatis.insertMultiRow                      thrpt   25        11.968 ±     0.325   ops/s
BenchmarkMybatis.insertMultiRow:·gc.alloc.rate       thrpt   25       258.661 ±     7.381  MB/sec
BenchmarkMybatis.insertMultiRow:·gc.alloc.rate.norm  thrpt   25  22661054.886 ± 81879.956    B/op
BenchmarkMybatis.insertMultiRow:·gc.count            thrpt   25       398.000              counts
BenchmarkMybatis.insertMultiRow:·gc.time             thrpt   25       348.000                  ms

// with this PR
Benchmark                                             Mode  Cnt         Score       Error   Units
BenchmarkMybatis.insertMultiRow                      thrpt   25        12.018 ±     1.310   ops/s
BenchmarkMybatis.insertMultiRow:·gc.alloc.rate       thrpt   25       128.307 ±    13.920  MB/sec
BenchmarkMybatis.insertMultiRow:·gc.alloc.rate.norm  thrpt   25  11196004.604 ± 18771.486    B/op
BenchmarkMybatis.insertMultiRow:·gc.count            thrpt   25       760.000              counts
BenchmarkMybatis.insertMultiRow:·gc.time             thrpt   25       766.000                  ms

Throughput is pretty much the same and allocated memory is reduced significantly.
I plan to merge this soon. 🤞

@kazuki43zoo Please let me know if you noticed anything (even after this PR is merged).

@harawata
Copy link
Member Author

I am going to have to investigate the impact on mybatis-velocity and mybatis-thymeleaf first.

@GeorgeSalu
Copy link

hello @harawata , is there any benchmark about these mybatis velocity, thymeleaf and freemarker template plugins? Can I think that using the default mybatis solution will always be the most performant? sorry if the question is not related to the issue but we are studying mybatis to use in our new projects

@harawata
Copy link
Member Author

Regarding the other language drivers...

  • mybatis-velocity is not affected by this change
  • mybatis-thymeleaf and mybatis-freemarker need a little adjustment. I'll open PRs after this PR is merged.

@GeorgeSalu ,
I don't know any benchmark comparing language drivers.
In terms of performance, the built-in XML language driver may have some advantages, but I am not sure if it "always" performs better than the others.
Performance depends on various factors and it's best to profile your application under the conditions close to the actual usage scenario.

@harawata harawata added this to the 3.6.x milestone Jan 2, 2023
@harawata
Copy link
Member Author

harawata commented Jan 3, 2023

As this change affects mybatis-freemarker and mybatis-thymeleaf, the target milestone is set to 3.6.x.
This is not a done deal, so further discussion is welcome.

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

Successfully merging this pull request may close these issues.

4 participants