Michael Ho has posted comments on this change. Change subject: IMPALA-3838: Codegen EvalRuntimeFilters(). ......................................................................
Patch Set 2: (19 comments) http://gerrit.cloudera.org:8080/#/c/4833/1/be/src/codegen/llvm-codegen.cc File be/src/codegen/llvm-codegen.cc: PS1, Line 1415: > Why not private linkage? Done http://gerrit.cloudera.org:8080/#/c/4833/1/be/src/exec/filter-context.cc File be/src/exec/filter-context.cc: Line 82: if (local_bloom_filter == NULL) return; > Do we have a JIRA for this? IMPALA-3360. The goal is to codegen the expression evaluation and hashing via loop unrolling and constant propagation. The comment should actually be placed at the call site instead. PS1, Line 151: r_ctx->root(); > Maybe we should factor this out into a local Expr* variable since we're goi Done Line 155: > Do we need to clone this function? It doesn't look like we modify it. Done Line 158: codegen->GetFunction(IRFunction::RUNTIME_FILTER_EVAL, false); > We should document next to the class members that we're depending on the or Done PS1, Line 179: the stack and pas > How does this differ to BitCast? Should we be using this in some places ins The source 'native_ptr' needs to be of pointer type while this doesn't seem to be requirement for bitcast. May be good to use if elsewhere for stricter type checking but not necessarily applicable everywhere. Line 195: "expr_type_arg"); > We should document next to the class members that we're depending on the or Done Line 205: > Weird wrapping. Done http://gerrit.cloudera.org:8080/#/c/4833/1/be/src/exec/hdfs-parquet-scanner-ir.cc File be/src/exec/hdfs-parquet-scanner-ir.cc: Line 73: if (!filter_ctxs_[i]->Eval(row)) { > The logic already did this, but it's weird that we count it as rejected whe Actually, RuntimeFilter::Eval() will return true if the filter hasn't arrived yet so it won't be counted as rejected. http://gerrit.cloudera.org:8080/#/c/4833/1/be/src/exec/hdfs-parquet-scanner.cc File be/src/exec/hdfs-parquet-scanner.cc: PS1, Line 63: "ROWS_PER_FILTER_SELECTIVITY_CHECK must be a power of two"); > BitUtil::IsPowerOf2(). Done Line 273: if (compression_types.empty()) compression_types.push_back(THdfsCompression::NONE); > Long line Done Line 617: DCHECK_EQ(scan_node_->tuple_idx(), 0); > This doesn't seem to be particularly related to TransferScratchTuples(). Ma Done PS1, Line 720: > Maybe "filter_rejected_block" or "return_false_block" or something. bail_ou Done Line 721: *fn = NULL; > Might be simpler to just handle the 0-filter case up the top by generating Done Line 725: codegen->GetType(TYPE_BOOLEAN)); > I don't think this makes sense - we don't expect the filters to arrive at t Done http://gerrit.cloudera.org:8080/#/c/4833/1/be/src/runtime/raw-value-ir.cc File be/src/runtime/raw-value-ir.cc: Line 111: // The choice of hash function needs to be consistent across all hosts of the cluster. > Missing space Done http://gerrit.cloudera.org:8080/#/c/4833/1/be/src/runtime/runtime-filter.h File be/src/runtime/runtime-filter.h: Line 64: > Document 'val' - not self-explanatory Done Line 86: > Missed this one Done http://gerrit.cloudera.org:8080/#/c/4833/1/tests/query_test/test_tpch_queries.py File tests/query_test/test_tpch_queries.py: Line 39: v.get_value('table_format').file_format in ['text', 'parquet', 'kudu']) > I guess because we don't have the same runtime filter support for text. Yes, I needed to use parquet format for debugging my change. -- To view, visit http://gerrit.cloudera.org:8080/4833 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I27114869840e268d17e91d6e587ef811628e3837 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Mostafa Mokhtar <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
