Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/8869 )
Change subject: KUDU-2231: sparse column predicate can cause excessive data-block reads ...................................................................... Patch Set 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/8869/3/src/kudu/cfile/cfile_reader.cc File src/kudu/cfile/cfile_reader.cc: http://gerrit.cloudera.org:8080/#/c/8869/3/src/kudu/cfile/cfile_reader.cc@691 PS3, Line 691: it's odd that this code looks pretty much like the code in the trailer of this function, except that here we don't call SeekToPositionInBlock() but down there we do. Why is that not necessary? Could we structure this code so that the "trailer" code is shared, and we put the "new seek" code path in an "else" here? (ie lines 696-717 become the else clause) http://gerrit.cloudera.org:8080/#/c/8869/3/src/kudu/cfile/cfile_reader.cc@705 PS3, Line 705: // TODO: fast seek within block (without reseeking index) is this the optimization that you implemented here? http://gerrit.cloudera.org:8080/#/c/8869/3/src/kudu/tablet/tablet-pushdown-test.cc File src/kudu/tablet/tablet-pushdown-test.cc: http://gerrit.cloudera.org:8080/#/c/8869/3/src/kudu/tablet/tablet-pushdown-test.cc@259 PS3, Line 259: TEST_F(TabletSparsePushdownTest, Kudu2231) { mind adding a comment describing the data that gets inserted here? -- To view, visit http://gerrit.cloudera.org:8080/8869 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8eb3be4a809f882ccd80c48612099b2071306ff7 Gerrit-Change-Number: 8869 Gerrit-PatchSet: 3 Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Tue, 19 Dec 2017 02:49:50 +0000 Gerrit-HasComments: Yes
