reuvenlax commented on issue #10413: [BEAM-9035] Typed options for Row Schema and Field URL: https://github.com/apache/beam/pull/10413#issuecomment-599170232 Some thoughts: I know your use case for options involves mapping specific input types to schemas, however we need to make sure that the actual mechanism is more generic than that. We have many uses for field metadata (or options) inside of Beam, completely unrelated to any option concept on input data types. I would not like to make a design choice that precludes nullable FieldTypes now, because we may very well find that we need nullable field types. That being said: I notice that you haven't removed the FieldType metadata in this PR. Is that planned for a future PR? I don't think we want both metadata and options in the protocol - Options seemed to me to be a better-typed version of metadata. I don't personally think that the hasOption code is too bad, and it's definitely better than constraining the type system for Options in a way that we quite likely will regret. That being said, you've already added the getValueOrDefault method. So you can still write this code: .getFields() .forEach( f -> { Integer pkSequence = f.getOptions().getValueOrDefault("vptech.data.contract.v1.primary_key", null); if (pkSequence != null) { primaryKeyMap.put(pkSequence, dataSchema.indexOf(f.getName())); } }); A tiny bit more verbose, but much more explicit about what you are trying to do. (FYI the reason I don't suggest allowing getValue to return an Optional<T> - even though it might be the "better" solution - is because the rest of the code base does not use Optional, and I want things to stay consistent). About the removeOption builder method. I'm struggling with this a bit, since it feels like a weird pattern to put remove into a builder. I'm also not sure whether copying a a Schema field with just a subset of options is a common use case or not. Generally we should start off by keeping such things out of the core Beam API until we are convinced that there are multiple users who need them. I think you can still accomplish the same thing by simply adding a new Options.Builder.setOptions override that takes in a Map. newField = newField.withOptions(Options.builder().setOptions( Maps.filterKeys( oldField.getOptions().getAllOptions(), n -> !n.equals(optionToRemove)))); This code is probably a bit more annoying for you to write. If we are convinced that this is a common use case then I'm ok with adding removeOption, but if it's specific to your use case then I think we're better off adding the new setOptions method and using that.
---------------------------------------------------------------- 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: [email protected] With regards, Apache Git Services
