Alex Behm has posted comments on this change. Change subject: IMPALA-3346: DeepCopy() Kudu rows into Impala tuples. ......................................................................
Patch Set 1: (4 comments) Matt, regarding your comments: 1. Removed varchar adjustment 2. Adding tests in Python seems like throwaway work. I'd prefer to wait for the SQL nullability support. http://gerrit.cloudera.org:8080/#/c/4862/1/be/src/exec/kudu-scanner.cc File be/src/exec/kudu-scanner.cc: Line 191: row->SetTuple(tuple_idx(), kudu_tuple); > If we just assume that tuple_idx() is 0, then we can just do something like Good idea. Done. http://gerrit.cloudera.org:8080/#/c/4862/1/be/src/exec/kudu-scanner.h File be/src/exec/kudu-scanner.h: Line 94: int tuple_idx() const { return 0; } > Not your change but this generality seems wholly unnecessary, I think we ca Good point. Done. http://gerrit.cloudera.org:8080/#/c/4862/1/be/src/runtime/tuple.cc File be/src/runtime/tuple.cc: Line 83: DeepCopy(result, desc, pool); > E.g. this is dropping the Status on the floor. See other comment. Just want to make sure we really really want me to make this change. http://gerrit.cloudera.org:8080/#/c/4862/1/be/src/runtime/tuple.h File be/src/runtime/tuple.h: Line 95: Status DeepCopy(Tuple* dst, const TupleDescriptor& desc, MemPool* pool); > There are other callsites that need to be fixed to handle the status, aren' These deep copies are all over the place in the code. Do you want me to change all of them to return a Status and have callers act on the Status? It's not like we are checking the mem limit today... I'm happy to make changes, just want to make sure we are prepared for the fallout (perf regressions?). -- To view, visit http://gerrit.cloudera.org:8080/4862 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic911e4eff9fe98bf28d8a1bab5c9d7e9ab66d9cb Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Alex Behm <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
