perf(dataset): Reduce FlatXmlProducer memory footprint - #915
Merged
Merged
Conversation
Reviewer's GuideAdds a per-parse column-name String cache to FlatXmlProducer to reduce memory usage by interning column names used in Column construction, verifies instance reuse with a new test, and documents the change in the project changelog. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 44 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughFlatXmlProducer now caches repeated column-name ChangesFlatXmlProducer column-name caching
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider parameterizing
_columnNameCacheasMap<String, String>(and using the genericHashMap<String, String>) to improve type safety and avoid unchecked casts ininternColumnName. - If
FlatXmlProducerinstances are long-lived, you may want to clear or null out_columnNameCacheat the end ofproduce()to avoid retaining the per-parse cache and its entries longer than necessary.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider parameterizing `_columnNameCache` as `Map<String, String>` (and using the generic `HashMap<String, String>`) to improve type safety and avoid unchecked casts in `internColumnName`.
- If `FlatXmlProducer` instances are long-lived, you may want to clear or null out `_columnNameCache` at the end of `produce()` to avoid retaining the per-parse cache and its entries longer than necessary.
## Individual Comments
### Comment 1
<location path="src/test/java/org/dbunit/dataset/xml/FlatXmlProducerTest.java" line_range="314" />
<code_context>
}
+ @Test
+ void testProduce_sameColumnNameAcrossTables_reusesColumnNameStringInstance() throws Exception
+ {
+ // Two distinct tables sharing a column name, and no DTD/metaDataSet, so both
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test that exercises column-name interning via the column-sensing path (handleMissingColumns).
The current test only exercises interning via `createTableMetaData`, where metadata comes from the first row’s attributes. Since `internColumnName()` is now also used in `handleMissingColumns` for column sensing (later rows introducing new columns), please add a test where:
- The first row omits a column that a later row adds, ensuring `handleMissingColumns` runs.
- The same column name appears in another table.
- The `Column` instances created via column sensing are verified to reuse the same `String` instance as the corresponding columns in the first table.
This will ensure the interning behavior is covered for both construction paths and guard against regressions in the column-sensing logic.
Suggested implementation:
```java
import org.dbunit.dataset.datatype.DataType;
import org.dbunit.dataset.stream.AbstractProducerTest;
import org.dbunit.dataset.stream.DefaultConsumer;
import org.dbunit.dataset.stream.IDataSetProducer;
import org.dbunit.dataset.stream.MockDataSetConsumer;
import org.dbunit.testutil.TestUtils;
import org.dbunit.dataset.Column;
import org.dbunit.dataset.ITableMetaData;
import org.dbunit.dataset.xml.FlatXmlProducer;
import org.xml.sax.InputSource;
import java.io.StringReader;
import java.util.ArrayList;
import java.util.List;
import static org.junit.jupiter.api.Assertions.assertSame;
```
To implement the requested test that exercises column-name interning via the column-sensing path (`handleMissingColumns`), you’ll need to add a new `@Test` method to `FlatXmlProducerTest` (near the existing `testProduce_sameColumnNameAcrossTables_reusesColumnNameStringInstance`), along these lines:
```java
@Test
void testProduce_columnSensing_reusesColumnNameStringInstanceAcrossTables() throws Exception
{
// TABLE_B: column metadata comes from the first row's attributes (createTableMetaData path).
// TABLE_A: first row omits COL_SHARED; second row introduces COL_SHARED so handleMissingColumns runs.
// Both tables share the same column name COL_SHARED, and we verify that the Column instances
// reuse the same String instance for the column name across tables.
final String content = "<?xml version=\"1.0\"?>"
+ "<dataset>"
+ "<TABLE_B COL_SHARED=\"b0\"/>"
+ "<TABLE_A COL0=\"a0\"/>"
+ "<TABLE_A COL0=\"a1\" COL_SHARED=\"a1\"/>"
+ "</dataset>";
final InputSource source = new InputSource(new StringReader(content));
final IDataSetProducer producer = new FlatXmlProducer(source);
final List<ITableMetaData> capturedMetaData = new ArrayList<>();
producer.setConsumer(new DefaultConsumer()
{
@Override
public void startTable(ITableMetaData metaData)
{
capturedMetaData.add(metaData);
super.startTable(metaData);
}
});
producer.produce();
ITableMetaData tableA = null;
ITableMetaData tableB = null;
for (ITableMetaData metaData : capturedMetaData)
{
if ("TABLE_A".equals(metaData.getTableName()))
{
tableA = metaData;
}
else if ("TABLE_B".equals(metaData.getTableName()))
{
tableB = metaData;
}
}
// Basic sanity checks to ensure tables were captured
org.junit.jupiter.api.Assertions.assertNotNull(tableA, "TABLE_A metadata should be captured");
org.junit.jupiter.api.Assertions.assertNotNull(tableB, "TABLE_B metadata should be captured");
Column colSharedA = tableA.getColumn("COL_SHARED");
Column colSharedB = tableB.getColumn("COL_SHARED");
org.junit.jupiter.api.Assertions.assertNotNull(colSharedA, "TABLE_A should have sensed COL_SHARED");
org.junit.jupiter.api.Assertions.assertNotNull(colSharedB, "TABLE_B should have COL_SHARED from first row");
// The Column instances created via column sensing (TABLE_A) must reuse the same String instance
// as the corresponding columns in TABLE_B (created via createTableMetaData).
assertSame(colSharedB.getColumnName(), colSharedA.getColumnName(),
"Column name String instances should be interned and reused across tables");
}
```
You should place this method inside the `FlatXmlProducerTest` class, alongside the other `@Test` methods. If the existing `testProduce_sameColumnNameAcrossTables_reusesColumnNameStringInstance` already captures metadata in a slightly different way (e.g., using a shared `DefaultConsumer` or helper), you can adapt the above to use the same pattern, as long as:
1. The first row of `TABLE_A` omits `COL_SHARED` and a later row adds it (triggering `handleMissingColumns`).
2. `TABLE_B` has `COL_SHARED` present in its first row (metadata from `createTableMetaData`).
3. You assert `assertSame` on the `String` instances returned by `getColumnName()` for `COL_SHARED` in both tables.
This will ensure column-name interning is covered for both construction paths and guard against regressions in the column-sensing logic.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/test/java/org/dbunit/dataset/xml/FlatXmlProducerTest.java (1)
313-347: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the column-sensing cache path.
This test verifies reuse through
createTableMetaData. The changedhandleMissingColumnspath at Lines 379-380 is exercised by the existing column-sensing test, but no test verifies that a sensed column name is reused.Add a case that senses
COL1in one table, then creates metadata forCOL1in another table, and assertisSameAson both column names.As per coding guidelines, changes under
src/test/**must be covered by unit tests and tests must be added or updated as needed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/org/dbunit/dataset/xml/FlatXmlProducerTest.java` around lines 313 - 347, Extend the FlatXmlProducer tests with a column-sensing scenario that first discovers COL1 in one table, then creates metadata for COL1 in another table through the handleMissingColumns path. Capture both ITableMetaData results and assert their COL1 column-name references with isSameAs, covering reuse in the sensed-column cache rather than only createTableMetaData.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/org/dbunit/dataset/xml/FlatXmlProducer.java`:
- Around line 379-380: Update the attribute-processing logic around
internColumnName and the _columnSensing check so ignored columns use
attributes.getQName(i) directly when sensing is disabled, avoiding
_columnNameCache growth. Only call internColumnName when _columnSensing is
enabled and the column will be retained.
In `@src/test/java/org/dbunit/dataset/xml/FlatXmlProducerTest.java`:
- Around line 327-331: Add JavaDoc to the overridden public startTable method in
the anonymous DefaultConsumer subclass, including a topic sentence plus complete
descriptions for the metaData parameter and DataSetException. Keep the existing
callback behavior unchanged.
---
Nitpick comments:
In `@src/test/java/org/dbunit/dataset/xml/FlatXmlProducerTest.java`:
- Around line 313-347: Extend the FlatXmlProducer tests with a column-sensing
scenario that first discovers COL1 in one table, then creates metadata for COL1
in another table through the handleMissingColumns path. Capture both
ITableMetaData results and assert their COL1 column-name references with
isSameAs, covering reuse in the sensed-column cache rather than only
createTableMetaData.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 28d32d5f-6d46-4028-a8ba-f95022520446
📒 Files selected for processing (3)
src/changes/changes.xmlsrc/main/java/org/dbunit/dataset/xml/FlatXmlProducer.javasrc/test/java/org/dbunit/dataset/xml/FlatXmlProducerTest.java
…and tables FlatXmlProducer built a new Column from each SAX attribute name it saw, even though column names commonly repeat across many rows of a table and across different tables in the same document. Add a per-parse String cache (created at the start of each produce() call, cleared at its end) and route both Column construction sites through it: the initial metadata built from the first row's attributes, and columns discovered later via column sensing when they will actually be retained. Columns that would just be logged as an ignored/extra-column warning (column sensing off) skip the cache instead of growing it for nothing. Attribute values are left untouched since they are far less likely to repeat. Refs: 512
Member
Author
|
Addressed this round of feedback, all amended into the single commit (now
Full unit suite green (2025 tests, 0 failures/errors). Not pushed yet - will push once after this round is fully processed. |
jeffjensen
force-pushed
the
512-flatxmlproducer-column-name-cache
branch
from
August 7, 2026 16:01
f72437a to
3bbdbe4
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
FlatXmlProducer(_columnNameCache, reset at the start of eachproduce()call) so a column name repeated across rows and across different tables shares oneStringinstance instead of eachColumnretaining its own duplicate copy.Column-construction sites via a newinternColumnName()helper:createTableMetaData()(metadata built from the first row's attributes) andhandleMissingColumns()(columns discovered later via column sensing).Test plan
./mvnw clean test -Dtest=FlatXmlProducerTest— 12 tests, 0 failures, including newtestProduce_sameColumnNameAcrossTables_reusesColumnNameStringInstance(AssertJisSameAs, proving actual String-instance reuse rather than just value-equality)./mvnw clean test— full unit suite, 2024 tests, 0 failures/errorsFixes #512
Summary by Sourcery
Introduce per-parse column-name interning in FlatXmlProducer to reduce memory usage while preserving existing behavior.
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
Performance
Bug Fixes