anujphadke has posted comments on this change. Change subject: IMPALA-2689: Log why codegen is disabled in TextConverter::CodegenWriteSlot() ......................................................................
Patch Set 3: (13 comments) http://gerrit.cloudera.org:8080/#/c/7574/2//COMMIT_MSG Commit Message: PS2, Line 7: why codegen is disabled in > How about "Log errors in"? "Disabled" reads as if it was done by a user on Tried to keep it consistent with the older fixes - http://gerrit.cloudera.org:8080/#/c/2048/ Let me know if this is fine. I will change it if sounds misleading. PS2, Line 10: propagate > typo Done http://gerrit.cloudera.org:8080/#/c/7574/2/be/src/exec/hdfs-scanner.cc File be/src/exec/hdfs-scanner.cc: PS2, Line 326: tabl > Switch to nullptr Done http://gerrit.cloudera.org:8080/#/c/7574/1/be/src/exec/text-converter.cc File be/src/exec/text-converter.cc: Line 280: // Parse failed, set slot to null and return false > ? Done http://gerrit.cloudera.org:8080/#/c/7574/2/be/src/exec/text-converter.cc File be/src/exec/text-converter.cc: Line 76: // %1 = bitcast <{ i32, i8 }>* %tuple_arg to i8* > Same question. I'm guessing that the old IR was just stale, in which case - This patch should not change the IR. This is just the updated IR. Line 111: DCHECK(fn != nullptr); > You could add a DCHECK(fn != nullptr) Done Line 125: } > This could be a DCHECK. We shouldn't be missing builtin functions. Done PS2, Line 126: > Yes, that sounds reasonable. Please add () to the function names to follow Added a DCHECK instead. PS2, Line 127: is_null_strin > From the code and this error message it's not clear to me what went wrong. Added a DCHECK. Line 232: default: > What does NYI mean? Maybe make this "Unsupported type". It should never be Not yet implemented. We have used this acronym elsewhere while fixing this JIRA in other places. This message gets propagated to the runtime profile. Line 280: // Parse failed, set slot to null and return false > This doesn't have any effect, did you forget a "return"? Forgot to git add. Had this change locally. Line 281: builder.SetInsertPoint(parse_failed_block); > Following the same convention as elsewhere in the code makes sense to me. Done http://gerrit.cloudera.org:8080/#/c/7574/2/be/src/exec/text-converter.h File be/src/exec/text-converter.h: Line 83: /// strict_mode: If set, numerical overflow/underflow are considered to be parse > Can you add a comment for the new output parameter? Done -- To view, visit http://gerrit.cloudera.org:8080/7574 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I355efe2d5566ba6a1b72648763794a79c80c3089 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: anujphadke <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: anujphadke <[email protected]> Gerrit-HasComments: Yes
