You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Joe McDonnell (Code Review)" <ge...@cloudera.org> on 2017/03/09 17:18:05 UTC
[Impala-ASF-CR] IMPALA-5055: Fix DCHECK in parquet-column-readers.cc ReadPageHeader()
Joe McDonnell has uploaded a new change for review.
http://gerrit.cloudera.org:8080/6328
Change subject: IMPALA-5055: Fix DCHECK in parquet-column-readers.cc ReadPageHeader()
......................................................................
IMPALA-5055: Fix DCHECK in parquet-column-readers.cc ReadPageHeader()
GetBytes only sets status in the case of an error. This means that ReadPageHeader
needs to initialize the status variable so that the status.ok() check is
accurate after the GetBytes call.
I verified that the other uses of status are ok. Most do not check status.ok()
directly, but rely on the return value of the function setting status.
Change-Id: Ie22a8cf6b53f507c378c2efe302482409935184e
---
M be/src/exec/parquet-column-readers.cc
1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/28/6328/1
--
To view, visit http://gerrit.cloudera.org:8080/6328
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie22a8cf6b53f507c378c2efe302482409935184e
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
[Impala-ASF-CR] IMPALA-5055: Fix DCHECK in parquet-column-readers.cc ReadPageHeader()
Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.
Marcel Kornacker has posted comments on this change.
Change subject: IMPALA-5055: Fix DCHECK in parquet-column-readers.cc ReadPageHeader()
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6328/1//COMMIT_MSG
Commit Message:
Line 9: GetBytes only sets status in the case of an error. This means that ReadPageHeader
long lines
this makes the commit message hard to view in the gerrit ui
--
To view, visit http://gerrit.cloudera.org:8080/6328
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie22a8cf6b53f507c378c2efe302482409935184e
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5055: Fix DCHECK in parquet-column-readers.cc ReadPageHeader()
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change.
Change subject: IMPALA-5055: Fix DCHECK in parquet-column-readers.cc ReadPageHeader()
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/6328/1//COMMIT_MSG
Commit Message:
Line 9: GetBytes only sets status in the case of an error. This means that
> long lines
Done
Line 13: I verified that the other uses of status are ok. Most do not check
> I think we should change GetBytes() to always initialize status. The functi
Since this JIRA is a build blocker, I am reluctant to delay it to do performance tests. I filed IMPALA-5060 to track that change.
--
To view, visit http://gerrit.cloudera.org:8080/6328
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie22a8cf6b53f507c378c2efe302482409935184e
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5055: Fix DCHECK in parquet-column-readers.cc ReadPageHeader()
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6328
to look at the new patch set (#2).
Change subject: IMPALA-5055: Fix DCHECK in parquet-column-readers.cc ReadPageHeader()
......................................................................
IMPALA-5055: Fix DCHECK in parquet-column-readers.cc ReadPageHeader()
GetBytes only sets status in the case of an error. This means that
ReadPageHeader needs to initialize the status variable so that the
status.ok() check is accurate after the GetBytes call.
I verified that the other uses of status are ok. Most do not check
status.ok() directly, but rely on the return value of the function
setting status.
Change-Id: Ie22a8cf6b53f507c378c2efe302482409935184e
---
M be/src/exec/parquet-column-readers.cc
1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/28/6328/2
--
To view, visit http://gerrit.cloudera.org:8080/6328
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie22a8cf6b53f507c378c2efe302482409935184e
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5055: Fix DCHECK in parquet-column-readers.cc ReadPageHeader()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5055: Fix DCHECK in parquet-column-readers.cc ReadPageHeader()
......................................................................
Patch Set 2: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/6328
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie22a8cf6b53f507c378c2efe302482409935184e
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5055: Fix DCHECK in parquet-column-readers.cc ReadPageHeader()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5055: Fix DCHECK in parquet-column-readers.cc ReadPageHeader()
......................................................................
Patch Set 2:
Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/370/
--
To view, visit http://gerrit.cloudera.org:8080/6328
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie22a8cf6b53f507c378c2efe302482409935184e
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5055: Fix DCHECK in parquet-column-readers.cc ReadPageHeader()
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5055: Fix DCHECK in parquet-column-readers.cc ReadPageHeader()
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6328/1/be/src/exec/parquet-column-readers.cc
File be/src/exec/parquet-column-readers.cc:
Line 802: bool success = stream_->GetBytes(
> We use this pattern when things are retryable (e.g. in a lot of spilling co
I was able to confirm that - it traces back to this commit.
commit e0920e82e85b90d5b435c1afa1533dedc93c08e5
Author: Nong Li <no...@cloudera.com>
Date: Thu Oct 11 22:57:28 2012 -0700
Sequence file performance improvements.
--
To view, visit http://gerrit.cloudera.org:8080/6328
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie22a8cf6b53f507c378c2efe302482409935184e
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5055: Fix DCHECK in parquet-column-readers.cc ReadPageHeader()
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5055: Fix DCHECK in parquet-column-readers.cc ReadPageHeader()
......................................................................
Patch Set 1: Code-Review+2
I spoke to Joe offline and he mentioned that changing the return type to Status involved touching many more things. Given that I'm happy with this fix.
--
To view, visit http://gerrit.cloudera.org:8080/6328
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie22a8cf6b53f507c378c2efe302482409935184e
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5055: Fix DCHECK in parquet-column-readers.cc ReadPageHeader()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged.
Change subject: IMPALA-5055: Fix DCHECK in parquet-column-readers.cc ReadPageHeader()
......................................................................
IMPALA-5055: Fix DCHECK in parquet-column-readers.cc ReadPageHeader()
GetBytes only sets status in the case of an error. This means that
ReadPageHeader needs to initialize the status variable so that the
status.ok() check is accurate after the GetBytes call.
I verified that the other uses of status are ok. Most do not check
status.ok() directly, but rely on the return value of the function
setting status.
Change-Id: Ie22a8cf6b53f507c378c2efe302482409935184e
Reviewed-on: http://gerrit.cloudera.org:8080/6328
Reviewed-by: Marcel Kornacker <ma...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/exec/parquet-column-readers.cc
1 file changed, 1 insertion(+), 0 deletions(-)
Approvals:
Marcel Kornacker: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/6328
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ie22a8cf6b53f507c378c2efe302482409935184e
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5055: Fix DCHECK in parquet-column-readers.cc ReadPageHeader()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5055: Fix DCHECK in parquet-column-readers.cc ReadPageHeader()
......................................................................
Patch Set 2:
Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/368/
--
To view, visit http://gerrit.cloudera.org:8080/6328
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie22a8cf6b53f507c378c2efe302482409935184e
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5055: Fix DCHECK in parquet-column-readers.cc ReadPageHeader()
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change.
Change subject: IMPALA-5055: Fix DCHECK in parquet-column-readers.cc ReadPageHeader()
......................................................................
Patch Set 1:
Just to make it a matter of record:
GetBytes's call signature is mirrored by several other functions:
ReadBytes, SkipBytes, ReadBoolean, ReadInt, ReadVLong, etc. See scanner-context.h for the full list. A change to the signatures would impact several files and a hundred or so calls. It might still be worth doing, but it is something that should be done in a separate JIRA.
--
To view, visit http://gerrit.cloudera.org:8080/6328
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie22a8cf6b53f507c378c2efe302482409935184e
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5055: Fix DCHECK in parquet-column-readers.cc ReadPageHeader()
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5055: Fix DCHECK in parquet-column-readers.cc ReadPageHeader()
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6328/1/be/src/exec/parquet-column-readers.cc
File be/src/exec/parquet-column-readers.cc:
Line 802: bool success = stream_->GetBytes(
I feel like this pattern (returning a bool and setting status on error) is pretty error-prone. In a couple of perf-critical places we use it because it's faster than returning a Status (e.g. partitioned-hash-join-node), but I don't see a reason to use it here.
We could consider inverting it for GetBytes() - returning Status and setting a bool* to indicate success. What do you think?
--
To view, visit http://gerrit.cloudera.org:8080/6328
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie22a8cf6b53f507c378c2efe302482409935184e
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5055: Fix DCHECK in parquet-column-readers.cc ReadPageHeader()
Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.
Marcel Kornacker has posted comments on this change.
Change subject: IMPALA-5055: Fix DCHECK in parquet-column-readers.cc ReadPageHeader()
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/6328
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie22a8cf6b53f507c378c2efe302482409935184e
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5055: Fix DCHECK in parquet-column-readers.cc ReadPageHeader()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5055: Fix DCHECK in parquet-column-readers.cc ReadPageHeader()
......................................................................
Patch Set 2: Verified-1
Build failed: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/368/
--
To view, visit http://gerrit.cloudera.org:8080/6328
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie22a8cf6b53f507c378c2efe302482409935184e
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5055: Fix DCHECK in parquet-column-readers.cc ReadPageHeader()
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change.
Change subject: IMPALA-5055: Fix DCHECK in parquet-column-readers.cc ReadPageHeader()
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6328/1//COMMIT_MSG
Commit Message:
Line 13: I verified that the other uses of status are ok. Most do not check status.ok()
I think we should change GetBytes() to always initialize status. The function comment even seems vague on this, so we should update it, too.
Does any of the other uses rely on the fact that status does not get changed if no error occurs? If so, I think we should revisit that code.
--
To view, visit http://gerrit.cloudera.org:8080/6328
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie22a8cf6b53f507c378c2efe302482409935184e
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5055: Fix DCHECK in parquet-column-readers.cc ReadPageHeader()
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change.
Change subject: IMPALA-5055: Fix DCHECK in parquet-column-readers.cc ReadPageHeader()
......................................................................
Patch Set 1:
Since we expect no performance penalty and consumers shouldn't rely on status being untouched, can initialize it to OK() in GetBytes and do a private perf run to see whether it has a perf impact? I think that would eliminate a potential cause for future problems.
--
To view, visit http://gerrit.cloudera.org:8080/6328
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie22a8cf6b53f507c378c2efe302482409935184e
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5055: Fix DCHECK in parquet-column-readers.cc ReadPageHeader()
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change.
Change subject: IMPALA-5055: Fix DCHECK in parquet-column-readers.cc ReadPageHeader()
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6328/1/be/src/exec/parquet-column-readers.cc
File be/src/exec/parquet-column-readers.cc:
Line 802: bool success = stream_->GetBytes(
> I feel like this pattern (returning a bool and setting status on error) is
What would be the benefit of the bool* if we always return a Status?
--
To view, visit http://gerrit.cloudera.org:8080/6328
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie22a8cf6b53f507c378c2efe302482409935184e
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5055: Fix DCHECK in parquet-column-readers.cc ReadPageHeader()
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5055: Fix DCHECK in parquet-column-readers.cc ReadPageHeader()
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6328/1/be/src/exec/parquet-column-readers.cc
File be/src/exec/parquet-column-readers.cc:
Line 802: bool success = stream_->GetBytes(
> What would be the benefit of the bool* if we always return a Status?
We use this pattern when things are retryable (e.g. in a lot of spilling code, false + OK status means "we didn't succeed, but you should free memory and retry).
You make a good point - GetBytes() doesn't actually do that. We could just change it to return a Status.
I wonder if this is some vestige of an optimisation from when Status was much more expensive.
--
To view, visit http://gerrit.cloudera.org:8080/6328
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie22a8cf6b53f507c378c2efe302482409935184e
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5055: Fix DCHECK in parquet-column-readers.cc ReadPageHeader()
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change.
Change subject: IMPALA-5055: Fix DCHECK in parquet-column-readers.cc ReadPageHeader()
......................................................................
Patch Set 1:
My proposal is to keep the signature as it is, but initialize the status parameter to Status::OK() and do a private perf run to see if it has any negative impact on performance. If it does not, then I'd advocate to initialize it, since it may help prevent similar bugs in the future.
--
To view, visit http://gerrit.cloudera.org:8080/6328
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie22a8cf6b53f507c378c2efe302482409935184e
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No