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

Ignore empty text node when parsing XML nodes #3349

Merged
merged 2 commits into from
Dec 31, 2024

Conversation

nieqiurong
Copy link
Contributor

When parsing XML, since nested nodes have line breaks by default, these nodes should not be parsed as text nodes and added, which will cause unnecessary heap space to be occupied.

image

The test project reduces the creation of this node, and the space occupied by this node is reduced from 1625KB to 748KB

Snipaste_2024-12-29_14-04-38
Snipaste_2024-12-29_14-06-39

@coveralls
Copy link

coveralls commented Dec 29, 2024

Coverage Status

coverage: 87.33% (+0.02%) from 87.31%
when pulling 5c757a3 on nieqiurong:20241229
into e08b503 on mybatis:master.

@hazendaz hazendaz requested a review from harawata December 31, 2024 00:27
@hazendaz
Copy link
Member

@harawata Any concerns here? Its using trim so it handles all whitespace type which would probably be more appropriate title here.

@harawata harawata changed the title Skip newline characters in dynamic node parsing Ignore empty text node when parsing XML nodes Dec 31, 2024
@harawata harawata added the polishing Improve a implementation code or doc without change in current behavior/content label Dec 31, 2024
Copy link
Member

@harawata harawata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @nieqiurong ,

This seems like a good optimization without affecting the final SQL.
Nice work!

@hazendaz hazendaz merged commit 27ca11c into mybatis:master Dec 31, 2024
19 checks passed
@nieqiurong
Copy link
Contributor Author

thanks

@nieqiurong
Copy link
Contributor Author

Hello @nieqiurong ,

This seems like a good optimization without affecting the final SQL. Nice work!

I think I missed compatibility considerations. If the label does not wrap, the statements will be connected together.

<!--parse: and id is not null and name is not null-->
<where>
    <if test="1==1">
        and id is not null
    </if>
    <if test="1==1">
        and name is not null
    </if>
</where>
<!-- parse: and id is not nulland name is not null-->
<where>
    <if test="1==1">and id is not null</if>
    <if test="1==1">and name is not null</if>
</where>

It may be better to change it to the following, so that there is no need to cooperate with Configuration#isShrinkWhitespacesInSql to remove line breaks

String data = child.getStringBody("").trim();
if (data.isEmpty()) {
  continue;
}
data += " ";

@epochcoder
Copy link
Contributor

epochcoder commented Jan 2, 2025

@nieqiurong The suggestion you posted would introduce extra spaces where it would not have normally been before this PR. In this case I think it is better to revert this. wdyt @harawata?

@nieqiurong
Copy link
Contributor Author

@nieqiurong您发布的建议将引入额外的空间,而在此 PR 之前通常不会出现这些空间。在这种情况下,我认为最好恢复此状态。瓦迪特@harawata

I agree with your suggestion, but can we add a configuration to decide whether to enable this operation, or provide an interface to support processing?

@harawata
Copy link
Member

harawata commented Jan 2, 2025

Yeah, adding extra space does not look great.

It's interesting that it only happens when <where> is used.
So, this is OK.

where 1=1
<if test="1==1">and id is not null</if>
<if test="1==1">and name is not null</if>

We might be able to workaround this in a different place.

In any case, if this PR changes the final SQL, it would be better to revert it and release 3.5.19.

@nieqiurong
Copy link
Contributor Author

I agree, but can you consider removing \n? For example, some parser frameworks will have problems with continuous line breaks.

@harawata
Copy link
Member

harawata commented Jan 2, 2025

Can't you use shrinkWhitespacesInSql?

Find out why it only happens when <where> is used.
Then you might be able to find a solution.

@nieqiurong
Copy link
Contributor Author

Thank you, I will look into it. You can roll back the code first to ensure compatibility.

epochcoder added a commit to epochcoder/mybatis-3 that referenced this pull request Jan 2, 2025
Regression when where node is used without new lines
epochcoder added a commit that referenced this pull request Jan 2, 2025
epochcoder added a commit that referenced this pull request Jan 2, 2025
Regression when where node is used without new lines

(cherry picked from commit 398fe8e)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
polishing Improve a implementation code or doc without change in current behavior/content
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants