Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/18929 )
Change subject: IMPALA-11539: Mitigate intra-node skew of file scans with MT_DOP ...................................................................... Patch Set 3: (14 comments) Thanks for the comments! http://gerrit.cloudera.org:8080/#/c/18929/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18929/3//COMMIT_MSG@13 PS3, Line 13: for HDFS > not just HDFS, right? do we also use the same logic for files in cloud stor Yeah, I just copied the title here, and also used HDFS scans in the title of this CR. Though I see that it can be confusing so I changed it to 'file scans'. http://gerrit.cloudera.org:8080/#/c/18929/3//COMMIT_MSG@16 PS3, Line 16: thisqueue. > typo Done http://gerrit.cloudera.org:8080/#/c/18929/3//COMMIT_MSG@25 PS3, Line 25: Ranges that are marked to use the hdfs cache are still handled with : higher priority. > Probably out of scope for this patch, but data cache / file handle cache co File handle cache: it would require a more invasive change plus testing, so might want to deal with it in another patch. Remote data cache: we have the file splits here, not the actual ranges that we need to read, i.e. it's not easy to tell if the data is cached or not. We can only assume that every file split scheduled here is potentially cached. About scans that doesn't fit the data cache: maybe we could use a different scheduling policy. I guess it causes a problem when the same query is executed repeatedly. It can harm benchmark results, but I don't know if it actually happens in real-world use cases. Again, we see the file splits here, and if only a subset of the columns are read then it's hard to tell the actual size of the scans. http://gerrit.cloudera.org:8080/#/c/18929/3/be/src/exec/scan-range-queue-mt.h File be/src/exec/scan-range-queue-mt.h: http://gerrit.cloudera.org:8080/#/c/18929/3/be/src/exec/scan-range-queue-mt.h@30 PS3, Line 30: Only used for MT scans where the scan ranges are dynamically assigned : /// to the fragment instances using this queue. > Why is it treated differently than the MT_DOP=0 case? I would assume that s The MT_DOP=0 uses different methods and I didn't want to change it in this CR as it would require a big refactor. Maybe we can unify the two in the future. http://gerrit.cloudera.org:8080/#/c/18929/3/be/src/exec/scan-range-queue-mt.h@39 PS3, Line 39: at_front > typo: high_prio Done http://gerrit.cloudera.org:8080/#/c/18929/3/be/src/exec/scan-range-queue-mt.h@75 PS3, Line 75: scan_range_queue_.Reserve(capacity); > This should also take the lock IMO, as reserve can cause the underlying vec Done http://gerrit.cloudera.org:8080/#/c/18929/3/be/src/exec/scan-range-queue-mt.h@83 PS3, Line 83: if (!lhs->UseHdfsCache() && rhs->UseHdfsCache()) return true; > Shouldn't we return false in the opposite case? Good catch, done. http://gerrit.cloudera.org:8080/#/c/18929/1/tests/query_test/test_scanners.py File tests/query_test/test_scanners.py: http://gerrit.cloudera.org:8080/#/c/18929/1/tests/query_test/test_scanners.py@368 PS1, Line 368: > flake8: E302 expected 2 blank lines, found 1 Done http://gerrit.cloudera.org:8080/#/c/18929/1/tests/query_test/test_scanners.py@387 PS1, Line 387: ( > flake8: E741 ambiguous variable name 'l' Done http://gerrit.cloudera.org:8080/#/c/18929/1/tests/query_test/test_scanners.py@388 PS1, Line 388: f > flake8: E261 at least two spaces before inline comment Done http://gerrit.cloudera.org:8080/#/c/18929/1/tests/query_test/test_scanners.py@397 PS1, Line 397: a > flake8: E711 comparison to None should be 'if cond is None:' Done http://gerrit.cloudera.org:8080/#/c/18929/1/tests/query_test/test_scanners.py@429 PS1, Line 429: b > flake8: E226 missing whitespace around arithmetic operator Done http://gerrit.cloudera.org:8080/#/c/18929/3/tests/query_test/test_scanners.py File tests/query_test/test_scanners.py: http://gerrit.cloudera.org:8080/#/c/18929/3/tests/query_test/test_scanners.py@378 PS3, Line 378: 'text', 'parquet > Does it add to the coverage if we execute with both fileformats? My guess w Interestingly I got similar min/max ratios with text and parquet. I was thinking that it's good to run the tests on different file formats, but I can limit the test to 'text' if you want. http://gerrit.cloudera.org:8080/#/c/18929/3/tests/query_test/test_scanners.py@419 PS3, Line 419: ' > oops, I was wrong - let's comes from "let us" Done -- To view, visit http://gerrit.cloudera.org:8080/18929 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib7dc1f1665565da6c0e155c1e585f7089b18a180 Gerrit-Change-Number: 18929 Gerrit-PatchSet: 3 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Gergely Fürnstáhl <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Wed, 31 Aug 2022 11:29:08 +0000 Gerrit-HasComments: Yes
