lupl/mapper refactor #194
Merged
lupl/mapper refactor #194
Conversation
This file contains 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
lu-pl
force-pushed
the
lupl/mapper-refactor
branch
2 times, most recently
from
65ce2de to
aaaba46
Compare
lu-pl
force-pushed
the
lupl/mapper-refactor
branch
from
aaaba46 to
576b0cf
Compare
lu-pl
force-pushed
the
lupl/mapper-refactor
branch
6 times, most recently
from
57b246e to
bc91244
Compare
This change introduces a refactor of the important ModelBindingsMapper class using a pandas DataFrame for effecting grouping and aggregation and a CurryModel utility for partially instantiating a given Pydantic model with checks running for every partial application, allowing for fast validation failure. Closes #170. Aggregation behavior is now only triggered for actually aggregated fields, and not for top-level models as well. Therefore this also closes #181.
Some model sanity checking was implemented in the old ModelBindingsMapper, all checking should be done in a dedicated model sanity checking pipeline, see issue #108. So these tests - for now - are xfails.
The expected data was actually wrong and passed a buggy behavior in the old ModelBindingsMapper. The change fixes the test to expect the correct result. Note that this test case is somewhat contrived, because the binding data is unordered and actual data from an RDFProxy-modified query for grouped models would be ordered. The refactored mapper uses a pd.DataFrame for grouping, so (unlike a possible low-level solution with itertools.groupby) the above case can be handled, because dataframes implement efficient grouping also across unordered row series. This is actually a quite powerful feature and makes the ModelBingingsMapper class generally useful, not just in the context of RDFProxy.
As indicated in the docstring for ModelBindingsMapper, the class is somewhat coupled to SPARQLModelAdapter, because ModelBindinsMapper does not run model sanity by itself - sanity checking should happen in SPARQLModelAdapter, i.e. as early as possible. See issue #108. Once sanity checking is implemented, RDFProxy will make a public ModelBindingsMapper class available which will run model sanity checking itself.
This test is related to a fixed bug in _ModelBindingsMapper._instantiate ungrouped_model_from_row, where default values were consulted on falsy field values, i.e. also on empty strings and None field values.
lu-pl
force-pushed
the
lupl/mapper-refactor
branch
from
bc91244 to
3b338b0
Compare
kevinstadler
approved these changes
Jan 22, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Closes #170 , closes #181 .