Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/14066 )
Change subject: IMPALA-8836: Support COMPUTE STATS on insert only ACID tables ...................................................................... Patch Set 7: (9 comments) http://gerrit.cloudera.org:8080/#/c/14066/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14066/7//COMMIT_MSG@9 PS7, Line 9: For ACID tables COMPUTE STATS needs to use a new HMS API, as the > You could also mention the inter-operability aspects with Hive, e.g. will H I added a paragraph to the additional change related to COLUMN_STATS_ACCURATE. I don't think that there are any other changes. http://gerrit.cloudera.org:8080/#/c/14066/7//COMMIT_MSG@32 PS7, Line 32: l > nit: typo Done http://gerrit.cloudera.org:8080/#/c/14066/7/fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java File fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java: http://gerrit.cloudera.org:8080/#/c/14066/7/fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java@79 PS7, Line 79: TblTransaction > nit: it's a bit weird that Tbl is abbreviated while Transaction isn't My reasoning was that Tbl is self-evident in Impala context, while Txn is only self-evident for ACID people. I am not happy about the name, but I plan to redesign the class anyway. http://gerrit.cloudera.org:8080/#/c/14066/7/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/14066/7/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@238 PS7, Line 238: for (Partition part: partitions) { > nit: missing space Done http://gerrit.cloudera.org:8080/#/c/14066/7/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/14066/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@948 PS7, Line 948: // TODO: Transaction committing / aborting seems weird for stat update, but I don't : // see other ways to get a new write id (which is needed to update : // transactional tables). Hive seems to use internal API for this. > nit: so what should we do here? Is there a Jira here to track? Done http://gerrit.cloudera.org:8080/#/c/14066/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@956 PS7, Line 956: -1 /* opens new transaction */ > nit: The interface is confusing. Later we should cut createTblTransaction() I agree, but I would do this later when the whole TblTransaction thing is redesigned. http://gerrit.cloudera.org:8080/#/c/14066/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3391 PS7, Line 3391: // > Should we remove this line and the comment above? Thanks for catching this, this was commented out as an experiment and I forgot to undo it. http://gerrit.cloudera.org:8080/#/c/14066/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3575 PS7, Line 3575: > nit too much spaces Done http://gerrit.cloudera.org:8080/#/c/14066/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3579 PS7, Line 3579: > nit: too much spaces Done -- To view, visit http://gerrit.cloudera.org:8080/14066 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5c06b4678c1ff75c5aa1586a78afea563e64057f Gerrit-Change-Number: 14066 Gerrit-PatchSet: 7 Gerrit-Owner: Csaba Ringhofer <csringho...@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-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Fri, 16 Aug 2019 17:06:24 +0000 Gerrit-HasComments: Yes
