Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/11650 )
Change subject: IMPALA-7272: Fix crash in StringMinMaxFilter ...................................................................... Patch Set 3: Code-Review+2 (3 comments) http://gerrit.cloudera.org:8080/#/c/11650/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11650/3//COMMIT_MSG@29 PS3, Line 29: : MemPool::Allocate. Can you please consider adding this query as a test as a regression test ? Although it may not be reproducible as standalone, it will still exercise the path in question. Since it's also timing dependent, it may be more readily reproducible in our regular testing environment where we run multiple queries concurrently. http://gerrit.cloudera.org:8080/#/c/11650/3/be/src/util/min-max-filter-test.cc File be/src/util/min-max-filter-test.cc: http://gerrit.cloudera.org:8080/#/c/11650/3/be/src/util/min-max-filter-test.cc@355 PS3, Line 355: EXPECT_EQ(TimestampValue::FromTColumnValue(tFilter2.max), t3); Should we call Close() on the filters above too ? http://gerrit.cloudera.org:8080/#/c/11650/3/be/src/util/min-max-filter.h File be/src/util/min-max-filter.h: http://gerrit.cloudera.org:8080/#/c/11650/3/be/src/util/min-max-filter.h@188 PS3, Line 188: MemPool mem_pool_; As mentioned previously, please add a comment for this. -- To view, visit http://gerrit.cloudera.org:8080/11650 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I751cad7e6b75c9d95d7ad029bbd1e52fe09e8a29 Gerrit-Change-Number: 11650 Gerrit-PatchSet: 3 Gerrit-Owner: Thomas Marshall <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Thomas Marshall <[email protected]> Gerrit-Comment-Date: Thu, 18 Oct 2018 02:34:46 +0000 Gerrit-HasComments: Yes
