Repository navigation
Stream XLSX/XML interpreter rows to reduce memory usage - #59
Jonathon-Meney-Torq wants to merge 2 commits into
Conversation
rjjackson22
left a comment
There was a problem hiding this comment.
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.
| exclude: | ||
| - "../../DataSource/Interpreter/PreviewRowsReadFilter.php" | ||
| - "../../DataSource/Interpreter/ChunkedRowsReadFilter.php" |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
CustomXlsxFileInterpreterwithXlsxFileInterpreterWithColumnNamesso we have a single parent xlsx interpreter with our general logic/improvements. Pick a new name for it, probably something better thanChunkedXlsxFileInterpreterWithColumnNamesbut whatever works. - Rename
CustomXmlFileInterpreterto 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.
There was a problem hiding this comment.
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.
Large import files could exhaust PHP memory during queue preparation:
Pairs with pimcore/data-importer#686