8000
Skip to content

[AIP-144] Approve the repeated fields AIP. - #476

Merged
lukesneeringer merged 2 commits into
masterfrom
aip-144-approval
Apr 24, 2020
Merged

[AIP-144] Approve the repeated fields AIP.#476
lukesneeringer merged 2 commits into
masterfrom
aip-144-approval

Conversation

@lukesneeringer
Copy link
Copy Markdown
Contributor

This issue is intended for public comment, and should remain open through April 24, 2020.

@lukesneeringer lukesneeringer added this to the April 2020 Newsletter milestone Mar 26, 2020
@lukesneeringer
lukesneeringer requested a review from a team March 26, 2020 20:05
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 26, 2020
@esnow100
esnow100 commented Apr 2, 2020
Copy link
Copy Markdown

"Note: While field masks can not address individual elements in a repeated field, they can address individual elements in a map. Therefore, structured data with a clear primary key may be represented with a map instead."

Is this true? while I've seen some proposals to extend field masks to support proto maps, I don't believe that this is supported today. can you clarify this guidance?

Thanks!

@ohadassi
ohadassi commented Apr 2, 2020
Copy link
Copy Markdown

+1
I could not find a way to address individual elements in a map using a field mask.

@lukesneeringer
Copy link
Copy Markdown
Contributor Author

Is this true? while I've seen some proposals to extend field masks to support proto maps, I don't believe that this is supported today. can you clarify this guidance?

We have definitely had APIs do it, but it would not shock me if they all wrote their own implementations according to the proposal you mentioned.

I am not aware of an actual reference implementation (which would be fantastic to have).

@esnow100
esnow100 commented Apr 5, 2020
Copy link
Copy Markdown

Luke, to be more clear, the official spec for field mask: https://developers.google.com/protocol-buffers/docs/reference/java/com/google/protobuf/FieldMask

does not make any mention of proto maps or give any spec for them. anyone who uses them, is in uncharted territory using their own custom spec.

I think field mask should be officially extended to include maps, but until then, I dont think we should recommend such usage.

@lukesneeringer
Copy link
Copy Markdown
Contributor Author

I am of two minds here. We have a spec (essentially), just not a reference implementation.
Not having a reference implementation is lousy, but it is my (possibly incorrect?) understanding that a lot of people have rolled their own according to the spec.

That said, as the spec is not public, I can see how this is kind of lousy, and this statement is probably unhelpful. I agree with removing it.

@jskeet jskeet left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we'll probably want to give more details about the "update via a map" approach at some point, but I'm fine with this as it is for now.

8000

@esnow100
Copy link
Copy Markdown

@jskeet +1.

I think we should get the spec for a field mask extended to include map -- this would be helpful to my team and I'm sure to others as well.

@lukesneeringer
Copy link
Copy Markdown
Contributor Author

I agree also, but I view it as a separate exercise.

@lukesneeringer
lukesneeringer merged commit edddc39 into master Apr 24, 2020
@lukesneeringer
lukesneeringer deleted the aip-144-approval branch April 24, 2020 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

0