You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@impala.apache.org by "Alex Behm (Code Review)" <ge...@cloudera.org> on 2016/08/08 20:34:16 UTC
[Impala-ASF-CR] IMPALA-3943: Adhere to abort on error when a Parquet file has no row groups.
Alex Behm has uploaded a new change for review.
http://gerrit.cloudera.org:8080/3862
Change subject: IMPALA-3943: Adhere to abort_on_error when a Parquet file has no row groups.
......................................................................
IMPALA-3943: Adhere to abort_on_error when a Parquet file has no row groups.
Change-Id: I6aff766a1ce6376efb329bdde51c648149dfe08c
---
M be/src/exec/hdfs-parquet-scanner.cc
M testdata/data/README
A testdata/data/no_row_groups.parquet
A testdata/workloads/functional-query/queries/QueryTest/no-row-groups-abort.test
A testdata/workloads/functional-query/queries/QueryTest/no-row-groups.test
M tests/query_test/test_scanners.py
6 files changed, 39 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/62/3862/1
--
To view, visit http://gerrit.cloudera.org:8080/3862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6aff766a1ce6376efb329bdde51c648149dfe08c
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
[Impala-ASF-CR] IMPALA-3943: Adhere to abort on error when a Parquet file has no row groups.
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-3943: Adhere to abort_on_error when a Parquet file has no row groups.
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3862/3/be/src/exec/hdfs-parquet-scanner.cc
File be/src/exec/hdfs-parquet-scanner.cc:
Line 856: }
> My original point was that there are different kinds of errors, ranging fro
One could argue that empty row group should only be a warning (i.e. regardless of abort_on_error), and that's how it's distinct from the other cases. But that's not what this change proposes.
But if we want to treat this case as an error, then it seems best to treat all errors as the same (and have consistent behavior w.r.t. abort_on_error).
--
To view, visit http://gerrit.cloudera.org:8080/3862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6aff766a1ce6376efb329bdde51c648149dfe08c
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3943: Adhere to abort on error when a Parquet file has no row groups.
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-3943: Adhere to abort_on_error when a Parquet file has no row groups.
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/3862/3/be/src/exec/hdfs-parquet-scanner.cc
File be/src/exec/hdfs-parquet-scanner.cc:
PS3, Line 854: Parqet
typo
Line 856: }
seems okay, but why treat this case differently than the other invalid metadata cases?
--
To view, visit http://gerrit.cloudera.org:8080/3862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6aff766a1ce6376efb329bdde51c648149dfe08c
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3943: Adhere to abort on error when a Parquet file has no row groups.
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-3943: Adhere to abort_on_error when a Parquet file has no row groups.
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3862/1/be/src/exec/hdfs-parquet-scanner.cc
File be/src/exec/hdfs-parquet-scanner.cc:
PS1, Line 178: if (!status.ok()) RETURN_IF_ERROR(state_->LogOrReturnError(status.msg()));
> I manually tried this out with ProcessFooter() returning from various place
Done
--
To view, visit http://gerrit.cloudera.org:8080/3862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6aff766a1ce6376efb329bdde51c648149dfe08c
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3943: Adhere to abort on error in ProcessFooter().
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3862
to look at the new patch set (#5).
Change subject: IMPALA-3943: Adhere to abort_on_error in ProcessFooter().
......................................................................
IMPALA-3943: Adhere to abort_on_error in ProcessFooter().
This patch focuses on properly handling abort_on_error when ProcessFooter()
fails in the Parquet scanner.
It also refactors a few existing tests to cover both possible values
of abort_on_error.
There are still several places left where abort_on_error is not honored.
Testing: I ran test_scanners.py::TestParquet and test_stale_metadata.py
locally on exhaustive with debug and asan builds. I ran the data errors
tests in a loop for 20 iterations. I also did a private core/hdfs run.
Change-Id: I6aff766a1ce6376efb329bdde51c648149dfe08c
---
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-parquet-scanner.h
M testdata/data/README
A testdata/data/no_row_groups.parquet
M testdata/workloads/functional-query/queries/DataErrorsTest/avro-errors.test
M testdata/workloads/functional-query/queries/DataErrorsTest/hdfs-scan-node-errors.test
A testdata/workloads/functional-query/queries/QueryTest/no-row-groups-abort.test
A testdata/workloads/functional-query/queries/QueryTest/no-row-groups.test
M testdata/workloads/functional-query/queries/QueryTest/parquet-abort-on-error.test
M testdata/workloads/functional-query/queries/QueryTest/parquet-continue-on-error.test
M testdata/workloads/functional-query/queries/QueryTest/parquet.test
M tests/common/test_dimensions.py
M tests/common/test_result_verifier.py
M tests/metadata/test_stale_metadata.py
M tests/query_test/test_scanners.py
15 files changed, 176 insertions(+), 78 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/62/3862/5
--
To view, visit http://gerrit.cloudera.org:8080/3862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6aff766a1ce6376efb329bdde51c648149dfe08c
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-3943: Adhere to abort on error in ProcessFooter().
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-3943: Adhere to abort_on_error in ProcessFooter().
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3862/5/be/src/exec/hdfs-parquet-scanner.cc
File be/src/exec/hdfs-parquet-scanner.cc:
PS5, Line 183: if (!status.ok()) return state_->LogOrReturnError(status.msg())
> My concern was if we get far enough to set the file metadata but it fails v
CreateSchemaTree() is done in the schema resolver on L186 which we would not reach if ProcessFooter() returns !ok(), so I still don't see a problem.
Regarding the files: Mostafa listed some broken files on the JIRA that contain the creation date. They were created in February.
--
To view, visit http://gerrit.cloudera.org:8080/3862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6aff766a1ce6376efb329bdde51c648149dfe08c
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3943: Adhere to abort on error in ProcessFooter().
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-3943: Adhere to abort_on_error in ProcessFooter().
......................................................................
Patch Set 5:
Please see my comment on the JIRA. Looking at the old code, I don't see how this could be a regression, so let's not rush to changing this. If you can confirm it's not a regression, I think we should de-prioritize the JIRA and not bother with this now.
--
To view, visit http://gerrit.cloudera.org:8080/3862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6aff766a1ce6376efb329bdde51c648149dfe08c
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3943: Adhere to abort on error in ProcessFooter().
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-3943: Adhere to abort_on_error in ProcessFooter().
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3862/5/be/src/exec/hdfs-parquet-scanner.cc
File be/src/exec/hdfs-parquet-scanner.cc:
PS5, Line 183: if (!status.ok()) return state_->LogOrReturnError(status.msg())
> I don't understand why we expect this to work if the file_metadata_ and col
Right, this won't work as was discussed in the earlier comment.
Alex, which specific change caused this regression (to hard-error on zero row-groups)? I'd like to look at the code before that change to see what state we were in.
--
To view, visit http://gerrit.cloudera.org:8080/3862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6aff766a1ce6376efb329bdde51c648149dfe08c
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3943: Adhere to abort on error in ProcessFooter().
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-3943: Adhere to abort_on_error in ProcessFooter().
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3862/5/be/src/exec/hdfs-parquet-scanner.cc
File be/src/exec/hdfs-parquet-scanner.cc:
PS5, Line 183: if (!status.ok()) return state_->LogOrReturnError(status.msg())
> Oops, I was looking at an older version of the code. The problem is still
Convinced. Added flag.
--
To view, visit http://gerrit.cloudera.org:8080/3862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6aff766a1ce6376efb329bdde51c648149dfe08c
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3943: Adhere to abort on error when a Parquet file has no row groups.
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has uploaded a new patch set (#2).
Change subject: IMPALA-3943: Adhere to abort_on_error when a Parquet file has no row groups.
......................................................................
IMPALA-3943: Adhere to abort_on_error when a Parquet file has no row groups.
Change-Id: I6aff766a1ce6376efb329bdde51c648149dfe08c
---
M be/src/exec/hdfs-parquet-scanner.cc
M testdata/data/README
A testdata/data/no_row_groups.parquet
A testdata/workloads/functional-query/queries/QueryTest/no-row-groups-abort.test
A testdata/workloads/functional-query/queries/QueryTest/no-row-groups.test
M tests/query_test/test_scanners.py
6 files changed, 41 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/62/3862/2
--
To view, visit http://gerrit.cloudera.org:8080/3862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6aff766a1ce6376efb329bdde51c648149dfe08c
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-3943: Adhere to abort on error when a Parquet file has no row groups.
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-3943: Adhere to abort_on_error when a Parquet file has no row groups.
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3862/3/be/src/exec/hdfs-parquet-scanner.cc
File be/src/exec/hdfs-parquet-scanner.cc:
Line 856: }
> the error handle for all of these would be the same, right? so i think we c
My original point was that there are different kinds of errors, ranging from corruption to no data (like this case), and that they maybe would behave differently (e.g. this is OK to warn on but corrupt files are not). I was thinking that this case isn't as bad as a header that can't even be read, so I was asking if we'd want to avoid putting all things in the category of abort on error, regardless of how severe they are. (And as an aside, by handling this here for this case, we shouldn't need to short circuit in GetNext().) That said, maybe the distinction between these errors isn't useful, and I see the benefit to handling all errors in a single way.
--
To view, visit http://gerrit.cloudera.org:8080/3862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6aff766a1ce6376efb329bdde51c648149dfe08c
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3943: Adhere to abort on error when a Parquet file has no row groups.
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-3943: Adhere to abort_on_error when a Parquet file has no row groups.
......................................................................
Patch Set 3: Code-Review+1
Carry Tim's +1
--
To view, visit http://gerrit.cloudera.org:8080/3862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6aff766a1ce6376efb329bdde51c648149dfe08c
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3943: Adhere to abort on error in ProcessFooter().
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-3943: Adhere to abort_on_error in ProcessFooter().
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3862/5/be/src/exec/hdfs-parquet-scanner.cc
File be/src/exec/hdfs-parquet-scanner.cc:
PS5, Line 183: if (!status.ok()) return state_->LogOrReturnError(status.msg())
I don't understand why we expect this to work if the file_metadata_ and column_readers_ was never set but we return OK. It looks to me like we'd index into garbage in GetNextInternal() when we index into column_readers_[0] which was never populated by Open(). Perhaps I'm missing something though? I'd think we'd want a way to explicitly short circuit return from GetNextInternal(), which is I think what Tim was suggesting originally (before we went with the more surgical fix, and then obviously now we've backed out).
--
To view, visit http://gerrit.cloudera.org:8080/3862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6aff766a1ce6376efb329bdde51c648149dfe08c
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3943: Adhere to abort on error when a Parquet file has no row groups.
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-3943: Adhere to abort_on_error when a Parquet file has no row groups.
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3862/3/be/src/exec/hdfs-parquet-scanner.cc
File be/src/exec/hdfs-parquet-scanner.cc:
Line 856: }
> One could argue that empty row group should only be a warning (i.e. regardl
I moved this check to the caller to handle more cases. I modified existing tests to run with both values of abort_on_error. They do not provide 100% coverage of all cases within ProcessFooter(), but certainly a few important ones.
Fixing and testing all possible scenarios with corrupt data files and making all error cases adhere to abort_on_error feels beyond the scope of this patch. I think we should focus our time and efforts on more pressing matters first.
--
To view, visit http://gerrit.cloudera.org:8080/3862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6aff766a1ce6376efb329bdde51c648149dfe08c
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3943: Adhere to abort on error in ProcessFooter().
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3862
to look at the new patch set (#7).
Change subject: IMPALA-3943: Adhere to abort_on_error in ProcessFooter().
......................................................................
IMPALA-3943: Adhere to abort_on_error in ProcessFooter().
This patch focuses on properly handling abort_on_error when ProcessFooter()
fails in the Parquet scanner.
It also refactors a few existing tests to cover both possible values
of abort_on_error.
There are still several places left where abort_on_error is not honored.
Testing: I ran test_scanners.py::TestParquet and test_stale_metadata.py
locally on exhaustive with debug and asan builds. I ran the data errors
tests in a loop for 20 iterations. I also did a private core/hdfs run.
Change-Id: I6aff766a1ce6376efb329bdde51c648149dfe08c
---
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-parquet-scanner.h
M testdata/data/README
A testdata/data/no_row_groups.parquet
M testdata/workloads/functional-query/queries/DataErrorsTest/avro-errors.test
M testdata/workloads/functional-query/queries/DataErrorsTest/hdfs-scan-node-errors.test
A testdata/workloads/functional-query/queries/QueryTest/no-row-groups-abort.test
A testdata/workloads/functional-query/queries/QueryTest/no-row-groups.test
M testdata/workloads/functional-query/queries/QueryTest/parquet-abort-on-error.test
M testdata/workloads/functional-query/queries/QueryTest/parquet-continue-on-error.test
M testdata/workloads/functional-query/queries/QueryTest/parquet.test
M tests/common/test_dimensions.py
M tests/common/test_result_verifier.py
M tests/metadata/test_stale_metadata.py
M tests/query_test/test_scanners.py
15 files changed, 197 insertions(+), 79 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/62/3862/7
--
To view, visit http://gerrit.cloudera.org:8080/3862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6aff766a1ce6376efb329bdde51c648149dfe08c
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-3943: Adhere to abort on error when a Parquet file has no row groups.
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-3943: Adhere to abort_on_error when a Parquet file has no row groups.
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3862/3/be/src/exec/hdfs-parquet-scanner.cc
File be/src/exec/hdfs-parquet-scanner.cc:
Line 856: }
> I'm not opposed to handling _more_ errors like this, it just wasn't obvious
the error handle for all of these would be the same, right? so i think we can reasonable test coverage with just a couple of example bad files. (of course, exercising all paths is best, but i don't see that as a pre-req for this change).
and better to put the code where it belongs, otherwise other code readers will sit puzzled like I was about why this case was so special.
--
To view, visit http://gerrit.cloudera.org:8080/3862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6aff766a1ce6376efb329bdde51c648149dfe08c
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3943: Adhere to abort on error in ProcessFooter().
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-3943: Adhere to abort_on_error in ProcessFooter().
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3862/5/be/src/exec/hdfs-parquet-scanner.cc
File be/src/exec/hdfs-parquet-scanner.cc:
PS5, Line 183: if (!status.ok()) return state_->LogOrReturnError(status.msg())
> I maintain this will work, and I did run through these paths on ASAN.
My concern was if we get far enough to set the file metadata but it fails validation, e.g. CreateSchemaTree() fails. Isn't that a problem?
In any case, I'll try to find time tomorrow to see why this has started failing recently since the code doesn't seem to have actually regressed yet those files were around for a while (how do you know that? are you sure data wasn't regenerated recently?).
--
To view, visit http://gerrit.cloudera.org:8080/3862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6aff766a1ce6376efb329bdde51c648149dfe08c
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3943: Adhere to abort on error when a Parquet file has no row groups.
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3862
to look at the new patch set (#4).
Change subject: IMPALA-3943: Adhere to abort_on_error when a Parquet file has no row groups.
......................................................................
IMPALA-3943: Adhere to abort_on_error when a Parquet file has no row groups.
Change-Id: I6aff766a1ce6376efb329bdde51c648149dfe08c
---
M be/src/exec/hdfs-parquet-scanner.cc
M testdata/data/README
A testdata/data/no_row_groups.parquet
A testdata/workloads/functional-query/queries/QueryTest/no-row-groups-abort.test
A testdata/workloads/functional-query/queries/QueryTest/no-row-groups.test
M tests/query_test/test_scanners.py
6 files changed, 41 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/62/3862/4
--
To view, visit http://gerrit.cloudera.org:8080/3862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6aff766a1ce6376efb329bdde51c648149dfe08c
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-3943: Adhere to abort on error when a Parquet file has no row groups.
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-3943: Adhere to abort_on_error when a Parquet file has no row groups.
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3862/3/be/src/exec/hdfs-parquet-scanner.cc
File be/src/exec/hdfs-parquet-scanner.cc:
Line 856: }
> I don't think we should treat it any differently. I originally had this che
If there's no good reason to treat them differently, then let's do it right. Deleting the bad files seems like a fine way to buy some more time to do that. (How did these files get created in the first place?)
--
To view, visit http://gerrit.cloudera.org:8080/3862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6aff766a1ce6376efb329bdde51c648149dfe08c
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3943: Adhere to abort on error in ProcessFooter().
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-3943: Adhere to abort_on_error in ProcessFooter().
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3862/7/be/src/exec/hdfs-parquet-scanner.cc
File be/src/exec/hdfs-parquet-scanner.cc:
Line 188: if (!status.ok()) return state_->LogOrReturnError(status.msg());
> Don't we want to return early if there was any kind of error? If its a non-
Please disregard this comment, I was confused.
--
To view, visit http://gerrit.cloudera.org:8080/3862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6aff766a1ce6376efb329bdde51c648149dfe08c
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3943: Adhere to abort on error when a Parquet file has no row groups.
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-3943: Adhere to abort_on_error when a Parquet file has no row groups.
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3862/1/be/src/exec/hdfs-parquet-scanner.cc
File be/src/exec/hdfs-parquet-scanner.cc:
PS1, Line 178: if (!status.ok()) RETURN_IF_ERROR(state_->LogOrReturnError(status.msg()));
> Done
As Tim pointed out, it doesn't seem like this would work in all cases since the metadata will be in some weird state. Seems we should short-circuit this.
--
To view, visit http://gerrit.cloudera.org:8080/3862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6aff766a1ce6376efb329bdde51c648149dfe08c
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3943: Adhere to abort on error when a Parquet file has no row groups.
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3862
to look at the new patch set (#3).
Change subject: IMPALA-3943: Adhere to abort_on_error when a Parquet file has no row groups.
......................................................................
IMPALA-3943: Adhere to abort_on_error when a Parquet file has no row groups.
Change-Id: I6aff766a1ce6376efb329bdde51c648149dfe08c
---
M be/src/exec/hdfs-parquet-scanner.cc
M testdata/data/README
A testdata/data/no_row_groups.parquet
A testdata/workloads/functional-query/queries/QueryTest/no-row-groups-abort.test
A testdata/workloads/functional-query/queries/QueryTest/no-row-groups.test
M tests/query_test/test_scanners.py
6 files changed, 41 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/62/3862/3
--
To view, visit http://gerrit.cloudera.org:8080/3862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6aff766a1ce6376efb329bdde51c648149dfe08c
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-3943: Adhere to abort on error when a Parquet file has no row groups.
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-3943: Adhere to abort_on_error when a Parquet file has no row groups.
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3862/3/be/src/exec/hdfs-parquet-scanner.cc
File be/src/exec/hdfs-parquet-scanner.cc:
Line 856: }
> I don't think we should treat it any differently. I originally had this che
I'm not opposed to handling _more_ errors like this, it just wasn't obvious to me that it would be what we wanted to do for _all_ errors that could be returned by ProcessFooter, especially since we wouldn't have test coverage for new behavior. I'm not opposed to it, just pointing it out. If others want to handle all the errors like this (i.e. v1), that's fine with me, but I think this is a good fix for now.
--
To view, visit http://gerrit.cloudera.org:8080/3862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6aff766a1ce6376efb329bdde51c648149dfe08c
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3943: Adhere to abort on error when a Parquet file has no row groups.
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-3943: Adhere to abort_on_error when a Parquet file has no row groups.
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3862/1/be/src/exec/hdfs-parquet-scanner.cc
File be/src/exec/hdfs-parquet-scanner.cc:
PS1, Line 178: if (!status.ok()) RETURN_IF_ERROR(state_->LogOrReturnError(status.msg()));
> It seems like if we hit an error here we don't want to continue any process
I manually tried this out with ProcessFooter() returning from various places and it all worked fine. I'm happy to make this error-to-warning more specific to the no-row-groups case.
--
To view, visit http://gerrit.cloudera.org:8080/3862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6aff766a1ce6376efb329bdde51c648149dfe08c
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3943: Adhere to abort on error in ProcessFooter().
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-3943: Adhere to abort_on_error in ProcessFooter().
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3862/5/be/src/exec/hdfs-parquet-scanner.cc
File be/src/exec/hdfs-parquet-scanner.cc:
PS5, Line 183: if (!status.ok()) return state_->LogOrReturnError(status.msg())
> CreateSchemaTree() is done in the schema resolver on L186 which we would no
Oops, I was looking at an older version of the code. The problem is still possible if e.g. file_metadata_.num_rows < 0.
As much as I dislike adding more state, I do think we need a flag. It's too fragile otherwise, especially with special case paths like IsZeroSlotTableScan() which won't care about the presence of row-groups.
--
To view, visit http://gerrit.cloudera.org:8080/3862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6aff766a1ce6376efb329bdde51c648149dfe08c
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3943: Adhere to abort on error in ProcessFooter().
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has abandoned this change.
Change subject: IMPALA-3943: Adhere to abort_on_error in ProcessFooter().
......................................................................
Abandoned
decided against this change
--
To view, visit http://gerrit.cloudera.org:8080/3862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: I6aff766a1ce6376efb329bdde51c648149dfe08c
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-3943: Adhere to abort on error when a Parquet file has no row groups.
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-3943: Adhere to abort_on_error when a Parquet file has no row groups.
......................................................................
Patch Set 1:
This also relates to IMPALA-3773
--
To view, visit http://gerrit.cloudera.org:8080/3862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6aff766a1ce6376efb329bdde51c648149dfe08c
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3943: Adhere to abort on error when a Parquet file has no row groups.
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-3943: Adhere to abort_on_error when a Parquet file has no row groups.
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3862/1/be/src/exec/hdfs-parquet-scanner.cc
File be/src/exec/hdfs-parquet-scanner.cc:
PS1, Line 178: if (!status.ok()) RETURN_IF_ERROR(state_->LogOrReturnError(status.msg()));
This means a number of other errors are also handled in this way now. Are we sure that's the behavior we want for other errors handled by ProcessFooter()? Having scanned that fn it seems reasonable, though I'd worry about not having test coverage for other bad metadata issues that would behave differently now, e.g. maybe now it's possible to induce a crash if we continue with certain kinds of bad files?
--
To view, visit http://gerrit.cloudera.org:8080/3862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6aff766a1ce6376efb329bdde51c648149dfe08c
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3943: Adhere to abort on error in ProcessFooter().
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-3943: Adhere to abort_on_error in ProcessFooter().
......................................................................
Patch Set 5:
(1 comment)
It does seem like this is not a regression at least in this code, but probably elsewhere, so I'm happy to abandon this if so desired.
It's still unclear to me why the perf runs started failing though. Those bad files have been there forever.
http://gerrit.cloudera.org:8080/#/c/3862/5/be/src/exec/hdfs-parquet-scanner.cc
File be/src/exec/hdfs-parquet-scanner.cc:
PS5, Line 183: if (!status.ok()) return state_->LogOrReturnError(status.msg())
> Right, this won't work as was discussed in the earlier comment.
I maintain this will work, and I did run through these paths on ASAN.
1. column_readers_ are set inside NextRowGroup() and are only accessed of there is a valid row group to scan
2. if ProcessFooter() exits before setting file_metadata_, then we have a file_metadata_ with 0 rows and no row groups, so we will cleanly exit out of GetNextInternal(), see L313 and L351, and never set or access the column readers
I added DCHECKs to show what state the file_metadata_ is in before calling into ProcessFooter().
There is still a discussion to be had on whether we should be able to continue on all of the errors in ProcessFooter(), e.g., if there's an unexpected file version.
If you'd prefer to add a special flag like open_ok for clarity that's fine, but not necessary.
Dan, the commit you are looking for is:
8343656189d631491b7372a60e228c851b61165c
--
To view, visit http://gerrit.cloudera.org:8080/3862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6aff766a1ce6376efb329bdde51c648149dfe08c
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3943: Adhere to abort on error when a Parquet file has no row groups.
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-3943: Adhere to abort_on_error when a Parquet file has no row groups.
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3862/1/be/src/exec/hdfs-parquet-scanner.cc
File be/src/exec/hdfs-parquet-scanner.cc:
PS1, Line 178: if (!status.ok()) RETURN_IF_ERROR(state_->LogOrReturnError(status.msg()));
> This means a number of other errors are also handled in this way now. Are w
It seems like if we hit an error here we don't want to continue any processing of the file. E.g. maybe we should just short-circuit GetNext() in this case.
--
To view, visit http://gerrit.cloudera.org:8080/3862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6aff766a1ce6376efb329bdde51c648149dfe08c
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3943: Adhere to abort on error when a Parquet file has no row groups.
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-3943: Adhere to abort_on_error when a Parquet file has no row groups.
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/3862/3/be/src/exec/hdfs-parquet-scanner.cc
File be/src/exec/hdfs-parquet-scanner.cc:
PS3, Line 854: Parqet
> typo
Done
Line 856: }
> seems okay, but why treat this case differently than the other invalid meta
I don't think we should treat it any differently. I originally had this check at the caller of ProcessFooter(). However, the concern was that the tests only cover this one case. I can move back to that solution and add more tests if you prefer. There was some urgency for this fix because it affected our TPCH nested perf runs, but I can fix the perf run separately (by deleting the bad files).
Let me know what you want to do.
--
To view, visit http://gerrit.cloudera.org:8080/3862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6aff766a1ce6376efb329bdde51c648149dfe08c
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3943: Adhere to abort on error when a Parquet file has no row groups.
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-3943: Adhere to abort_on_error when a Parquet file has no row groups.
......................................................................
Patch Set 2: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3862/2/be/src/exec/hdfs-parquet-scanner.cc
File be/src/exec/hdfs-parquet-scanner.cc:
Line 854: "Invalid file. This file: $0 has no row groups", filename()));
Mention that it's a Parquet file. I think we normally quote the filename with '' too.
--
To view, visit http://gerrit.cloudera.org:8080/3862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6aff766a1ce6376efb329bdde51c648149dfe08c
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3943: Adhere to abort on error in ProcessFooter().
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-3943: Adhere to abort_on_error in ProcessFooter().
......................................................................
Patch Set 7:
(1 comment)
Not this patch, but we should consider changing HdfsScanNode so that it handles the abort_on_error logic at the callsite for Open() if Open() fails. It might simplify this and the other scanners
http://gerrit.cloudera.org:8080/#/c/3862/7/be/src/exec/hdfs-parquet-scanner.cc
File be/src/exec/hdfs-parquet-scanner.cc:
Line 188: if (!status.ok()) return state_->LogOrReturnError(status.msg());
Don't we want to return early if there was any kind of error? If its a non-aborting error I think we want to return early with Status::OK(), since it's not clear if it's safe to execute the below code.
--
To view, visit http://gerrit.cloudera.org:8080/3862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6aff766a1ce6376efb329bdde51c648149dfe08c
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3943: Adhere to abort on error when a Parquet file has no row groups.
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-3943: Adhere to abort_on_error when a Parquet file has no row groups.
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3862/2/be/src/exec/hdfs-parquet-scanner.cc
File be/src/exec/hdfs-parquet-scanner.cc:
Line 854: "Invalid file. This file: $0 has no row groups", filename()));
> Mention that it's a Parquet file. I think we normally quote the filename wi
Done
--
To view, visit http://gerrit.cloudera.org:8080/3862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6aff766a1ce6376efb329bdde51c648149dfe08c
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3943: Adhere to abort on error in ProcessFooter().
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3862
to look at the new patch set (#6).
Change subject: IMPALA-3943: Adhere to abort_on_error in ProcessFooter().
......................................................................
IMPALA-3943: Adhere to abort_on_error in ProcessFooter().
This patch focuses on properly handling abort_on_error when ProcessFooter()
fails in the Parquet scanner.
It also refactors a few existing tests to cover both possible values
of abort_on_error.
There are still several places left where abort_on_error is not honored.
Testing: I ran test_scanners.py::TestParquet and test_stale_metadata.py
locally on exhaustive with debug and asan builds. I ran the data errors
tests in a loop for 20 iterations. I also did a private core/hdfs run.
Change-Id: I6aff766a1ce6376efb329bdde51c648149dfe08c
---
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-parquet-scanner.h
M testdata/data/README
A testdata/data/no_row_groups.parquet
M testdata/workloads/functional-query/queries/DataErrorsTest/avro-errors.test
M testdata/workloads/functional-query/queries/DataErrorsTest/hdfs-scan-node-errors.test
A testdata/workloads/functional-query/queries/QueryTest/no-row-groups-abort.test
A testdata/workloads/functional-query/queries/QueryTest/no-row-groups.test
M testdata/workloads/functional-query/queries/QueryTest/parquet-abort-on-error.test
M testdata/workloads/functional-query/queries/QueryTest/parquet-continue-on-error.test
M testdata/workloads/functional-query/queries/QueryTest/parquet.test
M tests/common/test_dimensions.py
M tests/common/test_result_verifier.py
M tests/metadata/test_stale_metadata.py
M tests/query_test/test_scanners.py
15 files changed, 180 insertions(+), 79 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/62/3862/6
--
To view, visit http://gerrit.cloudera.org:8080/3862
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6aff766a1ce6376efb329bdde51c648149dfe08c
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>