impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Vuk Ercegovac (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4993: extend dictionary filtering to collections
Date Tue, 09 Jan 2018 23:58:02 GMT
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/8775 )

Change subject: IMPALA-4993: extend dictionary filtering to collections
......................................................................


Patch Set 6:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/8775/6/be/src/exec/hdfs-parquet-scanner.h
File be/src/exec/hdfs-parquet-scanner.h:

http://gerrit.cloudera.org:8080/#/c/8775/6/be/src/exec/hdfs-parquet-scanner.h@462
PS6, Line 462:   std::unordered_map<const TupleDescriptor*, std::unique_ptr<ScopedBuffer>>
> It would make more sense at this point to use a MemPool instead of a prolif
Replaced with MemPool.

It was unclear to me whether to use MemPool or ScopedBuffer for this case (I didn't see anything
in ScopedBuffer to point me in this direction). Let me know if you'd like:
1) a blurb in ScopedBuffer that refers to MemPool for the right use-cases
2) min_max_tuple_buffer also replaced to use MemPool.


http://gerrit.cloudera.org:8080/#/c/8775/6/be/src/exec/hdfs-parquet-scanner.h@649
PS6, Line 649:   /// Gets the TupleDescriptor of slot_desc.
> Mention that 'slot_desc' can belong to the top-level tuple or a tuple neste
Done


http://gerrit.cloudera.org:8080/#/c/8775/6/be/src/exec/hdfs-parquet-scanner.cc
File be/src/exec/hdfs-parquet-scanner.cc:

http://gerrit.cloudera.org:8080/#/c/8775/6/be/src/exec/hdfs-parquet-scanner.cc@771
PS6, Line 771:   if (!col_reader->IsCollectionReader()) {
> nit: I think this would be easier to follow if we reversed the branches and
Done


http://gerrit.cloudera.org:8080/#/c/8775/6/be/src/exec/hdfs-parquet-scanner.cc@815
PS6, Line 815:   for (auto* col_reader : column_readers_) {
> nit: could fit on one line now
Done


http://gerrit.cloudera.org:8080/#/c/8775/6/be/src/exec/hdfs-parquet-scanner.cc@1657
PS6, Line 1657:   if (column_readers.empty()) return Status::OK();
> Is the early exit necessary for correctness? Might be worth mentioning if i
removed.



-- 
To view, visit http://gerrit.cloudera.org:8080/8775
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If3a2abcfc3d0f7d18756816659fed77ce12668dd
Gerrit-Change-Number: 8775
Gerrit-PatchSet: 6
Gerrit-Owner: Vuk Ercegovac <vercegovac@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <vercegovac@cloudera.com>
Gerrit-Comment-Date: Tue, 09 Jan 2018 23:58:02 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message