You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Fredy Wijaya (Code Review)" <ge...@cloudera.org> on 2018/03/09 05:24:12 UTC
[Impala-ASF-CR] IMPALA-6619: Incorrect partitions in RECOVER PARTITIONS
Fredy Wijaya has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9564
Change subject: IMPALA-6619: Incorrect partitions in RECOVER PARTITIONS
......................................................................
IMPALA-6619: Incorrect partitions in RECOVER PARTITIONS
ALTER TABLE RECOVER PARTITIONS creates unncessary partitions when
dealing with URL encoded partition value.
Testing:
- Ran end-to-end recover partitions tests
Change-Id: If45d78d095a5d3e85ebf61722566136d17825559
---
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M tests/metadata/test_recover_partitions.py
2 files changed, 40 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/64/9564/1
--
To view, visit http://gerrit.cloudera.org:8080/9564
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: If45d78d095a5d3e85ebf61722566136d17825559
Gerrit-Change-Number: 9564
Gerrit-PatchSet: 1
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
[Impala-ASF-CR] IMPALA-6619: Incorrect partitions in RECOVER PARTITIONS
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/9564 )
Change subject: IMPALA-6619: Incorrect partitions in RECOVER PARTITIONS
......................................................................
IMPALA-6619: Incorrect partitions in RECOVER PARTITIONS
ALTER TABLE RECOVER PARTITIONS creates unncessary partitions when
dealing with URL encoded partition value.
Testing:
- Ran end-to-end recover partitions tests
Change-Id: If45d78d095a5d3e85ebf61722566136d17825559
---
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M tests/metadata/test_recover_partitions.py
2 files changed, 45 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/64/9564/3
--
To view, visit http://gerrit.cloudera.org:8080/9564
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If45d78d095a5d3e85ebf61722566136d17825559
Gerrit-Change-Number: 9564
Gerrit-PatchSet: 3
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-6619: Incorrect partitions in RECOVER PARTITIONS
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9564 )
Change subject: IMPALA-6619: Incorrect partitions in RECOVER PARTITIONS
......................................................................
Patch Set 5:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2095/
--
To view, visit http://gerrit.cloudera.org:8080/9564
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If45d78d095a5d3e85ebf61722566136d17825559
Gerrit-Change-Number: 9564
Gerrit-PatchSet: 5
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Mar 2018 22:18:28 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6619: Incorrect partitions in RECOVER PARTITIONS
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9564 )
Change subject: IMPALA-6619: Incorrect partitions in RECOVER PARTITIONS
......................................................................
Patch Set 5: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/9564
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If45d78d095a5d3e85ebf61722566136d17825559
Gerrit-Change-Number: 9564
Gerrit-PatchSet: 5
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 14 Mar 2018 01:59:11 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6619: Incorrect partitions in RECOVER PARTITIONS
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/9564 )
Change subject: IMPALA-6619: Incorrect partitions in RECOVER PARTITIONS
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/9564/3/tests/metadata/test_recover_partitions.py
File tests/metadata/test_recover_partitions.py:
http://gerrit.cloudera.org:8080/#/c/9564/3/tests/metadata/test_recover_partitions.py@343
PS3, Line 343: @SkipIfLocal.hdfs_client
Which part doesn't work on the local FS?
http://gerrit.cloudera.org:8080/#/c/9564/3/tests/metadata/test_recover_partitions.py@399
PS3, Line 399: return self.count_value('hdfs://', lines)
This won't work on s3 because there is no "hdfs://"
Take a look at filesystem_utils.py FILESYSTEM_PREFIX
--
To view, visit http://gerrit.cloudera.org:8080/9564
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If45d78d095a5d3e85ebf61722566136d17825559
Gerrit-Change-Number: 9564
Gerrit-PatchSet: 3
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Mar 2018 21:21:06 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6619: Incorrect partitions in RECOVER PARTITIONS
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/9564 )
Change subject: IMPALA-6619: Incorrect partitions in RECOVER PARTITIONS
......................................................................
Patch Set 5: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9564/3/tests/metadata/test_recover_partitions.py
File tests/metadata/test_recover_partitions.py:
http://gerrit.cloudera.org:8080/#/c/9564/3/tests/metadata/test_recover_partitions.py@343
PS3, Line 343: @SkipIfLocal.hdfs_client
> I'm just following all other test methods here. According to https://github
My understanding is that @SkipIfLocal.hive means Hive's HiveServer2 is not started so running queries against Hive does not work. The Hive Metastore is certainly started, otherwise most tests would be pointless (nothing works without the Hive Metastore).
My understanding of @SkipIfLocal.hdfs_client is that hdfs_client python will not work. However, none of those calls are used here.
Basically, I think that several of the SkipIfLocal in this file might be wrong.
I'm ok with checking this in as is, but can you please investigate this further and file a JIRA to clean this up if necessary? Thanks.
--
To view, visit http://gerrit.cloudera.org:8080/9564
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If45d78d095a5d3e85ebf61722566136d17825559
Gerrit-Change-Number: 9564
Gerrit-PatchSet: 5
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Mar 2018 22:16:58 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6619: Incorrect partitions in RECOVER PARTITIONS
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9564 )
Change subject: IMPALA-6619: Incorrect partitions in RECOVER PARTITIONS
......................................................................
IMPALA-6619: Incorrect partitions in RECOVER PARTITIONS
ALTER TABLE RECOVER PARTITIONS creates unncessary partitions when
dealing with URL encoded partition value.
Testing:
- Ran end-to-end recover partitions tests
Change-Id: If45d78d095a5d3e85ebf61722566136d17825559
Reviewed-on: http://gerrit.cloudera.org:8080/9564
Reviewed-by: Fredy Wijaya <fw...@cloudera.com>
Reviewed-by: Alex Behm <al...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M tests/metadata/test_recover_partitions.py
2 files changed, 45 insertions(+), 6 deletions(-)
Approvals:
Fredy Wijaya: Looks good to me, but someone else must approve
Alex Behm: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/9564
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: If45d78d095a5d3e85ebf61722566136d17825559
Gerrit-Change-Number: 9564
Gerrit-PatchSet: 6
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-6619: Incorrect partitions in RECOVER PARTITIONS
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/9564 )
Change subject: IMPALA-6619: Incorrect partitions in RECOVER PARTITIONS
......................................................................
IMPALA-6619: Incorrect partitions in RECOVER PARTITIONS
ALTER TABLE RECOVER PARTITIONS creates unncessary partitions when
dealing with URL encoded partition value.
Testing:
- Ran end-to-end recover partitions tests
Change-Id: If45d78d095a5d3e85ebf61722566136d17825559
---
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M tests/metadata/test_recover_partitions.py
2 files changed, 45 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/64/9564/2
--
To view, visit http://gerrit.cloudera.org:8080/9564
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If45d78d095a5d3e85ebf61722566136d17825559
Gerrit-Change-Number: 9564
Gerrit-PatchSet: 2
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-6619: Incorrect partitions in RECOVER PARTITIONS
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/9564 )
Change subject: IMPALA-6619: Incorrect partitions in RECOVER PARTITIONS
......................................................................
Patch Set 2: Code-Review+1
(2 comments)
http://gerrit.cloudera.org:8080/#/c/9564/2/tests/metadata/test_recover_partitions.py
File tests/metadata/test_recover_partitions.py:
http://gerrit.cloudera.org:8080/#/c/9564/2/tests/metadata/test_recover_partitions.py@346
PS2, Line 346: partition value
nit: partition path
(the value is not encoded, right?)
http://gerrit.cloudera.org:8080/#/c/9564/2/tests/metadata/test_recover_partitions.py@356
PS2, Line 356: # only produce a single partition.
nit: ... when adding a single partition.
--
To view, visit http://gerrit.cloudera.org:8080/9564
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If45d78d095a5d3e85ebf61722566136d17825559
Gerrit-Change-Number: 9564
Gerrit-PatchSet: 2
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Mar 2018 20:39:56 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6619: Incorrect partitions in RECOVER PARTITIONS
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/9564 )
Change subject: IMPALA-6619: Incorrect partitions in RECOVER PARTITIONS
......................................................................
IMPALA-6619: Incorrect partitions in RECOVER PARTITIONS
ALTER TABLE RECOVER PARTITIONS creates unncessary partitions when
dealing with URL encoded partition value.
Testing:
- Ran end-to-end recover partitions tests
Change-Id: If45d78d095a5d3e85ebf61722566136d17825559
---
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M tests/metadata/test_recover_partitions.py
2 files changed, 45 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/64/9564/5
--
To view, visit http://gerrit.cloudera.org:8080/9564
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If45d78d095a5d3e85ebf61722566136d17825559
Gerrit-Change-Number: 9564
Gerrit-PatchSet: 5
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-6619: Incorrect partitions in RECOVER PARTITIONS
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/9564 )
Change subject: IMPALA-6619: Incorrect partitions in RECOVER PARTITIONS
......................................................................
Patch Set 1:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/9564/1/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java:
http://gerrit.cloudera.org:8080/#/c/9564/1/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@1896
PS1, Line 1896: URLDecoder.decode(
: keyValues.first, StandardCharsets.UTF_8.name())
should this be pushed into getTypeCompatibleValues? Any idea why path.getName(), which use URI underneath is not already decoding?
http://gerrit.cloudera.org:8080/#/c/9564/1/tests/metadata/test_recover_partitions.py
File tests/metadata/test_recover_partitions.py:
http://gerrit.cloudera.org:8080/#/c/9564/1/tests/metadata/test_recover_partitions.py@357
PS1, Line 357: result.data
I'd prefer in this case that the tests explicitly state what's expected: 1 line with the listed partition. Current assertion checks that the expected partition exists, but result could have more partitions, which is incorrect, yet untested.
http://gerrit.cloudera.org:8080/#/c/9564/1/tests/metadata/test_recover_partitions.py@372
PS1, Line 372: "ALTER TABLE %s RECOVER PARTITIONS failed to handle encoded partitioned value")
why not do this in a loop? that would make it clearer that you expect multiple alter table partition stmts to not change the number of partitions.
http://gerrit.cloudera.org:8080/#/c/9564/1/tests/metadata/test_recover_partitions.py@373
PS1, Line 373:
perhaps add a test that expects that given a single new partition, alter table partition results in only one additional partition?
http://gerrit.cloudera.org:8080/#/c/9564/1/tests/metadata/test_recover_partitions.py@402
PS1, Line 402: count_value(
if this is not needed, remove it.
--
To view, visit http://gerrit.cloudera.org:8080/9564
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If45d78d095a5d3e85ebf61722566136d17825559
Gerrit-Change-Number: 9564
Gerrit-PatchSet: 1
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Mar 2018 19:14:09 +0000
Gerrit-HasComments: Yes