[GitHub] [beam] nehsyc commented on pull request #13069: [BEAM-10475] Add a well-known coder for ShardedKey in Java/Python SDK

[GitBox <gi...@apache.org>]

nehsyc commented on pull request #13069:
URL: https://github.com/apache/beam/pull/13069#issuecomment-707515750


   > > > > Regarding the yaml issues, I would like to see what the Python failures were. If it's just that this coder is not implemented, perhaps simply implementing it in Python as well would be the simplest.
   > > > 
   > > > 
   > > > Yeah the failure is that the coder is not implemented. I have a separate workspace for python. If it's okay to put python and java changes together I can merge those changes to this PR.
   > > 
   > > 
   > > Note that Go might also fail since it has some coder tests that rely on standard_coder.yaml. One PR can be fine.
   
   Thanks for pointing it out. The standard coder test in Go now skips ShardedKey coder test. Also added Python coder in this PR. (Sorry for the mess of commits and force pushes. The last commit was for formatting.)
   
   > > > > As for using the empty byte string vs. an explicit marker, I don't think the extra byte in savings is significant. I think the bigger question is whether logically we would want to allow the empty byte string as a shard key (and if it would complicate code/the contract to explicitly avoid it). If it's OK, it should be included as an example and test.
   > > > 
   > > > 
   > > > Empty is different from null. We could 1) allow null value and add a constraint on non-null shard id to be non-empty, or 2) not allow null value and assume empty bytes to be non-existing/default shard id. Adding an indicator would avoid the the constraint or assumption on the user side I think.
   > > 
   > > 
   > > I think always being non-null will be easier for users and runner authors. Having the default be "" is fine as it is expected that runners with their own implementation may choose other values. Runners can also choose "" if they so choose if they can differentiate internally as to whether something is re-using an existing shard id or defining a new one. All the runner / user cares about is that they are distinct.
   > 
   > Sounds reasonable. It wouldn't add complexity in code to avoid shard id being empty explicitly, unless the runner/user really wants to use empty bytes as a meaningful shard id in which case they need to be responsible for differentiating set/unset shard ids.
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org