Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11359 )
Change subject: IMPALA-7203. Support UDFs in CatalogdMetaProvider ...................................................................... Patch Set 6: (2 comments) clarifying question for the thrift types. otherwise lgtm. http://gerrit.cloudera.org:8080/#/c/11359/6/common/thrift/Types.thrift File common/thrift/Types.thrift: http://gerrit.cloudera.org:8080/#/c/11359/6/common/thrift/Types.thrift@205 PS6, Line 205: specifier This is clear in the context of this change (and elegant), but I think it will be unclear without this context. Perhaps update the comment on L198? Another option is to just duplicate these fields used for lookups. This struct is already fairly context sensitive, e.g., what needs to be serialized to HMS, what's stored in the catalog, what's passed around with the plan. http://gerrit.cloudera.org:8080/#/c/11359/6/common/thrift/Types.thrift@206 PS6, Line 206: optional just checking, but when changing this from required to optional, existing, serialized TFunction objects will be able to be deserialized with this change? -- To view, visit http://gerrit.cloudera.org:8080/11359 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifef8ece9f214dca9441833b00f65c7c152d0ab53 Gerrit-Change-Number: 11359 Gerrit-PatchSet: 6 Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Wed, 05 Sep 2018 07:42:10 +0000 Gerrit-HasComments: Yes
