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 <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Tue, 13 Mar 2018 19:14:09 +0000 Gerrit-HasComments: Yes
