Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/17252 )
Change subject: IMPALA-10532: TestOverlapMinMaxFilters.test_overlap_min_max_filters seems flaky ...................................................................... Patch Set 10: (3 comments) http://gerrit.cloudera.org:8080/#/c/17252/10//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17252/10//COMMIT_MSG@7 PS10, Line 7: IMPALA-10532: TestOverlapMinMaxFilters.test_overlap_min_max_filters seems flaky In my opinion, we should split this into 2 separate changes. One is focus on fixing the test_overlap_min_max_filters, and the other is for the query profile improvement. http://gerrit.cloudera.org:8080/#/c/17252/10/be/src/exec/scan-node.cc File be/src/exec/scan-node.cc: http://gerrit.cloudera.org:8080/#/c/17252/10/be/src/exec/scan-node.cc@236 PS10, Line 236: Current " : "time(ms): $2", I think this is redundant and potentially confuse users who try to debug query profile. The information from wait_time (Maximum arrival delay) should be sufficient. As far as I know, MonotonicMilis mostly used to calculate time interval, not to represent system time. Besides, filter arrival time is already displayed under host profile like this: impala-executor-000-1:27000: Filter 16 arrival: 662ms Filter 14 arrival: 10s454ms Filter 6 arrival: 10s514ms Filter 8 arrival: 5m46s Filter 9 arrival: 5m46s Filter 10 arrival: 5m46s Filter 0 arrival: 7m29s Filter 1 arrival: 7m29s Filter 2 arrival: 7m29s http://gerrit.cloudera.org:8080/#/c/17252/10/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/17252/10/be/src/runtime/coordinator.cc@1614 PS10, Line 1614: minmaxDisabled_ = true; I think we should maintain the same behavior as bloom_filter_ in here, making the logic consistent for both filter type. I understand that this change is made to distinguish between 1) an actual AlwaysTrue filter that originate from executor, vs 2) AlwaysTrue filter that comes from disabling by coordinator. Instead, what if we have a new boolean variable to track 1). Say, a boolean variable 'always_true_minmax_received_'. Coordinator::FilterDebugString() then should refer this boolean variable instead of checking minmax_filterPB.always_true(). Similarly also for AlwaysFalse filter arrival. -- To view, visit http://gerrit.cloudera.org:8080/17252 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I326317833979efcbe02ce6c95ad80133dd5c7964 Gerrit-Change-Number: 17252 Gerrit-PatchSet: 10 Gerrit-Owner: Qifan Chen <qc...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Comment-Date: Thu, 08 Apr 2021 16:25:43 +0000 Gerrit-HasComments: Yes
