impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bharath Vissapragada (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4196: Cross compile bit-byte-functions
Date Fri, 30 Sep 2016 18:14:12 GMT
Bharath Vissapragada has posted comments on this change.

Change subject: IMPALA-4196: Cross compile bit-byte-functions
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4557/3/testdata/workloads/functional-query/queries/QueryTest/exprs.test
File testdata/workloads/functional-query/queries/QueryTest/exprs.test:

Line 2499: select count(shiftleft(int_col, 1)) from functional_parquet.alltypes
> You mean it fails with an *error*, not a warning.  Okay.
No, it fails with a warning. Basically I tend to agree with Tim that the execution should
fall back to interpretation. The problem seems to be in ScalarFnCall::Prepare() (pasted the
snippet in the bottom) which fails if the codegen path fails for some reason. I think, we
should ideally have something like 

if (skip_codegen || codegen_fails) {
 // Interpret.
}

Should that be fixed too? Thoughts?

======================== SNIPPET ==================
if (skip_codegen) {
    // Builtins with char arguments must still have <= 8 arguments.
    // TODO: delete when we have codegen for char arguments
    if (has_char_arg_or_result) {
      DCHECK(NumFixedArgs() <= 8 && fn_.binary_type == TFunctionBinaryType::BUILTIN);
    }
    Status status = LibCache::instance()->GetSoFunctionPtr(
        fn_.hdfs_location, fn_.scalar_fn.symbol, &scalar_fn_, &cache_entry_);
    if (!status.ok()) {
      if (fn_.binary_type == TFunctionBinaryType::BUILTIN) {
        // Builtins symbols should exist unless there is a version mismatch.
        return Status(TErrorCode::MISSING_BUILTIN, fn_.name.function_name,
            fn_.scalar_fn.symbol);
      } else {
        DCHECK_EQ(fn_.binary_type, TFunctionBinaryType::NATIVE);
        return Status(Substitute("Problem loading UDF '$0':\n$1",
            fn_.name.function_name, status.GetDetail()));
        return status;
      }
    }
  } else {
    // If we got here, either codegen is enabled or we need codegen to run this function.
    LlvmCodeGen* codegen;
    RETURN_IF_ERROR(state->GetCodegen(&codegen));

    if (fn_.binary_type == TFunctionBinaryType::IR) {
      string local_path;
      RETURN_IF_ERROR(LibCache::instance()->GetLocalLibPath(
          fn_.hdfs_location, LibCache::TYPE_IR, &local_path));
      // Link the UDF module into this query's main module (essentially copy the UDF
      // module into the main module) so the UDF's functions are available in the main
      // module.
      RETURN_IF_ERROR(codegen->LinkModule(local_path));
    }


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5a1291bfd202b500405a884e4a62f0ca2447244a
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bharathv@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bharathv@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message