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 4: (6 comments) > - At this point every kind of COMPUTE STATS update > impala.lastComputeStatsTime, including INCREMENTAL, TABLESAMPLE, > and calls that only COMPUTE STATS for specific columns. I can > exclude some of these, but I think that the current behavior should > cause the "least surprise". That seems reasonable to me, but please check with Alex. > - I think that the tests should be reorganized, but I would like to > finalize the behavior before doing that. Yes. :) http://gerrit.cloudera.org:8080/#/c/10116/4/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/4/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@247 PS4, Line 247: msClient.alter_table(msTable_.getDbName(), nit: single line? 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@2857 PS3, Line 2857: * command if the metadata is not completely in-sync. This affects both Hive and > Is the missing field explanation really necessary? I think that the current lgtm http://gerrit.cloudera.org:8080/#/c/10116/4/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/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@782 PS4, Line 782: if (params.isSetTable_stats()) updateTableStats(params, msTbl); : : if (params.isSetTable_stats()) { This could be one check now http://gerrit.cloudera.org:8080/#/c/10116/4/tests/metadata/test_last_ddl_time_update.py File tests/metadata/test_last_ddl_time_update.py: http://gerrit.cloudera.org:8080/#/c/10116/4/tests/metadata/test_last_ddl_time_update.py@24 PS4, Line 24: # Checks different statements effect the last DDL time and last compute stats time. nit: Checks that... http://gerrit.cloudera.org:8080/#/c/10116/4/tests/metadata/test_last_ddl_time_update.py@50 PS4, Line 50: # compute statistics the fill table property impala.lastComputeStatsTime nit: "to fill the.."? http://gerrit.cloudera.org:8080/#/c/10116/4/tests/metadata/test_last_ddl_time_update.py@167 PS4, Line 167: False, False Can you think of ways to make these calls more readable? We could have to constants, e.g. DDL=1 and STATS=2 and then pass a list of changed values. These would then read self.run_test("bla", unique_database, TBL_NAME, [DDL, STATS]) There might be better ways I cannot think of right now. -- 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: 4 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: Fri, 27 Apr 2018 19:46:15 +0000 Gerrit-HasComments: Yes
