impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "anujphadke (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-2689: Log why codegen is disabled in TextConverter::CodegenWriteSlot()
Date Fri, 11 Aug 2017 19:05:39 GMT
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 <aphadke@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: anujphadke <aphadke@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message