[Impala-ASF-CR] IMPALA-2689: Log why codegen is disabled in TextConverter::CodegenWriteSlot()

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has submitted this change and it was merged.

Change subject: IMPALA-2689: Log why codegen is disabled in 
TextConverter::CodegenWriteSlot()
..


IMPALA-2689: Log why codegen is disabled in TextConverter::CodegenWriteSlot()

Plumbing statuses through TextConverter::CodegenWriteSlot(), which
eventually propagate to the runtime profile.

Change-Id: I355efe2d5566ba6a1b72648763794a79c80c3089
Reviewed-on: http://gerrit.cloudera.org:8080/7574
Reviewed-by: Tim Armstrong 
Tested-by: Impala Public Jenkins
---
M be/src/exec/hdfs-scanner.cc
M be/src/exec/text-converter.cc
M be/src/exec/text-converter.h
3 files changed, 53 insertions(+), 41 deletions(-)

Approvals:
  Impala Public Jenkins: Verified
  Tim Armstrong: Looks good to me, approved



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I355efe2d5566ba6a1b72648763794a79c80c3089
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 

[Impala-ASF-CR] IMPALA-2689: Log why codegen is disabled in TextConverter::CodegenWriteSlot()

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-2689: Log why codegen is disabled in 
TextConverter::CodegenWriteSlot()
..


Patch Set 5: Verified+1

-- 
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: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2689: Log why codegen is disabled in TextConverter::CodegenWriteSlot()

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-2689: Log why codegen is disabled in 
TextConverter::CodegenWriteSlot()
..


Patch Set 5: Code-Review+2

-- 
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: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2689: Log why codegen is disabled in TextConverter::CodegenWriteSlot()

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-2689: Log why codegen is disabled in 
TextConverter::CodegenWriteSlot()
..


Patch Set 4: Code-Review+2

-- 
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: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2689: Log why codegen is disabled in TextConverter::CodegenWriteSlot()

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-2689: Log why codegen is disabled in 
TextConverter::CodegenWriteSlot()
..


Patch Set 5:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1052/

-- 
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: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2689: Log why codegen is disabled in TextConverter::CodegenWriteSlot()

[Tim Armstrong (Code Review)]

Tim Armstrong has uploaded a new patch set (#5).

Change subject: IMPALA-2689: Log why codegen is disabled in 
TextConverter::CodegenWriteSlot()
..

IMPALA-2689: Log why codegen is disabled in TextConverter::CodegenWriteSlot()

Plumbing statuses through TextConverter::CodegenWriteSlot(), which
eventually propagate to the runtime profile.

Change-Id: I355efe2d5566ba6a1b72648763794a79c80c3089
---
M be/src/exec/hdfs-scanner.cc
M be/src/exec/text-converter.cc
M be/src/exec/text-converter.h
3 files changed, 53 insertions(+), 41 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/74/7574/5
-- 
To view, visit http://gerrit.cloudera.org:8080/7574
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I355efe2d5566ba6a1b72648763794a79c80c3089
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 

[Impala-ASF-CR] IMPALA-2689: Log why codegen is disabled in TextConverter::CodegenWriteSlot()

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-2689: Log why codegen is disabled in 
TextConverter::CodegenWriteSlot()
..


Patch Set 3: Code-Review+2

One business day has elapsed and it looks like Lars' comments were addressed. 
Let's move forward.

-- 
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 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2689: Log why codegen is disabled in TextConverter::CodegenWriteSlot()

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-2689: Log why codegen is disabled in 
TextConverter::CodegenWriteSlot()
..


Patch Set 3: Code-Review+1

-- 
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 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2689: Log why codegen is disabled in TextConverter::CodegenWriteSlot()

[anujphadke (Code Review)]

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 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2689: Log why codegen is disabled in TextConverter::CodegenWriteSlot()

[anujphadke (Code Review)]

anujphadke has uploaded a new patch set (#3).

Change subject: IMPALA-2689: Log why codegen is disabled in 
TextConverter::CodegenWriteSlot()
..

IMPALA-2689: Log why codegen is disabled in TextConverter::CodegenWriteSlot()

Plumbing statuses through TextConverter::CodegenWriteSlot(), which
eventually propagate to the runtime profile.

Change-Id: I355efe2d5566ba6a1b72648763794a79c80c3089
---
M be/src/exec/hdfs-scanner.cc
M be/src/exec/text-converter.cc
M be/src/exec/text-converter.h
3 files changed, 53 insertions(+), 41 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/74/7574/3
-- 
To view, visit http://gerrit.cloudera.org:8080/7574
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I355efe2d5566ba6a1b72648763794a79c80c3089
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke