Henry Robinson has posted comments on this change.

Change subject: IMPALA-(3895,3859): Don't log file data on parse errors
......................................................................


Patch Set 1:

(3 comments)

Yes - hdfs-rcfile-scan-node-errors.test needed updating. The reason I missed it 
was a bug: these test *never* ran - and were broken - because they didn't have 
a RESULTS section and were silently skipped. I added logic to assert if there 
was an ERROR section with no RESULTS section.

I'm running a core build to see if there are other tests that are affected by 
this bug that will now throw errors - so this patch might churn a bit.

http://gerrit.cloudera.org:8080/#/c/4020/1//COMMIT_MSG
Commit Message:

Line 7: IMPALA-(3895,3859): Don't log file data on parse errors
> IMPALA-1872 too? or more needed?  I guess we can use that JIRA to remove Sk
Yeah, I'd like to keep the scope of this change limited to 3859 / 3895 for now.


PS1, Line 16: __DFS_FILENAME__
> This works for s3a and file scheme as well, right?  So how about calling it
Ha, I was working off your last suggestion over email not to call it HDFS*. 

I'd prefer not to use just __FILENAME__, because then it's not clear that a URI 
won't be matched. 

I'll go with HDFS for now.


http://gerrit.cloudera.org:8080/#/c/4020/1/testdata/workloads/functional-query/queries/QueryTest/parquet-continue-on-error.test
File 
testdata/workloads/functional-query/queries/QueryTest/parquet-continue-on-error.test:

PS1, Line 84: : 
> why is it : here but = above?
Typo, fixed - already caught that from the last Jenkins run.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5a604f8784a9ff7b4bf878f82ee7f56697df3272
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <[email protected]>
Gerrit-Reviewer: Dan Hecht <[email protected]>
Gerrit-Reviewer: Henry Robinson <[email protected]>
Gerrit-Reviewer: Tim Armstrong <[email protected]>
Gerrit-HasComments: Yes

Reply via email to