reuvenlax commented on PR #17172: URL: https://github.com/apache/beam/pull/17172#issuecomment-1124338991
Let me take another look. IIRC I had some concerns, but it's possible that the extensive benchmarking addressed those. On Wed, May 11, 2022 at 2:58 PM Brian Hulette ***@***.***> wrote: > ***@***.**** approved this pull request. > > Thanks @mosche <https://github.com/mosche> for the extensive > benchmarking, and apologies for the atrocious review latency! > > I'm ok with this if it passes current tests, given the extensive > benchmarking. A couple of things: > > - I'd still like to get some final feedback on the approach from > @reuvenlax <https://github.com/reuvenlax>, but I acn go ahead and > merge next Monday if that doesn't happen this week. > - It could be interesting to run your benchmarks continuously to 1) > monitor for regressions, and 2) give us a convenient way to test other > potential improvements. Maybe file a jira for that if you don't have time > now? > > ------------------------------ > > In > sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/FieldValueGetter.java > <https://github.com/apache/beam/pull/17172#discussion_r870780914>: > > > @@ -33,5 +33,9 @@ > @Nullable > ValueT get(ObjectT object); > > + default @Nullable Object getRaw(ObjectT object) { > > nit: maybe getUntyped is a more appropriate name here? > > Also could you clarify why we need this? > > — > Reply to this email directly, view it on GitHub > <https://github.com/apache/beam/pull/17172#pullrequestreview-970037354>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AFAYJVOBUJMEVNFJNON7Y7DVJQUOVANCNFSM5RRHFBKQ> > . > You are receiving this because you were mentioned.Message ID: > ***@***.***> > -- 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. To unsubscribe, e-mail: github-unsubscr...@beam.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
