Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/12168 )
Change subject: IMPALA-6503: Support reading complex types from ORC format files ...................................................................... Patch Set 8: (12 comments) Started to look at it. Currently mostly have comments about style. I plan to look at it deeper. http://gerrit.cloudera.org:8080/#/c/12168/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12168/8//COMMIT_MSG@25 PS8, Line 25: Don’t like nit: 'They are not like', or 'They differ from' http://gerrit.cloudera.org:8080/#/c/12168/8/be/src/exec/hdfs-orc-scanner.h File be/src/exec/hdfs-orc-scanner.h: http://gerrit.cloudera.org:8080/#/c/12168/8/be/src/exec/hdfs-orc-scanner.h@160 PS8, Line 160: orc::RowReaderOptions row_reader_options; nit: put '_' at the end of the variable name. http://gerrit.cloudera.org:8080/#/c/12168/8/be/src/exec/hdfs-orc-scanner.cc File be/src/exec/hdfs-orc-scanner.cc: http://gerrit.cloudera.org:8080/#/c/12168/8/be/src/exec/hdfs-orc-scanner.cc@181 PS8, Line 181: base nit: based http://gerrit.cloudera.org:8080/#/c/12168/8/be/src/exec/hdfs-orc-scanner.cc@329 PS8, Line 329: *template_tuple = : Tuple::Create(tuple_desc.byte_size(), template_tuple_pool_.get()); nit: put curly braces around it http://gerrit.cloudera.org:8080/#/c/12168/8/be/src/exec/hdfs-orc-scanner.cc@364 PS8, Line 364: if (id >= node.getColumnId() && id <= node.getMaximumColumnId()) return true; nit: put curly braces around it http://gerrit.cloudera.org:8080/#/c/12168/8/be/src/exec/hdfs-orc-scanner.cc@681 PS8, Line 681: RETURN_IF_ERROR(AssembleCollection(*child_reader, child_batch_offset + i, : coll_value_builder)); nit: add curly braces around it http://gerrit.cloudera.org:8080/#/c/12168/8/be/src/exec/orc-column-readers.h File be/src/exec/orc-column-readers.h: http://gerrit.cloudera.org:8080/#/c/12168/8/be/src/exec/orc-column-readers.h@101 PS8, Line 101: batch_ = dynamic_cast<orc::LongVectorBatch*>(orc_batch); We avoid doing dynamic casts in release mode. You can do a dynamic_cast inside a DCHECK, then use static_cast here. http://gerrit.cloudera.org:8080/#/c/12168/8/be/src/exec/orc-column-readers.h@126 PS8, Line 126: int64_t val = batch_->data.data()[row_idx]; Maybe you could add a DCHECK that batch_ is not null. http://gerrit.cloudera.org:8080/#/c/12168/8/be/src/exec/orc-column-readers.cc File be/src/exec/orc-column-readers.cc: http://gerrit.cloudera.org:8080/#/c/12168/8/be/src/exec/orc-column-readers.cc@190 PS8, Line 190: *slot = TimestampValue::FromUnixTimeNanos(secs, nanos, Do we know that orc timestamps are timezone-aware or not? http://gerrit.cloudera.org:8080/#/c/12168/8/be/src/exec/orc-column-readers.cc@326 PS8, Line 326: children_[c]->UpdateInputBatch(batch_->fields[children_fields_[c]]); nit: add curly brackets around it since it is a multi-line for stmt. http://gerrit.cloudera.org:8080/#/c/12168/8/be/src/exec/orc-column-readers.cc@331 PS8, Line 331: RETURN_IF_ERROR(child->ReadValue(row_idx_, tuple, pool)); nit: add curly brackets around it http://gerrit.cloudera.org:8080/#/c/12168/8/be/src/exec/orc-column-readers.cc@395 PS8, Line 395: } else CreateChildForSlot(node, slot_desc); nit: it's against the Google style guide to have braces on only one branch of an if-stmt: https://google.github.io/styleguide/cppguide.html#Conditionals -- To view, visit http://gerrit.cloudera.org:8080/12168 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I244dc9d2b3e425393f90e45632cb8cdbea6cf790 Gerrit-Change-Number: 12168 Gerrit-PatchSet: 8 Gerrit-Owner: Quanlong Huang <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Fri, 25 Jan 2019 17:25:23 +0000 Gerrit-HasComments: Yes
