Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10950 )
Change subject: IMPALA-376: add built-in functions for parsing JSON ...................................................................... Patch Set 9: (2 comments) I'm not planning on doing a full review but had a couple of high-level questions. http://gerrit.cloudera.org:8080/#/c/10950/9//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10950/9//COMMIT_MSG@11 PS9, Line 11: old RapidJson version, we cannot get detailed errors if parse fails. Should we consider upgrading rapidjson? http://gerrit.cloudera.org:8080/#/c/10950/9/be/src/exprs/string-functions.cc File be/src/exprs/string-functions.cc: http://gerrit.cloudera.org:8080/#/c/10950/9/be/src/exprs/string-functions.cc@51 PS9, Line 51: return ctx_->Allocate(size); The temporary result allocation mechanism (used by StringVal(FunctionContext* context, int len)) might be a better fit here since the allocations only have to live as long as the function executes, right? That's only currently exposed by that StringVal() constructor but we could potentially extend the UDF interface to expose it directly. -- To view, visit http://gerrit.cloudera.org:8080/10950 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6a9d3598cb3beca0865a7edb094f3a5b602dbd2f Gerrit-Change-Number: 10950 Gerrit-PatchSet: 9 Gerrit-Owner: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Mon, 23 Jul 2018 20:54:47 +0000 Gerrit-HasComments: Yes
