Dan Burkert 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 4: (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: return Status::NotSupported("no positional index in file"); > it's odd that this code looks pretty much like the code in the trailer of t Done. You are right that SeekToPositionInBlock should be called in both cases. I looked into why it didn't break here, and it turns out you can entirely remove both calls and all tests still pass. It seems that Scan will call SeekToOrdinal based on the needs_rewind_ field, and that appears to always work. I admit I don't fully understand what's going on here, but I think it's safe to call in both cases. http://gerrit.cloudera.org:8080/#/c/8869/3/src/kudu/cfile/cfile_reader.cc@705 PS3, Line 705: // block in the file. > is this the optimization that you implemented here? Not sure, you wrote that comment in 2012? Commit #14. I can remove it if you think it's not needed anymore. 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: // The is a regression test for KUDU-2231, which fixed a CFileReader bug causing > mind adding a comment describing the data that gets inserted here? Done -- 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: 4 Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: 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: Wed, 20 Dec 2017 23:38:01 +0000 Gerrit-HasComments: Yes
