Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/12247 )
Change subject: IMPALA-5051: Add INT64 timestamp write support in Parquet ...................................................................... Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/12247/3/be/src/exec/parquet/hdfs-parquet-table-writer.cc File be/src/exec/parquet/hdfs-parquet-table-writer.cc: http://gerrit.cloudera.org:8080/#/c/12247/3/be/src/exec/parquet/hdfs-parquet-table-writer.cc@583 PS3, Line 583: Overrides of this function are expected to set 'result_' > I prefer the current solution - my guess is that adding an additional argum It seemed premature optimization to me, so I did some measurements and looked at the generated assembly. Both 'this' and the output parameter are passed in via registers. In the current case you need less registers, but you need to access 'result_' via 'this' plus an offset. In the other case, you need one more register to pass the output parameter, but you don't need to add an offset to 'this'. I couldn't measure any performance difference between the two approaches. So if performance was the only reason to do this, I'd rather choose the conventional way. -- To view, visit http://gerrit.cloudera.org:8080/12247 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib41ad532ec902ed5a9a1528513726eac1c11441f Gerrit-Change-Number: 12247 Gerrit-PatchSet: 3 Gerrit-Owner: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Zoltan Ivanfi <[email protected]> Gerrit-Comment-Date: Thu, 24 Jan 2019 17:13:41 +0000 Gerrit-HasComments: Yes
