Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/10116 )
Change subject: IMPALA-6131: Track time of last statistics update in metadata ...................................................................... Patch Set 3: (11 comments) http://gerrit.cloudera.org:8080/#/c/10116/3/fe/src/main/java/org/apache/impala/catalog/KuduTable.java File fe/src/main/java/org/apache/impala/catalog/KuduTable.java: http://gerrit.cloudera.org:8080/#/c/10116/3/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@240 PS3, Line 240: // HMS updates transient_lastDdlTime if it is removed. Can you explain in a comment why we need to clone the table instead of removing the key from msTable_? http://gerrit.cloudera.org:8080/#/c/10116/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/10116/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@728 PS3, Line 728: * and 'numUpdatedColumns', respectively. This comment should point out that it updates impala.lastComputeStatsTime, and that it only does so when running compute stats. http://gerrit.cloudera.org:8080/#/c/10116/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@782 PS3, Line 782: && Why do we only update this if both are set? Why not if we're just updating one? http://gerrit.cloudera.org:8080/#/c/10116/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2857 PS3, Line 2857: * longer period of time. Explain the bool parameter in the comment including what value the lastDdlTime is set to if it's set to true. Since we already describe the HMS API we could also mention what happens with missing fields (they get removed). http://gerrit.cloudera.org:8080/#/c/10116/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2862 PS3, Line 2862: long lastDdlTime = -1; unused? http://gerrit.cloudera.org:8080/#/c/10116/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3221 PS3, Line 3221: * Updates the lastDdlTime of the table if new partitions were created. Assuming this line is still correct, can you add a comment where this happens? e.g. "This call also updates lastDdlTime" http://gerrit.cloudera.org:8080/#/c/10116/3/tests/metadata/test_last_ddl_time_update.py File tests/metadata/test_last_ddl_time_update.py: http://gerrit.cloudera.org:8080/#/c/10116/3/tests/metadata/test_last_ddl_time_update.py@50 PS3, Line 50: # initialize last compute stats time This should be more verbose, explain why it is necessary to initialize it. http://gerrit.cloudera.org:8080/#/c/10116/3/tests/metadata/test_last_ddl_time_update.py@75 PS3, Line 75: self.run_test("compute stats %s" % FQ_TBL_NAME, We should also have a test for computing incremental stats, for sampled stats, and for manually updating both table and column stats. http://gerrit.cloudera.org:8080/#/c/10116/3/tests/metadata/test_last_ddl_time_update.py@145 PS3, Line 145: expect_changed_ddl_time, expect_changed_stat_time = False): I think we should have defaults for both expect_* parameters here, or for none. http://gerrit.cloudera.org:8080/#/c/10116/3/tests/metadata/test_last_ddl_time_update.py@161 PS3, Line 161: beforeStatTime = table.parameters[LAST_COMPUTE_STATS_TIME_KEY] nit: beforeStatsTime http://gerrit.cloudera.org:8080/#/c/10116/3/tests/metadata/test_last_ddl_time_update.py@180 PS3, Line 180: if expect_changed_stat_time: stats_time, given the key is lastComputeStatsTime, too. -- To view, visit http://gerrit.cloudera.org:8080/10116 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I59a671ac29d352bd92ce40d5cb6662bb23f146b5 Gerrit-Change-Number: 10116 Gerrit-PatchSet: 3 Gerrit-Owner: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Thu, 19 Apr 2018 21:08:16 +0000 Gerrit-HasComments: Yes
