[Impala-ASF-CR] WiP: IMPALA-10798 : Prototype for JSON reader

["Anonymous Coward (Code Review)" <ge...@cloudera.org>]

shikha.asrani10@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 <sh...@gmail.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <sh...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Wed, 01 Dec 2021 01:30:36 +0000
Gerrit-HasComments: Yes