Expands InternalExportDataProvider per dataverse-spi 2.1.0 and refactors the DDI exporter to utilize the new functionality - #11799
Expands InternalExportDataProvider per dataverse-spi 2.1.0 and refactors the DDI exporter to utilize the new functionality#11799landreev wants to merge 57 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
|
Note that the PR now builds under Jenkins (with a snapshot release of dataverse-spi that is now published). |
This comment has been minimized.
This comment has been minimized.
| DataFile dataFile = fileMetadata.getDataFile(); | ||
| jab.add(JsonPrinter.json(dataFile, fileMetadata, true)); | ||
| if (fileAndDataDetails == null) { | ||
| JsonArrayBuilder jab = Json.createArrayBuilder(); |
There was a problem hiding this comment.
When I run this...
mvn test -Dtest=TabularDataExportIT#exportTabularMetadata
... I get these errors:
[INFO] Reading API signatures: /Users/pdurbin/github/iqss/dataverse/src/maven/forbiddenapis.txt
[INFO] Loading classes to check...
[INFO] Scanning classes for violations...
[ERROR] Forbidden method invocation: jakarta.json.Json#createArrayBuilder() [Use edu.harvard.iq.dataverse.util.json.JsonUtil.createArrayBuilder(...) instead]
[ERROR] in edu.harvard.iq.dataverse.export.InternalExportDataProvider (InternalExportDataProvider.java:148)
[ERROR] Forbidden method invocation: jakarta.json.Json#createArrayBuilder() [Use edu.harvard.iq.dataverse.util.json.JsonUtil.createArrayBuilder(...) instead]
[ERROR] in edu.harvard.iq.dataverse.export.InternalExportDataProvider (InternalExportDataProvider.java:179)
[ERROR] Scanned 1225 class file(s) for forbidden API invocations (in 0.77s), 2 error(s).
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
| assertTrue(false, "Unexpected XMLStreamException when attempting to parse the dataDscr section: " + xse.getMessage()); | ||
| } finally { | ||
| if (xmlr != null) { | ||
| try {xmlr.close();} catch (XMLStreamException e) {} |
There was a problem hiding this comment.
🚫 [reviewdog] <com.puppycrawl.tools.checkstyle.checks.blocks.LeftCurlyCheck> reported by reviewdog 🐶
'{' at column 21 should have line break after.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Philip Durbin <philip_durbin@harvard.edu>
Co-authored-by: Philip Durbin <philip_durbin@harvard.edu>
This comment has been minimized.
This comment has been minimized.
2 similar comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| Dataverse metadata export code has been re-factored to use the new and improved Data Export framework (note also that the relevant interface and the accompanying code have been moved out of the main Dataverse repository and into the dedicated [GDCC project](https://github.com/gdcc/dataverse-spi)). In this release this will result in a measurable improvement in exporting the DDI format. But it will be possible to take advantage of this refactoring in the exports of other data-rich formats in future releases. | ||
| An optional query parameter ``formats`` has been added to the ``reExportAll`` and ``reExportDataset``, allowing an administrator to force re-export of only the formats specified. |
There was a problem hiding this comment.
We can wordsmith this at release time but a few thoughts.
I'd probably pitch this as two related features:
## Highlights
### Export improvements
- Faster DDI export
- Ability to export only selected formats
Then, further down:
## API updates
### Ability to export only selected formats
curl example, link to API Guide.
Still further down:
## Updates for developers
We've refactored, blah blah blah.
There was a problem hiding this comment.
Sure, makes sense. Will do.
|
|
📦 Pushed preview images as 🚢 See on GHCR. Use by referencing with full name as printed above, mind the registry name. |


Un-drafting the PR now!
What this PR does / why we need it:
See issue #11405. Short version: the current export data framework is prohibitively expensive for datasets with very large amounts of tabular data (i.e., "ingested" DataFiles that have associated DataTable entities , with large numbers of child DataVariable objects). This PR improves it, via adding code that retrieves the data in manageable chunks. This in turn relies on the new version of the
ExportDataProviderinterface from https://github.com/gdcc/dataverse-spi.Which issue(s) this PR closes:
Special notes for your reviewer:
Note that the PR builds with a snapshot release of dataverse-spi:
I.e., the version 2.1.0 still needs to be properly released under gdcc. But I don't think that should be stopping putting the PR into review and merging it rather sooner than later.
Note that this PR also removes
modules/dataverse-spi, and the action that used to build snapshot releases of it (.github/workflows/spi_release.yml) from the main source tree; now that dataverse-spi lives under gdcc.Note specifically the test for the functionality,
TabularDataExportITadded in this PR.I have added it to the list of integration tests that are performed automatically by the workflow action (ineven though, strictly speaking, it is not a real integration test. [update: No, it is not executed as an "integration test". It is run by the GitHub action via this change in the main pom.xml] I.e., it does not rely on calling a fully-functional Dataverse instance started remotely. It relies instead on the new test framework added in @poikilotherm's #12365 where a local Docker instance of the database is spun up in which some dummy objects - Datasets, Versions, Datatables and Variables etc are created instantly, allowing testing of complex functionality that requires real database access. Without this new framework, the only way to test an export of a dataset with N large tabular files would be to actually ingest these N files via the API in the remote Dataverse instance used by the RestAssured tests. Which would add to the ongoing bloating of the test suite, and still would not allow to test the underlying functionality as thoroughly (for example, there is no easy way to count the actual database queries under such scenario).tests/integration-tests.txt)You can run the test in question in your own dev. environment via
mvn test -Dtest=TabularDataExportIT#exportTabularMetadataIf you do not have Docker running, the test will be politely skipped. Otherwise it will create a throwaway postgres instance and proceed to test tabular data exports and associated data retrieval, comparing the results produced using the legacy methods vs. the new and optimized versions added in this PR and more.
Suggestions on how to test this:
Please see the info under the "notes for the reviewer" and feel free to play and experiment with the new test described there. Please keep in mind that the test is still somewhat of a reference demonstrator of the new
JpaEntityManagerService-based framework from #12365. It can be expanded and improved going forward.The ultimate test would be to test before vs. after on some real-life monster datasets.
I would suggest these bad boys on qa:
https://qa.dataverse.org/dataset.xhtml?persistentId=doi:10.7910/DVN/KZW8R2 (207 ingested tab. files; 186906 variables total)
https://qa.dataverse.org/dataset.xhtml?persistentId=doi:10.7910/DVN/S5BRBO (134 tab. files; 536006 variables).
"ddi" (aka the "full" ddi) is the export format that needs to be tested.
Note that some extra steps will be required in order to test this on qa:
export_ddi.cachedwill need to be erased between testing runs.Happy to assist with the above if needed.
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Is there a release notes update needed for this change?:
Additional documentation: