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

Reply via email to