Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/17252 )
Change subject: IMPALA-10647 Improve always-true min/max filter handling in coordinator ...................................................................... Patch Set 17: (5 comments) http://gerrit.cloudera.org:8080/#/c/17252/15//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17252/15//COMMIT_MSG@29 PS15, Line 29: row group is processed. By comparing these two timestamps, one : can easily diagnose issues related to late arrival of min/max : filters. > Consider adding these three tests for Final table as its content is sending The test added in Patch set 17 looks good to me, thanks! http://gerrit.cloudera.org:8080/#/c/17252/17/be/src/runtime/coordinator-filter-state.h File be/src/runtime/coordinator-filter-state.h: http://gerrit.cloudera.org:8080/#/c/17252/17/be/src/runtime/coordinator-filter-state.h@178 PS17, Line 178: True value means the always false flag in aggregated filter is flipped. To further clarify the doc, please mention that a True value means the filter was flipped from True to False by coordinator. http://gerrit.cloudera.org:8080/#/c/17252/15/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/17252/15/be/src/runtime/coordinator.cc@659 PS15, Line 659: > If AlwaysTrueFilterReceived() is true, then the accumulated filter is logic New logic looks good to me. I'll continue my comments on patch set 17. http://gerrit.cloudera.org:8080/#/c/17252/17/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/17252/17/be/src/runtime/coordinator.cc@668 PS17, Line 668: state.AlwaysFalseFlipped() The method name sounds ambiguous. Was it flipped from true to false, or false to true. Seems like the former. The method return true IF filter was an AlwaysFalse before being disabled (flipped to an AlwaysTrue) by coordinator. Maybe "WasAlwaysFalse" is more descriptive? http://gerrit.cloudera.org:8080/#/c/17252/17/be/src/util/min-max-filter.h File be/src/util/min-max-filter.h: http://gerrit.cloudera.org:8080/#/c/17252/17/be/src/util/min-max-filter.h@358 PS17, Line 358: : std::string DebugString(const MinMaxFilterPB& filter); : bool AlwaysTrue(const MinMaxFilterPB& filter); : bool AlwaysFalse(const MinMaxFilterPB& filter); : std::string DebugString(const ColumnValuePB& value); Any reason not making these methods as static methods under MinMaxFilter class? -- 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: 17 Gerrit-Owner: Qifan Chen <qc...@cloudera.com> Gerrit-Reviewer: Fang-Yu Rao <fangyu....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com> Gerrit-Comment-Date: Mon, 12 Apr 2021 18:59:06 +0000 Gerrit-HasComments: Yes
