-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
HHH-19850 replace arraylist by bitSet when resolving dirty attribute indexes #11104
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2434,7 +2434,8 @@ public int[] resolveDirtyAttributeIndexes( | |
| attributeNames == null | ||
| ? 0 | ||
| : attributeNames.length + mutablePropertiesIndexes.cardinality(); | ||
| final List<Integer> fields = new ArrayList<>( estimatedSize ); | ||
| final BitSet fields = new BitSet(); | ||
|
|
||
| if ( estimatedSize == 0 ) { | ||
| return EMPTY_INT_ARRAY; | ||
| } | ||
|
|
@@ -2446,7 +2447,7 @@ public int[] resolveDirtyAttributeIndexes( | |
| i = mutablePropertiesIndexes.nextSetBit(i + 1) ) { | ||
| // This is kindly borrowed from org.hibernate.type.TypeHelper.findDirty | ||
| if ( isDirty( currentState, previousState, propertyTypes, propertyCheckability, i, session ) ) { | ||
| fields.add( i ); | ||
| fields.set( i ); | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -2479,8 +2480,8 @@ class ChildEntity extends SuperEntity { | |
| final String attributeName = attributeMapping.getAttributeName(); | ||
| if ( isPrefix( attributeMapping, attributeNames[index] ) ) { | ||
| final int position = attributeMapping.getStateArrayPosition(); | ||
| if ( propertyUpdateability[position] && !fields.contains( position ) ) { | ||
| fields.add( position ); | ||
| if ( propertyUpdateability[position] && !fields.get( position ) ) { | ||
| fields.set( position ); | ||
| } | ||
| index++; | ||
| if ( index < attributeNames.length ) { | ||
|
|
@@ -2503,16 +2504,36 @@ class ChildEntity extends SuperEntity { | |
| else { | ||
| for ( String attributeName : attributeNames ) { | ||
| final Integer index = getPropertyIndexOrNull( attributeName ); | ||
| if ( index != null && propertyUpdateability[index] && !fields.contains( index ) ) { | ||
| fields.add( index ); | ||
| if ( index != null && propertyUpdateability[index] && !fields.get( index ) ) { | ||
| fields.set( index ); | ||
|
Comment on lines
+2507
to
+2508
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See previous comment |
||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return toIntArray( fields ); | ||
| return bitSetToIntArray( fields ); | ||
| } | ||
|
|
||
| /** | ||
| * Converts a BitSet to an int array containing the indices of all set bits. | ||
| * The resulting array is naturally sorted since we iterate through set bits in order. | ||
| * | ||
| * @param bitSet the BitSet to convert | ||
| * @return an int array containing the indices of set bits, or EMPTY_INT_ARRAY if none | ||
| */ | ||
| private static int[] bitSetToIntArray(final BitSet bitSet) { | ||
| final int cardinality = bitSet.cardinality(); | ||
| if ( cardinality == 0 ) { | ||
| return EMPTY_INT_ARRAY; | ||
| } | ||
|
|
||
| final int[] result = new int[cardinality]; | ||
| int arrayIndex = 0; | ||
| for ( int i = bitSet.nextSetBit(0); i >= 0; i = bitSet.nextSetBit(i + 1) ) { | ||
| result[arrayIndex++] = i; | ||
| } | ||
| return result; | ||
| } | ||
| private boolean isDirty( | ||
| Object[] currentState, | ||
| Object[] previousState, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,170 @@ | ||
| /* | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| * Copyright Red Hat Inc. and Hibernate Authors | ||
| */ | ||
| package org.hibernate.persister.entity; | ||
|
|
||
| import org.hibernate.engine.spi.SessionImplementor; | ||
| import org.hibernate.testing.orm.junit.JiraKey; | ||
| import org.hibernate.testing.junit4.BaseCoreFunctionalTestCase; | ||
| import org.junit.Before; | ||
| import org.junit.Test; | ||
|
|
||
| import jakarta.persistence.Entity; | ||
| import jakarta.persistence.GeneratedValue; | ||
| import jakarta.persistence.Id; | ||
| import jakarta.persistence.Table; | ||
|
|
||
| import java.util.Arrays; | ||
|
|
||
| import static org.assertj.core.api.Assertions.assertThat; | ||
|
|
||
| /** | ||
| * @author El Mehdi KZADRI | ||
| */ | ||
| @JiraKey("HHH-19850") | ||
| public class AbstractEntityPersisterTest extends BaseCoreFunctionalTestCase { | ||
|
|
||
| private AbstractEntityPersister persister; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We already have existing tests which should check the behavior of the method you changed, no need to create one which artificially tests it through internal-only calls. It would be great if you could modify an existing one instead, adding any assertions if needed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I see ! |
||
|
|
||
| @Override | ||
| protected Class<?>[] getAnnotatedClasses() { | ||
| return new Class[] { TestEntity.class }; | ||
| } | ||
|
|
||
| @Before | ||
| @SuppressWarnings("resource") | ||
| public void setUp() { | ||
| persister = (AbstractEntityPersister) sessionFactory() | ||
| .getMappingMetamodel() | ||
| .getEntityDescriptor(TestEntity.class.getName()); | ||
| } | ||
|
|
||
| @Test | ||
| public void testResolveDirtyAttributeIndexes_EmptyAttributeNames() { | ||
| inTransaction( entityManager -> { | ||
| SessionImplementor session = entityManager.unwrap(SessionImplementor.class); | ||
|
|
||
| Object[] currentState = new Object[5]; | ||
| Object[] previousState = new Object[5]; | ||
| String[] attributeNames = new String[0]; | ||
|
|
||
| int[] result = persister.resolveDirtyAttributeIndexes( | ||
| currentState, previousState, attributeNames, session | ||
| ); | ||
|
|
||
| assertThat(result).isNotNull(); | ||
| assertThat(result.length).isEqualTo(0); | ||
| }); | ||
| } | ||
|
|
||
| @Test | ||
| public void testResolveDirtyAttributeIndexes_WithAttributes() { | ||
| inTransaction( entityManager -> { | ||
| SessionImplementor session = entityManager.unwrap(SessionImplementor.class); | ||
|
|
||
| Object[] currentState = new Object[] { 1L, "name1", "value1" }; | ||
| Object[] previousState = new Object[] { 1L, "name2", "value2" }; | ||
| String[] attributeNames = {"name", "value"}; | ||
|
|
||
| int[] result = persister.resolveDirtyAttributeIndexes( | ||
| currentState, previousState, attributeNames, session | ||
| ); | ||
|
|
||
| assertThat(result).isNotNull(); | ||
| // Verify no duplicates | ||
| assertThat(result.length).isEqualTo(Arrays.stream(result).distinct().count()); | ||
| // Verify sorted order | ||
| int[] sorted = result.clone(); | ||
| Arrays.sort(sorted); | ||
| assertThat(sorted).isEqualTo(result); | ||
| }); | ||
| } | ||
|
|
||
| @Test | ||
| public void testResolveDirtyAttributeIndexes_DuplicateAttributeNames() { | ||
| inTransaction( entityManager -> { | ||
| SessionImplementor session = entityManager.unwrap(SessionImplementor.class); | ||
|
|
||
| Object[] currentState = new Object[] { 1L, "name1", "value1" }; | ||
| Object[] previousState = new Object[] { 1L, "name2", "value2" }; | ||
| String[] attributeNames = {"name", "name", "value", "value"}; | ||
|
|
||
| int[] result = persister.resolveDirtyAttributeIndexes( | ||
| currentState, previousState, attributeNames, session | ||
| ); | ||
|
|
||
| // Should not contain duplicate indices | ||
| assertThat(result.length).isEqualTo(Arrays.stream(result).distinct().count()); | ||
| }); | ||
| } | ||
|
|
||
| @Test | ||
| public void testResolveDirtyAttributeIndexes_NoDirtyFields() { | ||
| inTransaction( entityManager -> { | ||
| SessionImplementor session = entityManager.unwrap(SessionImplementor.class); | ||
|
|
||
| Object[] state = new Object[] { 1L, "name1", "value1" }; | ||
| String[] attributeNames = {"name"}; | ||
|
|
||
| int[] result = persister.resolveDirtyAttributeIndexes( | ||
| state, state, attributeNames, session | ||
| ); | ||
|
|
||
| assertThat(result).isNotNull(); | ||
| }); | ||
| } | ||
|
|
||
| @Test | ||
| public void testResolveDirtyAttributeIndexes_NonExistentAttribute() { | ||
| inTransaction( entityManager -> { | ||
| SessionImplementor session = entityManager.unwrap(SessionImplementor.class); | ||
|
|
||
| Object[] currentState = new Object[] { 1L, "name1", "value1" }; | ||
| Object[] previousState = new Object[] { 1L, "name2", "value2" }; | ||
| String[] attributeNames = {"nonExistent"}; | ||
|
|
||
| int[] result = persister.resolveDirtyAttributeIndexes( | ||
| currentState, previousState, attributeNames, session | ||
| ); | ||
|
|
||
| assertThat(result).isNotNull(); | ||
| }); | ||
| } | ||
|
|
||
| @Entity | ||
| @Table(name = "TEST_ENTITY") | ||
| public static class TestEntity { | ||
| @Id | ||
| @GeneratedValue | ||
| private Long id; | ||
|
|
||
| private String name; | ||
|
|
||
| private String value; | ||
|
|
||
| public Long getId() { | ||
| return id; | ||
| } | ||
|
|
||
| public void setId(Long id) { | ||
| this.id = id; | ||
| } | ||
|
|
||
| public String getName() { | ||
| return name; | ||
| } | ||
|
|
||
| public void setName(String name) { | ||
| this.name = name; | ||
| } | ||
|
|
||
| public String getValue() { | ||
| return value; | ||
| } | ||
|
|
||
| public void setValue(String value) { | ||
| this.value = value; | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to check
!fields.get( position ), as setting an already-set bit is idempotent.