drill icon indicating copy to clipboard operation
drill copied to clipboard

DRILL-3715 Support all encoded vector types for Hash Aggregator.

Open amithadke opened this issue 9 years ago • 7 comments

amithadke avatar Aug 26 '15 22:08 amithadke

@StevenMPhillips @jaltekruse

amithadke avatar Aug 26 '15 22:08 amithadke

We should have a plan verification test to make sure that this change actually has the desired effect. This should be able to execute fine without your change as well, but the plan will have and unnecessary SelectionVectorRemover in it today. You can see examples of plan verification tests in /Users/jaltekruse/drill/exec/java-exec/src/test/java/org/apache/drill/TestCTASPartitionFilter.java and other subclasses of PlanTestBase

jaltekruse avatar Aug 26 '15 23:08 jaltekruse

There have been bugs in the handling of sv2 in the case where the first and second batch are filtered out. Could you add a test case for this? a similar test was added for streaming aggregate as part of DRILL-3069: https://github.com/apache/drill/commit/97a63168e93a70c7ed88d2e801dd2ea2e5f1dd74

mehant avatar Aug 26 '15 23:08 mehant

@mehant @jaltekruse Could you take a look at recent patch? Thanks.

amithadke avatar Aug 31 '15 19:08 amithadke

@amithadke, could you look at trying to write a test like this instead of checking in a large input file?

There is a session option to turn off hash aggregate, you should be able to just duplicate this test changing the session option. Be sure to change it back to the default value after your test as the session is currently shared across tests.

https://github.com/apache/drill/commit/97a63168e93a70c7ed88d2e801dd2ea2e5f1dd74#diff-91bfb8c9a26887fbe594262b6b872c98R295

jaltekruse avatar Sep 02 '15 16:09 jaltekruse

I got that backwards, it looks like the existing test should be disabling hash aggregate and verifying that streaming agg is planned instead. To be very thorough you might want to include a plan check in your test, but hash based operations are currently the default.

jaltekruse avatar Sep 02 '15 18:09 jaltekruse

@amithadke, did you have a chance to rework test in the way @jaltekruse proposed?

vvysotskyi avatar Jul 17 '19 10:07 vvysotskyi