-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Direct relations list fix #946
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #946 +/- ##
==========================================
+ Coverage 80.85% 81.12% +0.26%
==========================================
Files 46 46
Lines 3087 3046 -41
Branches 120 117 -3
==========================================
- Hits 2496 2471 -25
+ Misses 591 575 -16
|
This reverts commit 697f470.
This reverts commit 732a814.
Haven't read the bulk of dms change part, but so far changes are dms-only, old tests pass and one new is added so looks good so far 🙂 but will try to read through the main part too |
most of it is just cleanup/code deduplication, the actual changes needed for this were pretty small, but I had some trouble understanding the existing code correctly! |
could you point to a summary or place in diff? Looking at commits in the PR there is some separation of changes but a bit too many to easily split into cosmetic&refactor vs the fix |
Will do in the PR header. Yeah sorry about the number of commits, lots of them are just scalafmt and stuff like that as well ^^ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some small notes for the non-fix part
src/main/scala/cognite/spark/v1/FlexibleDataModelRelationUtils.scala
Outdated
Show resolved
Hide resolved
src/main/scala/cognite/spark/v1/FlexibleDataModelRelationUtils.scala
Outdated
Show resolved
Hide resolved
src/main/scala/cognite/spark/v1/FlexibleDataModelRelationUtils.scala
Outdated
Show resolved
Hide resolved
src/main/scala/cognite/spark/v1/FlexibleDataModelRelationUtils.scala
Outdated
Show resolved
Hide resolved
src/main/scala/cognite/spark/v1/FlexibleDataModelRelationUtils.scala
Outdated
Show resolved
Hide resolved
….scala Co-authored-by: Dmitry Ivankov <dmitry.ivankov@cognite.com>
….scala Co-authored-by: Dmitry Ivankov <dmitry.ivankov@cognite.com>
….scala Co-authored-by: Dmitry Ivankov <dmitry.ivankov@cognite.com>
Changes:
Cosmetic: creating a method "getListPropAsSeq" to handle parsing of a row into a list The method is generic and can parse into any type, but I decided to use it to replace the code that was there in place, so I parse into whatever it was first parsed into before. This was done because this piece of code was repeated for each possible list property.
Cosmetic: dealing with number parsing in a generic way. There were hundreds of lines of code dedicated to parsing numbers and the way they all worked was to parse like this: => any => string => bigint => specific type. I decided to constrain my change to just code deduplication for now, so I created a second order function that takes the final transformation bigint => specific type and does everything.
Cosmetic: I deleted the
if p.isList
in all cases of the list handling, and the opposite in the non-list handling because we already test this before entering the method.Cosmetic/Functional: I moved the parsing of DirectRelations from happening before the flow separation into lists to happening inside it. So now non-list direct relations are handled in the "toInstancePropertyValueOfNonList" whereas before they were a separate case
Functional: I added handling of DirectRelations in
toInstancePropertyValueOfList
For that I reworkedextractDirectRelationReferenceFromStruct
and I added the methodtryAsDirectNodeRelationList
In the end that's not a lot of functional changes, but it was a bit messy to figure out how it works and how the tests should actually work!