impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Zach Amsden (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4864 Speed up single slot predicates with dictionaries
Date Tue, 30 May 2017 19:10:00 GMT
Zach Amsden has posted comments on this change.

Change subject: IMPALA-4864 Speed up single slot predicates with dictionaries
......................................................................


Patch Set 9:

(13 comments)

http://gerrit.cloudera.org:8080/#/c/6726/9/be/src/exec/exec-node.cc
File be/src/exec/exec-node.cc:

PS9, Line 567: skippable_map
> skippable_map != nullptr
Done


PS9, Line 586: 0 
> huh ?
Was diverting for testing


PS9, Line 586: (*skippable_map)[i] == true
> (*skippable_map)[i]
Done


PS9, Line 590: bool
> skip_val
Done


http://gerrit.cloudera.org:8080/#/c/6726/9/be/src/exec/hdfs-parquet-scanner-ir.cc
File be/src/exec/hdfs-parquet-scanner-ir.cc:

PS9, Line 88: LOG(INFO) << "tuple idx: " << tuple_index;
> debugging output ?
Yeah, removed this


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

PS9, Line 1050: std::map<SlotId, std::vector<int>>&
> DictionaryFilterPositionMap
I think it will need to be HdfsScanNodeBase::DictionaryFilterPositionMap - not sure even a
using declaration will help have the syntax, but I'll try.


http://gerrit.cloudera.org:8080/#/c/6726/9/be/src/exec/hdfs-scan-node-base.h
File be/src/exec/hdfs-scan-node-base.h:

PS9, Line 167: typedef std::map<SlotId, std::vector<int>> DictFilterConjunctsPositionMap;
> Comment what this map stands for ?
Done


PS9, Line 366: Mapping to original position in conjuncts
> Mapping from SlotId to ...
Done


http://gerrit.cloudera.org:8080/#/c/6726/9/be/src/exec/parquet-column-readers.cc
File be/src/exec/parquet-column-readers.cc:

PS9, Line 206: IS_FILTERED
> Comment what IS_FILTERED stands for.
Done


PS9, Line 240: (filters == nullptr) ^ IS_FILTERED
> Is there a reason to not use DCHECK((filters == nullptr) != IS_FILTERED) ?
1 fewer character?  I can change this though.


http://gerrit.cloudera.org:8080/#/c/6726/9/be/src/exec/parquet-column-readers.h
File be/src/exec/parquet-column-readers.h:

PS9, Line 390: return false;
> Is this hard coded for testing ? If not, please document why it's always fa
No, it gets overridden in the derived class.


http://gerrit.cloudera.org:8080/#/c/6726/9/be/src/util/bitmap.h
File be/src/util/bitmap.h:

PS9, Line 86: (mask & buffer_[word_index])
> (mask & buffer[word_index]) == 0
Done


PS9, Line 99: buffer_.size()
> num_words_
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I65981c89e5292086809ec1268f5a273f4c1fe054
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <zamsden@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <joemcdonnell@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Zach Amsden <zamsden@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message