Thomas Tauber-Marshall has posted comments on this change. Change subject: IMPALA-3360: Codegen inserting into runtime filters ......................................................................
Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/8029/2/be/src/exec/filter-context.cc File be/src/exec/filter-context.cc: Line 217: > You're right, we should be able to cross-compile Insert() and substitute in So I've been working on this, but it turns out to be tricky. I uploaded a new patch, but it doesn't fully work. One issue is that the function returned by ScalarExpr::GetCodegendComputeFn() can't be used as a drop-in replacement for ScalarExprEvaluator::GetValue() as they have different return types (AnyVal and void* respectively). I solved this with ScalarExpr::GetCodegendComputePtrFn(), but it means keeping a lot of the IRBuilder code that's here. The other issue is how to deal with making the type argument to GetHashValue() a constant. As far as I can tell, we don't already have any tools for directly replacing arguments to functions. The latest version of the patch defines a function GetType(), called in FilterContext::Insert(), that I replace with ReplaceCallSitesWithValue(), but this doesn't work (in particular, the values to be hashed that are passed to GetHashValue() appear to be corrupted, and from dumping the IR the branching isn't eliminated anyways), and I'm not sure if its even reasonable to expect it to work since we don't use ReplaceCallSitesWithValue() in that way anywhere else. I also considered making something like RawValue::CodegenGetHashValue() which would take the type and return a codegen'd function that can be used to replace GetHashValue() with ReplaceCallSites() (ie. the codegen'd function would still also take a type argument but would ignore it), but this would be a bunch more IRBuilder (more even than the original version of the patch, I think), so I wanted to see if that approach makes sense before doing it. http://gerrit.cloudera.org:8080/#/c/8029/2/be/src/util/bloom-filter.h File be/src/util/bloom-filter.h: Line 135: // Same as Insert(), but skips the CPU check and assumes that AVX is not available. > naming nit: We actually need AVX2 -- AVX won't cut it Done -- To view, visit http://gerrit.cloudera.org:8080/8029 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I79cf23ad92dadaab996a50a2ca07ef9ebe8639bb Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
