Skip to content
This repository has been archived by the owner on Jan 27, 2025. It is now read-only.

Array handling with and without set_array #80

Closed
TobiasNx opened this issue Nov 30, 2021 · 14 comments · Fixed by #87
Closed

Array handling with and without set_array #80

TobiasNx opened this issue Nov 30, 2021 · 14 comments · Fixed by #87
Assignees
Labels
Bug Something isn't working

Comments

@TobiasNx
Copy link
Collaborator

TobiasNx commented Nov 30, 2021

It seems that fix can only create arrays of objects, if with set_array the target array is created or it already exists.

If one only creates an array by copy_field("sourc_field", "target.$append.field")

The example are taken from but look at the playground to explore:
https://gitlab.com/oersi/oersi-etl/-/commit/3c3bb1cdb907b631c9848f245d8ca71aa45c17b2

connected to: #78 and #65

Without set_array it seems to handle newly created arrays as list of values but not of objects.

_______with set_array See example also in the playground

#  ------------creator------------------------------------
set_array("creator[]")
do list("path":"metadata.mods:mods.mods:name","var":"name")
    if any_match("name.mods:role.mods:roleTerm.*.value","aut|cre")
        copy_field("name.mods:displayForm.value","creator[].$append.name")
        /* This seems not to work */
        if any_equal("name.type","personal") /* TODO: looks at all names, not just current name */
            add_field("creator[].$last.type", "Person")
        end
        if any_equal("name.type","corporate")
            add_field("creator[].$last.type", "Organization")
        end
    end
end

=>

  "creator" : [ {
    "name" : "Armbruster, André",
    "type" : "Person"
  }, {
    "name" : "Seyfert, Robert",
    "type" : "Person"
  } ]

_________without set_array See example here in Playground.

`/* TODO: learningResourceType */
# TODO: Needs lookup but array behaviour some not to work here.
do list("path":"metadata.mods:mods.mods:genre","var":"lRT")
    if any_match("lRT.authorityURI","https://duepublico.uni-due.de/api/v1/classifications/mir_genres") /* TODO: looks at all, not just current */
        copy_field("lRT.valueURI","learningResourceType[].$append.id") #???
        copy_field("lRT.valueURI","learningResourceType[].$last.prefLabel.de") /* TODO: we only get the very last, not every last one */
    end
end

=>

  "learningResourceType" : [ "https://duepublico.uni-due.de/api/v1/classifications/mir_genres#audio", {
    "de" : "https://duepublico.uni-due.de/api/v1/classifications/mir_genres#audio"
  } ],
@TobiasNx TobiasNx added the Bug Something isn't working label Nov 30, 2021
@TobiasNx

This comment has been minimized.

@TobiasNx
Copy link
Collaborator Author

TobiasNx commented Nov 30, 2021

It seems that the "workaround" nor the direct way with create_field is working with arrays of strings

See Playground with set_array

#  ------------inLanguage------------------------------------
set_array("inLanguage[]")
copy_field("metadata.mods:mods.mods:language.mods:languageTerm.value","inLanguage[].$append")

breaks the process.

Also breaks withou set_array:

See Playground without set_array

it only works if you use copy_field and add a subfield. .See example here:

copy_field("metadata.mods:mods.mods:language.mods:languageTerm.value","inLanguage[].$append.label")

This hints perhaps an error for another ticket: you only can refrence/target subfields of objects in an array but you cannot do that with an string in an list of strings.

@TobiasNx
Copy link
Collaborator Author

TobiasNx commented Dec 1, 2021

I added playground examples to better illustrate the errors.

@TobiasNx
Copy link
Collaborator Author

TobiasNx commented Dec 1, 2021

An workaround with set_array is only working if there at least on instance of the subfield is expected otherwise an emtpy array will appear. vacuum() does not recognize empty arrays this does not help to:
See here

@fsteeg
Copy link
Member

fsteeg commented Dec 7, 2021

It seems that the "workaround" nor the direct way with create_field is working with arrays of strings

The samples from #80 (comment) seem to be fixed with #82, both look good: w/ set_array, w/o set_array.

An workaround with set_array is only working if there at least on instance of the subfield is expected otherwise an emtpy array will appear.

Tweaked your sample: remove if-conditional and add 'mods' namespace to 'displayForm': looks good.

(In general issue examples should point to the production playground, not test, so we can easily compare them in review).

We're certainly not done with the set_array behavior, but since we're leaving #65 open for inconsistencies in array handling, and since for practical purposes, set_array now seems to work, maybe we can close this issue here?

@TobiasNx
Copy link
Collaborator Author

TobiasNx commented Dec 7, 2021

Great! +1

An workaround with set_array is only working if there at least on instance of the subfield is expected otherwise an emtpy array will appear.

Tweaked your sample: remove if-conditional and add 'mods' namespace to 'displayForm': looks good.

(In general issue examples should point to the production playground, not test, so we can easily compare them in review).

I think the error was intentiona or at least the result was right :Dl. To show that vacuum() does not help with empty arrays and this is a problem for the . This should be a new ticket.

We're certainly not done with the set_array behavior, but since we're leaving #65 open for inconsistencies in array handling, and since for practical purposes, set_array now seems to work, maybe we can close this issue here?
+1

@TobiasNx
Copy link
Collaborator Author

TobiasNx commented Dec 7, 2021

wait i spottet the following for working without set_array:

It does not work

@TobiasNx TobiasNx assigned fsteeg and unassigned TobiasNx Dec 7, 2021
@fsteeg
Copy link
Member

fsteeg commented Dec 9, 2021

wait i spottet the following for working without set_array

Fixed with related changes from #82 / #87: issue on prod, fixed on test.

@fsteeg fsteeg assigned TobiasNx and unassigned fsteeg Dec 9, 2021
@fsteeg fsteeg linked a pull request Dec 9, 2021 that will close this issue
fsteeg added a commit that referenced this issue Dec 9, 2021
@blackwinter
Copy link
Member

@TobiasNx: Can you assign me when #87 is ready to be merged?

@TobiasNx
Copy link
Collaborator Author

I wait for fix of #87 to review this again.

@TobiasNx TobiasNx assigned fsteeg and unassigned TobiasNx Dec 13, 2021
@fsteeg
Copy link
Member

fsteeg commented Dec 14, 2021

I don't understand, still looks good to me, as linked in #80 (comment):

issue on prod

{
  "contributor" : [ {
    "1" : "Armbruster, André",
    "2" : "Seyfert, Robert"
  } ]
}
{
  "contributor" : [ "Oeste, Bettina" ]
}

fixed on test

{
  "contributor" : [ {
    "name" : "Armbruster, André"
  }, {
    "name" : "Seyfert, Robert"
  } ]
}
{
  "contributor" : [ {
    "name" : "Oeste, Bettina"
  } ]
}

@fsteeg fsteeg assigned TobiasNx and unassigned fsteeg Dec 14, 2021
@TobiasNx
Copy link
Collaborator Author

I set this on hold, because of the combined PR for this and the other ticket #82.

Adding and copying sub-fields seem to work. What seems not to work is copying and moving objects into an array:
https://github.com/oersi/oersi-etl/blob/2a05f299ebb6346cf59b4d1ad7b8930531a9a834/data/experimental/appendArrays/test_append_object.flux

See here in the Playground too

This might be a separate ticket though.

@TobiasNx TobiasNx assigned fsteeg and unassigned TobiasNx Dec 15, 2021
@fsteeg
Copy link
Member

fsteeg commented Dec 15, 2021

This might be a separate ticket though.

Right, what's missing here is $append support for objects in move_field and copy_field. We can still discuss if we need this now in #87, or later. But this issue here seems no longer related at this point.

@TobiasNx
Copy link
Collaborator Author

PS: From the original tests it seems that $lastis not working anymore. Also should be dealt with in another ticket.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants