Yongzhi Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/14037 )
Change subject: IMPALA-8839: Remove COLUMN_STATS_ACCURATE from properties ...................................................................... Patch Set 4: (15 comments) Patch 4 addresses the review. http://gerrit.cloudera.org:8080/#/c/14037/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14037/3//COMMIT_MSG@10 PS3, Line 10: stored statistics accurate. A > Can you add some comments here (and probably in the code too) about why thi Done http://gerrit.cloudera.org:8080/#/c/14037/3//COMMIT_MSG@14 PS3, Line 14: The patch impletes: > I needed a bit of time to think about what "stats change" means here - I th Done http://gerrit.cloudera.org:8080/#/c/14037/3/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java File fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java: http://gerrit.cloudera.org:8080/#/c/14037/3/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@110 PS3, Line 110: : /** > These arguments could be taken from tbl Done http://gerrit.cloudera.org:8080/#/c/14037/3/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@144 PS3, Line 144: reClient.alter_parti > Needing a writeIdList here seems weird, as I cannot express what those writ I am not sure if it can be called a hack, a correct value can make the alter table transaction commit, the wrong one can fail the transaction. http://gerrit.cloudera.org:8080/#/c/14037/3/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@460 PS3, Line 460: return client.getValidWriteIds(tableFullName); > name: writeID suggests a single number to me. I would prefer getValidWriteI Done http://gerrit.cloudera.org:8080/#/c/14037/3/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@462 PS3, Line 462: > The pattern in this file seems to be to catch the Hive/thrift related excep This private method which is used by alterTable and alterPartition methods in the file all throw the three exceptions. For it is helper methods for them, follow the same pattern is easier. http://gerrit.cloudera.org:8080/#/c/14037/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/14037/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3711 PS3, Line 3711: if (!catalog_.tryLockTable(table)) { > nit: extra line Done http://gerrit.cloudera.org:8080/#/c/14037/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3762 PS3, Line 3762: table.getDb().getName(), table.getName()); : } : } > Do we need to allocate a new writeId here, or the writeId used for the inse >From my test, it seems the same value. How to get Insert statement's writeId? >the Write ID in table is not valid here. http://gerrit.cloudera.org:8080/#/c/14037/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3770 PS3, Line 3770: ttempt to rem > I think that errorMessages was created specifically for HDFS caching relate Done http://gerrit.cloudera.org:8080/#/c/14037/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3890 PS3, Line 3890: > optional, refactor: Moving the next block to a function like UnsetPartition Done http://gerrit.cloudera.org:8080/#/c/14037/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3942 PS3, Line 3942: * > nit: extra line Done http://gerrit.cloudera.org:8080/#/c/14037/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@4314 PS3, Line 4314: long writeId, long txnId) throws ImpalaRuntimeException{ > nit: exists Done http://gerrit.cloudera.org:8080/#/c/14037/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@4316 PS3, Line 4316: > consistency: lower U Done http://gerrit.cloudera.org:8080/#/c/14037/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@4321 PS3, Line 4321: > I think that this needs some explanation, see https://issues.apache.org/jir Done http://gerrit.cloudera.org:8080/#/c/14037/3/tests/query_test/test_acid.py File tests/query_test/test_acid.py: http://gerrit.cloudera.org:8080/#/c/14037/3/tests/query_test/test_acid.py@104 PS3, Line 104: pIfABFS.hive > optional, code size reduction: the two .test files seem very similar, the o I have to use hive to create tables to make the test work for I need hive to compute stats(to verify the correctness of the change). What I found in .test file, when hive queries follow impala queries, it can run before impala query, so some table create by impala invisible to hive. -- To view, visit http://gerrit.cloudera.org:8080/14037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I13f4a77022a7112e10a07314359f927eae083deb Gerrit-Change-Number: 14037 Gerrit-PatchSet: 4 Gerrit-Owner: Yongzhi Chen <yc...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com> Gerrit-Comment-Date: Fri, 09 Aug 2019 22:22:21 +0000 Gerrit-HasComments: Yes
