Skip to content
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

2136-missingIsPartOf #2137

Merged
merged 6 commits into from
Jan 24, 2025
Merged

2136-missingIsPartOf #2137

merged 6 commits into from
Jan 24, 2025

Conversation

TobiasNx
Copy link
Contributor

@TobiasNx TobiasNx commented Jan 23, 2025

See #2136

This PR does the following:

  • Adds a new test
  • Makes the isPartOf schema more rigid by introducing a required statement.
  • Moves the isPartOf.type to the end of each object. (Prerequisite)
  • Deletes the unnecessary fallback for label.
  • Adjusts the mapping for 830 and isPartOf so that only 830 is mapped if $a and $w exist. Otherwise it would be skipped.

Comment on lines 83 to 86
"isPartOf" : [ {
"type" : [ "IsPartOfRelation" ],
"numbering" : "11"
} ],
Copy link
Contributor

@acka47 acka47 Jan 23, 2025

Choose a reason for hiding this comment

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

I am not sure I understand this. The file is invalid against the updated schema. Is this somehow taken into account in any tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I rearranged this PR. It was not ready for review yet.

@acka47 acka47 self-requested a review January 23, 2025 11:57
@TobiasNx TobiasNx requested a review from dr0i January 23, 2025 12:10
Copy link
Contributor

@acka47 acka47 left a comment

Choose a reason for hiding this comment

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

+1

Copy link
Member

@dr0i dr0i left a comment

Choose a reason for hiding this comment

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

I don't understand: why should the order of the fields matter? Order cannot be guaranteed .

@TobiasNx
Copy link
Contributor Author

I don't understand: why should the order of the fields matter? Order cannot be guaranteed .

Sure it matters with regard to the fix transformation in our case. As well as the elastic search for reviewing since the order is also kept by elastic search see: #2000

In our case here: Moving the adding of the type-property down behind mapping of the hasSuperordinate property enables us to easily use a conditional to test if the first object in hasSuperordinate is empty before adding any type or numbering information.

@dr0i
Copy link
Member

dr0i commented Jan 23, 2025

As well as the elastic search for reviewing since the order is also kept by elastic search see: #2000

can you quote where "the order is also kept by elastic search"? I couldn't find it in the issue you've linked to (#2000)

You described why this is useful for the fix transformation, but I did not comprehend that: isn't it the case that in fix everything is already "there" (as a record) in contrast to morphwhre we have stream evenst, i.e. linearity does matter?

I ACK that this is a presentational thing (humans want to read the most important things at the top (like described in #2000)).

@TobiasNx
Copy link
Contributor Author

TobiasNx commented Jan 23, 2025

As well as the elastic search for reviewing since the order is also kept by elastic search see: #2000

can you quote where "the order is also kept by elastic search"? I couldn't find it in the issue you've linked to (#2000)

I am speaking from experience in context of #2000

Also see e.g. the info explicit in the documentation of ES with regards to arrays:
https://www.elastic.co/guide/en/elasticsearch/guide/current/complex-core-fields.html#_multivalue_fields

You described why this is useful for the fix transformation, but I did not comprehend that: isn't it the case that in fix everything is already "there" (as a record) in contrast to morphwhre we have stream evenst, i.e. linearity does matter?

For fix the aspect of order is totally different to the stream order of morph where order in the incoming record has relevance as well as the order of the morph functions.
But while in fix the incoming record is fully available for a transformation. The transformation in fix still is linear and has a chronological order. The transformations/fixes at the top of a script come first. And when the fix adds a new fields and create a new structures, that was not there, the order of the fix functions as well the order of the newly created elements can become relevant for the transformation e.g. for conditionals or transformations that should manipulate/use newly created fields.

e.g.
Example a:

fix(a)
fix(b)
add_field(new, field)
add_field(anotherNew,field)
if exists (new)
	fix(c)
end

Example b:

fix(a)
fix(b)
add_field(anotherNew,field)
if exists (new)
	fix(c)
end
add_field(new,field)

Only in example a the conditional if exists would operate therefore the order can be relevant.

In our case changing the order of the transformation which results in changing the order of the output (e6354b6) is the prerequisite of the introduction of the conditionals that decide if an isPartOf-Object is filled with fields or deleted later on by the vacuum because it is empty(e6354b6).

I ACK that this is a presentational thing (humans want to read the most important things at the top (like described in #2000)).

Technically you are right.

@TobiasNx TobiasNx requested a review from dr0i January 23, 2025 16:15
@dr0i
Copy link
Member

dr0i commented Jan 24, 2025

Ok, now I've got it, thx.

@dr0i dr0i merged commit 22e9ef6 into master Jan 24, 2025
1 check passed
@dr0i dr0i deleted the 2136-missingIsPartOf branch January 24, 2025 07:45
@dr0i
Copy link
Member

dr0i commented Jan 24, 2025

Will be deployed Monday.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants