DRILL-6030: Managed sort should minimize number of batches in a k-way merge

[vrozov]

@paul-rogers Please review

[paul-rogers]

Looks good, thanks! Just a minor suggestion about changing the existing config option rather than adding a new hard-coded value.

[paul-rogers]

The merge limit is already a config option. (I'd forgotten about that.) The comment on the config option says "Limit on the number of spilled batches that can be merged in a single pass." So, let's just set that default (in drill-override-conf) to your new value of 128 and leave the code here unchanged.

[vrozov]

IMO, it is better to change the default to avoid upgrade problems. In an upgrade scenario, users may simply overwrite drill-override.conf from their prior installations and forget to set the merge limit. Is there a reason not to change the default merge limit?

[paul-rogers]

There may be a misunderstanding of how config options work. We define the defaults in Drill's own source code: drill-module.conf in each module. (Here it is in java-exec.)

To change the default option, we change the value in drill-module.conf. In the highly unlikely case that a user has overridden this value in drill-override.conf, their value will be used. But, the option is not documented in drill-override-example.conf so it is very, very unlikely that anyone created an override. (The property is meant to be internal, for use in tests.)

So, rather than introducing yet another variable, we might as well use the existing config property. This has the added advantage that, if experience suggests that we need a smaller or larger limit for some scenarios, we can make the adjustment in the field via the config system.

[paul-rogers]

One additional thought. This bug was found when sorting 18 GB of data in 8 GB of memory. That is, a case in which the sort must spill.

What happens in the case in which the 18 GB of data is sorted in, say, 20 GB of memory (an in-memory sort)? We don't want the merge limit to force a spill in this case; kind of defeats the purpose of an in-memory sort.

So:

1. Does the limit affect in memory sort? If so, we need to revise the solution.
2. Does the in-memory sort suffer from a similar performance issue? If so, we need to revise the in memory sort.

One possible solution is to:

1. Defer sorting of individual batches until necessary.
2. Sort batches just before spilling.
3. If all batches fit in memory, do a single, combined sort (using an SV4).

[vrozov]

The scenario when all batches can be merged in memory is covered by 'if (canUseMemoryMerge())check inSortImpl.java:399`. The affected code path applies only to cases where merge between spilled and in-memory batches is necessary. Note that this is a short term fix to improve managed sort performance, in a long run, it is necessary to have an ability to merge all batches in memory (using SV4) without spilling and be able to merge it with the spilled data.

[vrozov]

@paul-rogers Please review

[paul-rogers]

LGTM
+1

[paul-rogers]

Still LGTM
+1