Skip to content

Stream XLSX/XML interpreter rows to reduce memory usage - #59

Open
Jonathon-Meney-Torq wants to merge 2 commits into
masterfrom
feat/streaming-interpreters
Open

Jonathon-Meney-Torq wants to merge 2 commits into
masterfrom
feat/streaming-interpreters

Conversation

@Jonathon-Meney-Torq

@Jonathon-Meney-Torq Jonathon-Meney-Torq commented Aug 28, 2026 •

Copy link
Copy Markdown
Contributor

Large import files could exhaust PHP memory during queue preparation:

  • SpoutXlsxDataLoader now yields rows (getRows returns iterable); advanced/bulk XLSX interpreters consume the stream with in-loop header handling
  • CustomXlsxFileInterpreter reads the workbook in 1000-row chunks instead of toArray()
  • CustomXmlFileInterpreter streams records via XMLReader for simple element paths (full-DOM fallback for complex XPath), XSD validated incrementally
  • ExpressionLanguage instantiated once instead of per row

Pairs with pimcore/data-importer#686

@Jonathon-Meney-Torq Jonathon-Meney-Torq self-assigned this Aug 28, 2026
@Jonathon-Meney-Torq Jonathon-Meney-Torq added bug Something isn't working enhancement New feature or request labels Aug 28, 2026

@rjjackson22 rjjackson22 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I assume this will have to change alongside pimcore/data-importer#686 as that gets reviewed, but I've identified a few things worth investigating. Looking forward to having this merged though, should be a big improvement to performance and usability.

Comment on lines +31 to +33
exclude:
- "../../DataSource/Interpreter/PreviewRowsReadFilter.php"
- "../../DataSource/Interpreter/ChunkedRowsReadFilter.php"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I believe this could actually be completely removed by using the #[Exclude] attribute on the class instead. They if it's moved around for whatever reason the exclude goes with it.

use TorqIT\DataImporterExtensionsBundle\DataSource\Interpreter\PreviewRowsReadFilter;

// Copy of Pimcore\Bundle\DataImporterBundle\DataSource\Interpreter\XlsxFileInterpreter
class CustomXlsxFileInterpreter extends AbstractInterpreter

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The original goal of this file (and CustomXmlFileInterpreter.php) was to make an exact copy of Pimcore's base class because in 2026.x they prepended all of their internal services with final so we couldn't inherit from them. With exact copies it should be easier to compare with Pimcore's originals, if they happen to change them.

That being said, the only function not overridden from Pimcore's original now is fileValid() which is 2 lines. I don't think it's worth keeping a copy of the original if we're not inheriting from them. I think we should do the following:

  • Combine CustomXlsxFileInterpreter with XlsxFileInterpreterWithColumnNames so we have a single parent xlsx interpreter with our general logic/improvements. Pick a new name for it, probably something better than ChunkedXlsxFileInterpreterWithColumnNames but whatever works.
  • Rename CustomXmlFileInterpreter to something more descriptive (ChunkedCustomXmlFileInterpreter?)
  • Move both parent interpreters out of the "/Overrides" folder and remove any indications they're "copies" of Pimcore's original.

Would like input from @lukemacausland and/or @IronSean on this as well, if possible.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm just now seeing your PR to Pimcore/data-importer, but comparing the changes there with this file and it doesn't look identical, so I think my point(s) still stand, but worth interrogating further.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants