Fix NestedLoopJoin performance regression #12531
Conversation
alihan-synnada
changed the title
| let capacity = left_row_count * right_row_count; | ||
|
|
||
| // Left indices are 0..left_row_count repeated right_row_count times | ||
| let mut left_indices_builder = UInt64Array::builder(capacity); |
There was a problem hiding this comment.
Using Vec to build the indices is slightly faster and has some nicer syntax.
| // Right indices are each right row index repeated left_row_count times | ||
| let mut right_indices_builder = UInt32Array::builder(capacity); | ||
| for right_index in 0..right_row_count { | ||
| right_indices_builder.extend(vec![Some(right_index as u32); left_row_count]) |
There was a problem hiding this comment.
We should avoid this intermediate Vec
There was a problem hiding this comment.
Thanks for the heads up
|
/benchmark |
There was a problem hiding this comment.
LGTM overall -- I have some doubts regarding if caching really required here -- we can discuss it.
Also I've noticed that this PR increases the size of intermediate batches, which, though, seems to be acceptable until NLJ is allowed to emit massive (significantly more than configured batch size) batches as its output (it works as before and there is an issue for fixing this).
|
|
||
| // We always use the same indices before applying the filter, so we can cache them | ||
| let (left_indices_cache, right_indices_cache) = indices_cache; | ||
| let cached_output_row_count = left_indices_cache.len(); |
There was a problem hiding this comment.
In case of 25 rows build-side there are 200k arrays, for 500 rows -- 4kk and so on (I suppose we don't need that much data on the build side to reach GBs size for these arrays).
I understand that we still will have to create interemediate batches to apply filter, and produce output batches, but I suppose, that starting from some point the size of these caches will become meaningful.
There was a problem hiding this comment.
I guess we can do away with the cache or make it optional. In case we remove the cache, we could create the indices and apply the filter in chunks similar to before. If we pass in a range that we then use to calculate the indices for instead of creating right_batch.num_rows() chunks, we can control the size of the intermediate batches too. Something like (0..output_row_count).chunks(CHUNK_SIZE) should do the trick, now that we create the indices by mapping the current row index.
I believe it can bring the performance without cache down to a similar level to before the regression, maybe even better. I'll run a few benchmarks with this setup without a cache and update the benchmarks table.
There was a problem hiding this comment.
The chunks approach didn't change the performance, but it helped reduce the sizes of the intermediate batches. The 10% performance hit without a cache comes from the way the arrays are constructed and I couldn't find a faster approach for now. I suggest we go with the cached approach for now. When the issue that enables NLJ to emit massive batches is implemented, we can choose between the cached and chunked approaches depending on NLJ's output size. I'll open an issue about it
There was a problem hiding this comment.
Sounds good. I will merge this soon to avoid performance issues in any upcoming release unless there is more feedback. We seem to gain 20% performance relative to how it was before with caches, and we can migrate to a cached-vs-chunked-depending-on-output-batch-size approach in the future.
There was a problem hiding this comment.
The chunks approach didn't change the performance, but it helped reduce the sizes of the intermediate batches.
Thank you for checking this option
Which issue does this PR close?
Closes #12528.
Rationale for this change
Iterating over the right rows instead of the left while calling
build_join_indicesincreased the number of calls toapply_join_filter_to_indicesin cases where the right table has more rows (which applies to most common use cases). Building the indices insidebuild_join_indiceshelps reduce the number of calls toapply_join_filter_to_indicesThe same indices are created inside
build_join_indicesevery time with an expensive operation so it makes sense to cache them.What changes are included in this PR?
This PR builds the indices in one go inside
build_join_indices, removing the outer iteration, and reduces the number of calls toapply_join_filter_to_indicesto 1 perjoin_left_and_right_batchcall.Also adds caching for join indices.
Are these changes tested?
Tested with the following query using TPCH data (SF=1)
Are there any user-facing changes?
Only performance changes.