Shant Hovsepian has posted comments on this change. ( http://gerrit.cloudera.org:8080/16723 )
Change subject: IMPALA-10314: Optimize planning time for simple limits ...................................................................... Patch Set 11: (4 comments) Just a couple corner cases I have run into; given this is an opt-in optimization now it might not be incorrect to ignore these. I think it's good to think about the case where this optimization helps and not risk an incorrect limit in other cases. Where this helps most. a. lots of files b. small limits a) the scan range and scheduling overhead is only slow when there are many hosts + files. b) for large limits maybe the bulk of query run time goes to fetching results and not the planning, but that said it may not hurt too much in this case. http://gerrit.cloudera.org:8080/#/c/16723/11/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java File fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java: http://gerrit.cloudera.org:8080/#/c/16723/11/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@854 PS11, Line 854: FileSystem partitionFs; I'd consider only doing this optimization for "record oriented" or "splittable" file formats, like parquet/avro. For text tables it's not uncommon for parsing or record boundary issues to cause an entire file to be invalid. http://gerrit.cloudera.org:8080/#/c/16723/11/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@867 PS11, Line 867: for (FileDescriptor fd : partition.getFileDescriptors()) { Also maybe a threshold on the number of scan ranges where we wouldn't bother with the optimization, be/src/benchmarks/scheduler-benchmark is a good test to run, if I recall tens of nodes and hundreds of files isn't too slow. Just thinking that the cases where this optimization assumption is potentially problematic are when there are a few malformed files better to err on the side of caution then. http://gerrit.cloudera.org:8080/#/c/16723/11/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@873 PS11, Line 873: simpleLimitNumRows++; // conservatively estimate 1 row per file The flip side of this might be for simple limits that are relatively large or larger than the number of files a maximum bail out threshold might make sense. I.e. if the limit is 10,000 then no sense in doing this optimization unless the number of files is much greater than 10,000? http://gerrit.cloudera.org:8080/#/c/16723/11/fe/src/test/java/org/apache/impala/planner/PlannerTest.java File fe/src/test/java/org/apache/impala/planner/PlannerTest.java: http://gerrit.cloudera.org:8080/#/c/16723/11/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@1121 PS11, Line 1121: For Hive ACID and deleted records would this logic still work? Might be a helpful test. -- To view, visit http://gerrit.cloudera.org:8080/16723 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9d6a79263bc092e0f3e9a1d72da5618f3cc35574 Gerrit-Change-Number: 16723 Gerrit-PatchSet: 11 Gerrit-Owner: Aman Sinha <amsi...@cloudera.com> Gerrit-Reviewer: Aman Sinha <amsi...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com> Gerrit-Reviewer: Shant Hovsepian <sh...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Mon, 23 Nov 2020 21:32:08 +0000 Gerrit-HasComments: Yes
