impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3346: DeepCopy() Kudu rows into Impala tuples.
Date Fri, 28 Oct 2016 04:40:20 GMT
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 <alex.behm@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message