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 <fwij...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Mar 2018 19:14:09 +0000
Gerrit-HasComments: Yes

Reply via email to