say which processor in the error message?

this message might get confusing if people don't specify any fields, in which case we are going to throw index out of bounds later. btw we should add a test for that.

at this point I wonder if we should always require a field and a pattern ( or separator) for these processors too? key:value might be more verbose but clearer. it would make things also more consistent with other processors.

so,

"join" : {
  "field" : "joined_array_field",
  "separator" : "-"
}

I think you are correct. it will be more consistent. then it also gets rid of the whole "multiple fields" provided issue. where there is a concrete set of keys to be expected

@kimchy @martijnvg @clintongormley do you have any preferences surrounding this?

Whether these single field key:value configurations should move to a more explicit configurations with two fields, one representing the field, and one representing the value. In join's case, that value is the separator.

@talevy @javanna I personally prefer the key value approach (like in @talevy example) too.

@martijnvg haha, everything is a key-value and I have two examples. which one do you prefer? the existing solution?

or this example?

"join" : {
  "field" : "joined_array_field",
  "separator" : "-"
}

and this of course brings the question of consistency with all other processors as well. Are we explicit there or not. I see that with Gsub we are explicit... so now we have inconsistency across the different processors. I vote to always be explicit (so always have the "field" explicitly set)

++

I'll update the syntax for the ones that apply:

• convert
• join
• rename
• set
• split

updated to reflect the new spec

seems like the join example needs updating

@talevy this example seems not aligned still: "joined_array_field": "-"

when we have field: null containsKey will return true, but the method will return null as remove returns null, which was the actual value. I think that null shouldn't be returned and we should throw an exception instead, like we do in other methods. Just call remove and check the returned value, without containsKey.

ah, you're right

for the sake of separated error messages, I will treat contains property vs. null property value separately

I am also not entirely convinced this is correct. I was expecting a user to wish to set a field to be null.

yes, in a document. but this configuration, i think we can simplify and consider null and missing values the same way.

In all other methods in this class we don't call containsKey. I would stay aligned with those here.

@talevy I think you have missed my last comment ^ . We can simplify things like we do in other methods of the same class here.