Lars Volker has posted comments on this change. Change subject: IMPALA-2869: Log why codegen is disabled in TextConverter::CodegenWriteSlot() ......................................................................
Patch Set 2: (11 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 purpose. PS2, Line 7: IMPALA-2869 Can you re-open the JIRA and assign it to yourself? PS2, Line 10: propogate typo 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* Why did the IR change? Line 111: if (slot_desc->type().type == TYPE_CHAR) { You could add a DCHECK(fn != nullptr) PS2, Line 126: TextConverter::CodegenWriteSlot Do you need this part? If the error gets logged, won't it include where it came from? PS2, Line 127: a null string >From the code and this error message it's not clear to me what went wrong. Can >you improve the error message? Line 232: return Status("TextConverter::CodegenWriteSlot: Codegen'd parser NYI for the" What does NYI mean? Maybe make this "Unsupported type". It should never be printed anyways and the Status will have a stack trace to find it. Line 280: Status("TextConverter::CodegenWriteSlot:codegen'd " This doesn't have any effect, did you forget a "return"? Line 281: "WriteSlot function failed verification"); Maybe call it "FinalizeFunction() failed"? The error message seems different from what's actually happening (finalize vs verify). http://gerrit.cloudera.org:8080/#/c/7574/2/be/src/exec/text-converter.h File be/src/exec/text-converter.h: Line 83: /// errors. Can you add a comment for the new output parameter? -- 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: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: anujphadke <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-HasComments: Yes
