[kudu-CR] [tablet] Skip stable sort for already sorted batch

["Marton Greber (Code Review)" <ge...@cloudera.org>]

Marton Greber has posted comments on this change. ( http://gerrit.cloudera.org:8080/24615 )

Change subject: [tablet] Skip stable_sort for already sorted batch
......................................................................


Patch Set 2:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/24615/2/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:

http://gerrit.cloudera.org:8080/#/c/24615/2/src/kudu/tablet/tablet.cc@1154
PS2, Line 1154:   const MonoTime bulk_check_start = MonoTime::Now();
nit: bulk_check_start is computed unconditionally, but it's only consumed under `if (metrics_)` at line 1319. Worth guarding the MonoTime::Now() behind metrics_ (or accepting it as negligible) - trivial either way, but it's dead work when metrics_ is null.


http://gerrit.cloudera.org:8080/#/c/24615/2/src/kudu/tablet/tablet.cc@1179
PS2, Line 1179:     keys_and_indexes.emplace_back(key, i);
No test accompanies the change. Given the correctness argument rests on "non-decreasing input == valid stable-sort output," a small regression test asserting that a pre-sorted batch and its shuffled permutation yield identical presence results (and that the pre_sorted/needed_sort counters move as expected) would lock this in cheaply. Do existing bulk-check tests already cover both orderings?


http://gerrit.cloudera.org:8080/#/c/24615/2/src/kudu/tablet/tablet.cc@1203
PS2, Line 1203:       sort_us = (MonoTime::Now() - sort_start).ToMicroseconds();
The commit message says these metrics exist "so that the sort-skip optimisation can be verified in isolation." Are all five intended to ship permanently, or are bulk_check_duration / bulk_check_sort_duration diagnostic scaffolding? This is a perf patch, yet it permanently adds 2-3 MonoTime::Now() calls plus several histogram/counter Increments to every write batch on the path it's optimizing. The per-op work dominates so the net benchmark is still positive, but it's worth being explicit about which of these we want long-term vs. what was just for validating the change.


http://gerrit.cloudera.org:8080/#/c/24615/2/src/kudu/tablet/tablet.cc@1211
PS2, Line 1211:     if (is_sorted) {
q: a batch where every op was filtered out (has_result / orig_result_from_log) reaches here with keys_and_indexes empty, is_sorted still true, and gets counted as a "pre-sorted batch" with input_size 0. Minor, but it slightly inflates the pre-sorted counter with empty batches. Intended?



-- 
To view, visit http://gerrit.cloudera.org:8080/24615
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I28d3f5ebdf7e3f13f04ffe8d35417a98c370fcf7
Gerrit-Change-Number: 24615
Gerrit-PatchSet: 2
Gerrit-Owner: Ashwani Raina <ar...@gmail.com>
Gerrit-Reviewer: Ashwani Raina <ar...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Comment-Date: Sun, 02 Aug 2026 10:19:11 +0000
Gerrit-HasComments: Yes