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

Support for associations and collections filled with multiple result sets #27

Closed
wants to merge 3 commits into from
Closed

Conversation

emacarron
Copy link
Member

Hi.

The purpose of this change is adding support for filling associations and collections out of multiple result sets. (see issue #512 in gcode)

Suppose we have the tipical N+1 problem: An order that has order lines.

Right now MyBatis provides two ways of solving this.

  • A nested select
  • A join (nested result map)

Both work but have their drawbacks.

  • The nested select implies executing N+1 statements that kills performance.
  • The join gets more data than needed and it is hard to process and get the entities back out of the result.

The proposal is to add the third way:

First, two sentences are sent to the database at the same time.

<select id="getOrders" resultMap="orderResult">
    select * from mbtest.order_header ;
    select * from mbtest.order_detail
</select>

Which causes an execution like this one:

DEBUG [main] - ==>  Preparing: select * from mbtest.order_header ; select * from mbtest.order_detail 
DEBUG [main] - ==> Parameters: 
TRACE [main] - <==    Columns: order_id, cust_name
TRACE [main] - <==        Row: 1, Fred
TRACE [main] - <==        Row: 2, Barney
TRACE [main] - <==    Columns: order_id, line_number, quantity, item_description
TRACE [main] - <==        Row: 1, 1, 1, Pen
TRACE [main] - <==        Row: 1, 2, 3, Pencil
TRACE [main] - <==        Row: 1, 3, 2, Notepad
TRACE [main] - <==        Row: 2, 1, 1, Compass
TRACE [main] - <==        Row: 2, 2, 1, Protractor
TRACE [main] - <==        Row: 2, 3, 2, Pencil
DEBUG [main] - Resetting autocommit to true on JDBC Connection [org.postgresql.jdbc3.Jdbc3Connection@7f1ee770]
DEBUG [main] - Closing JDBC Connection [org.postgresql.jdbc3.Jdbc3Connection@7f1ee770]```

Note that MyBatis already supports multiple resultsets with this notation:

<select id="getOrders" resultMap="orderResult,orderDetail">
    select * from mbtest.order_header ;
    select * from mbtest.order_detail
</select>

This will produce a List with two lists, each one filled with one type of object (Order, OrderDetail) and no relationships between them.

In order to crate a full object graph I am proposing to include some new attributes in the associations or collection elements:

  • resultSet indidcates the name of the result set where the data comes in
  • column (this one already exists) identifies the key (as it does when using a nested select)
  • foreignColum identifies the column to match to build relations between objects.

And another one in the statement:

  • resultSets list of names separated by commas that will identify the resultsets.
<select id="getOrders" resultSets="orders,details" resultMap="orderResult">
    select * from mbtest.order_header ;
    select * from mbtest.order_detail
</select>

<resultMap type="order" id="orderResult">
    <id column="order_id" property="orderId" />
    <result column="cust_name" property="customerName" />
    <collection property="detailLines" column="order_id" foreignColumn="order_id" resultMap="orderDetail" resultSet="details" />
</resultMap>

For the suspicious, this is not ORM at all. The intention is not to map objects to tables but to resultsets.

What do you think?

@emacarron emacarron closed this Apr 7, 2013
@emacarron emacarron reopened this Apr 7, 2013
@emacarron
Copy link
Member Author

From the dev list:

"We have been discussing this feature off-line. Let me provide the
relevant info for others to follow the topic.

I do agree with Iwao in that probably this feature can be used in such
a few trivial cases that it is not worth adding it.

But, it can be really useful for stored procs that return more than
one result set so that MyBatis gets all resultsets and build one
object graph out of them.

I will try this way to see where it gets and let you know."

@emacarron emacarron closed this Apr 10, 2013
@emacarron
Copy link
Member Author

Patch applied in fd34445

@ghost ghost assigned emacarron Apr 20, 2013
@chhex
Copy link

chhex commented Dec 16, 2021

This change has broken a significant code basis for us, which we now have to migrate because of this backward incompatibility. Why simply a new Annotation? How many really need a List of resultMap?

@harawata
Copy link
Member

Hello @chhex ,

This feature is crucial for a stored procedure that returns multiple result sets.
https://stackoverflow.com/q/69145531/
In any case, it is highly unlikely that we remove this feature after eight years.

I'm wondering how this change could break backward compatibility. Could you elaborate?
One possibility, of course, is that you include a comma in a result map ID, but it should not be too hard to replace it with underscore or something before upgrading to the latest version.

@chhex
Copy link

chhex commented Dec 20, 2021

Hey @harawata

Because of the large Code Basis we have, and the Mapper Files being in many different jars, we load and parse the Mapper Interfaces dynamically with the Mybatis Configuration API. I could look up the details. This worked fine up-to 3.2.2. With this change we get a Java Type Error upon parsing the ResultSet Annotation:

java.lang.annotation.AnnotationTypeMismatchException: Incorrectly typed data found for annotation element public abstract java.lang.String[] org.apache.ibatis.annotations.ResultMap.value()

kind of understandable, isn’t it? Yes a String Array isn’t a String, maybe in some scenarios, you look a String as a single String in a String vararg, but clearly from Type Declaration resp Annotation point of view the two are different.

This could have be avoided with for example a new Annotation, which it effectively is from a Java point view. Or have the same Annotation with two arguments, a String and a String Array , whatever.

Importance / Criticality is always realative, specially in Software Engineering.

Thanks!

@harawata
Copy link
Member

harawata commented Dec 21, 2021

Hello @chhex ,

Thank you for the explanation!
I think you are talking about #43 .

The change (i.e. replacing String with String[]) retains source-code compatibility which means that the same code still works. You just need to recompile it.
I'm sorry that this binary-code incompatibility surprised you, however, the change looks reasonable and I wouldn't have done it in any other way.

FYI, for recent versions, we list backward incompatible changes in the release note when there is any, but there may be some unlisted changes that break binary-code compatibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improve a feature or add a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants