shikha.asran...@gmail.com has posted comments on this change. ( http://gerrit.cloudera.org:8080/17771 )
Change subject: WiP: IMPALA-10798 : Prototype for JSON reader ...................................................................... Patch Set 10: (9 comments) http://gerrit.cloudera.org:8080/#/c/17771/9/be/src/exec/CMakeLists.txt File be/src/exec/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/17771/9/be/src/exec/CMakeLists.txt@71 PS9, Line 71: > nit: redundant whitespace Done http://gerrit.cloudera.org:8080/#/c/17771/9/be/src/exec/hdfs-json-scanner.h File be/src/exec/hdfs-json-scanner.h: http://gerrit.cloudera.org:8080/#/c/17771/9/be/src/exec/hdfs-json-scanner.h@38 PS9, Line 38: #include <arrow/memory_pool.h> : #include <arrow/result.h> : #include <arrow/type_fwd.h> : #include <arrow/util/macros.h> : #include <arrow/util/string_view.h> : #include <arrow/util/type_fwd.h> : #include "exec/hdfs-scan-node.h" > nit: use <> for external includes Done http://gerrit.cloudera.org:8080/#/c/17771/9/be/src/exec/hdfs-json-scanner.h@132 PS9, Line 132: row_read > nit: rows_read_ Done http://gerrit.cloudera.org:8080/#/c/17771/9/be/src/exec/hdfs-json-scanner.h@133 PS9, Line 133: num_rows > nit: num_rows_ Done http://gerrit.cloudera.org:8080/#/c/17771/9/be/src/exec/hdfs-json-scanner.h@136 PS9, Line 136: std::shared_ptr<arrow::json::TableReader> reader_ = nullptr; > nit: could you move this above to be together with the methods? Done http://gerrit.cloudera.org:8080/#/c/17771/9/be/src/exec/hdfs-json-scanner.cc File be/src/exec/hdfs-json-scanner.cc: http://gerrit.cloudera.org:8080/#/c/17771/9/be/src/exec/hdfs-json-scanner.cc@19 PS9, Line 19: #include "common/names.h" : #include "runtime/collection-value-builder.h" : #include "runtime/datetime-simple-date-format-parser.h" : #include "runtime/io/request-context.h" : #include "runtime/mem-tracker.h" : #include "runtime/row-batch.h" : #include "runtime/runtime-filter.inline.h" : #include "runtime/timestamp-value.h" : #include "runtime/timestamp-value.inline.h" : #include "runtime/tuple-row.h" : #include "util/decompress.h" : : using namespace impala; : using namespace impala::io; : : Status HdfsJsonScanner::IssueIni > nit: could you please remove headers that are included in hdfs-json-scanner Done http://gerrit.cloudera.org:8080/#/c/17771/9/be/src/exec/hdfs-json-scanner.cc@296 PS9, Line 296: *(reinterpret_cast<bool*> > nit: for (const auto& column : table_->columns()) Done http://gerrit.cloudera.org:8080/#/c/17771/9/be/src/exec/hdfs-json-scanner.cc@305 PS9, Line 305: memcpy(blob_, blob->data(), blob->size()); : char* src_ptr; > nit: could you rename these variables? I'm confused in what "ar" means.. BT Done http://gerrit.cloudera.org:8080/#/c/17771/9/cmake_modules/FindArrow.cmake File cmake_modules/FindArrow.cmake: http://gerrit.cloudera.org:8080/#/c/17771/9/cmake_modules/FindArrow.cmake@18 PS9, Line 18: # - Find Arrow (headers and libarrow.a) with ARROW_ROOT hinting a location : # This module defines : # ARROW_INCLUDE_DIR, directory containing headers : # ARROW_STATIC_LIB, path to libarrow.a : # ARROW_FOU > nit: please update these comments Done -- To view, visit http://gerrit.cloudera.org:8080/17771 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If79364a421d862d0d837f9be694911e388d4d629 Gerrit-Change-Number: 17771 Gerrit-PatchSet: 10 Gerrit-Owner: Anonymous Coward <shikha.asran...@gmail.com> Gerrit-Reviewer: Aman Sinha <amsi...@cloudera.com> Gerrit-Reviewer: Anonymous Coward <shikha.asran...@gmail.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Comment-Date: Wed, 01 Dec 2021 01:30:36 +0000 Gerrit-HasComments: Yes
