Skip to content

Commit a220d57

Browse files
committed
[Sandbox] Reverse indexed-parquet segment iteration for opposite-direction sorts
Mirror of `ContextIndexSearcher.shouldUseTimeSeriesDescSortOptimization` for the indexed-parquet path. When `index.sort.field` is configured and the query's leading ORDER BY runs counter to the catalog's stored direction, reverse the per-shard segment iteration so a TopK above the scan pulls the highest-priority segment first and parquet page stats prune the rest. Reuses PR #22041's `ShardView.sort_fields` / `sort_orders` plumbing — no new Java/FFM surface. Only the indexed scan path (`IndexedTableProvider` / `QueryShardExec`) consumes them; the vanilla `ListingTable` path remains covered by #22041. `indexed_executor.rs`: - `analyze_top_sort(&LogicalPlan)` walks Projection/Limit/SubqueryAlias/ Distinct/Filter to find the leading top-of-plan ORDER BY. Returns None if the leading sort key isn't a plain `Expr::Column` (function on the sort key breaks catalog monotonicity). - `should_reverse_segments` triggers iff the catalog has a sort, the query's leading sort column matches `sort_fields[0]` (case-sensitive, matching PR #22041), and the directions differ. - `reverse_segment_iteration_order` flips `Vec<SegmentFileInfo>` in place. Per-segment `global_base` is deliberately NOT recomputed — see below. - Wired into `execute_indexed_with_context_inner` after `from_substrait_plan` decodes the LogicalPlan. `SessionContextHandle` carries `sort_fields` / `sort_orders` cloned from `ShardView` so both `create_session_context` and `create_session_context_indexed` populate the indexed-path consumer. The `__row_id__` emitted by the indexed scan is `segment.global_base + position`. QTF's fetch phase (`api::fetch_by_row_ids`) goes through a different code path (`ShardTableProvider`, not `IndexedTableProvider`) and rebuilds segments via `build_segments(ShardView.object_metas, ShardView.writer_generations)` — always in catalog order. Fetch then partition_points the cached row IDs against catalog-order `global_base`s to find the owning segment. For the round trip to hold, query-phase IDs must match catalog-order bases. `reverse_segment_iteration_order` therefore flips iteration order ONLY — each segment retains its catalog-order `global_base`. Reversal becomes purely a scheduling decision; the row-id space is unchanged. Rust unit tests in `indexed_executor::tests`: - `analyze_top_sort` (top-level desc, descends through Limit/Projection, None for no-sort, None for function sort key). - `should_reverse_segments` (matching field opposite direction, matching same direction, catalog desc query asc, no query sort, no catalog sort, non-leading catalog field, case-sensitive name). - `reverse_segments_preserves_global_base`, `reverse_segments_empty_is_noop`, `reverse_segments_single_keeps_its_base`. Rust e2e tests in `indexed_table/tests_e2e/sort_reverse_row_id.rs`: - `three_segments_row_ids_invariant_under_reversal`, `four_segments_row_ids_invariant_under_reversal` — pin the load-bearing invariant: same shard-global `__row_id__` set whether iteration is natural or reversed. Java IT in `sandbox/qa/analytics-engine-coordinator/.../SortReverseQtfIT.java` (parquet+lucene composite, 2 shards, 3 flushes → 2-4 segments per shard): - `testReversalTriggers_orderByTsDescLimit_correctResults` — captures the reversal log line via `MockLogAppender` on `RustLoggerBridge` AND asserts result correctness (top-k by ts DESC). - `testNoReversal_orderByTsAscLimit_correctResults` — `UnseenEventExpectation` proves reversal does NOT fire; results still correct. - `testReversal_keywordFilterDelegationPath_correctResults` — keyword filter shipped to Lucene perf peer + reversal; verifies QTF row-id round-trip composes with delegation. Signed-off-by: Arpit Bandejiya <abandeji@amazon.com>
1 parent 6a87a06 commit a220d57

9 files changed

Lines changed: 987 additions & 11 deletions

File tree

sandbox/plugins/analytics-backend-datafusion/rust/src/api.rs

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -346,9 +346,14 @@ pub struct ShardView {
346346
/// Index sort fields, in priority order. Sourced from the index's
347347
/// `index.sort.field` setting on the Java side. Empty when the index has
348348
/// no `index.sort.field` configured. Parallel to `sort_orders`.
349-
/// Used to build `ListingOptions.with_file_sort_order(...)` so DataFusion
350-
/// advertises `output_ordering` from the scan and enables the
351-
/// `sort_prefix` optimization on TopK / SortPreservingMerge.
349+
///
350+
/// Two consumers today:
351+
/// - Vanilla path: `ListingOptions.with_file_sort_order(...)` so DataFusion advertises
352+
/// `output_ordering` from the scan and the `sort_prefix` optimization fires on
353+
/// TopK / SortPreservingMerge.
354+
/// - Indexed path (`indexed_executor`): when the query's leading ORDER BY runs counter
355+
/// to catalog-snapshot order, the per-shard segment iteration is reversed so a TopK
356+
/// above us can prune via parquet page statistics.
352357
pub sort_fields: Vec<String>,
353358
/// Index sort directions per field — values: `"asc"` or `"desc"`.
354359
/// Parallel to `sort_fields`. Sourced from `index.sort.order`.

sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_executor.rs

Lines changed: 299 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -201,6 +201,101 @@ pub async fn execute_indexed_query(
201201

202202
// ── Helpers ───────────────────────────────────────────────────────────
203203

204+
/// Result of walking a logical plan looking for the leading top-of-plan ORDER BY.
205+
///
206+
/// `column` is the bare column name (no qualifier — we compare against `index.sort.field`
207+
/// which is also unqualified). `descending` is `true` for `ORDER BY x DESC`.
208+
#[derive(Debug, Clone, PartialEq, Eq)]
209+
pub(crate) struct TopSort {
210+
pub column: String,
211+
pub descending: bool,
212+
}
213+
214+
/// Walk through the top of a logical plan to find the leading sort expression.
215+
///
216+
/// Descends through `Projection`, `Limit`, `SubqueryAlias`, `Distinct`, and `Filter` —
217+
/// nodes that don't reorder rows or rewrite sort key columns. On reaching
218+
/// `LogicalPlan::Sort`, returns the leading sort key's bare column name and direction.
219+
/// Returns `None` if the plan has no top-level Sort, or if the first Sort key is not
220+
/// a plain `Expr::Column` (e.g. `ORDER BY lower(x)` — we can't claim catalog monotonicity
221+
/// after a function).
222+
pub(crate) fn analyze_top_sort(plan: &datafusion::logical_expr::LogicalPlan) -> Option<TopSort> {
223+
use datafusion::logical_expr::{Expr, LogicalPlan};
224+
let mut current = plan;
225+
loop {
226+
match current {
227+
LogicalPlan::Sort(s) => {
228+
let leading = s.expr.first()?;
229+
let col = match &leading.expr {
230+
Expr::Column(c) => c.name.clone(),
231+
_ => return None,
232+
};
233+
return Some(TopSort {
234+
column: col,
235+
descending: !leading.asc,
236+
});
237+
}
238+
LogicalPlan::Projection(p) => current = p.input.as_ref(),
239+
LogicalPlan::Limit(l) => current = l.input.as_ref(),
240+
LogicalPlan::SubqueryAlias(a) => current = a.input.as_ref(),
241+
LogicalPlan::Distinct(d) => match d {
242+
datafusion::logical_expr::Distinct::All(input) => current = input.as_ref(),
243+
datafusion::logical_expr::Distinct::On(on) => current = on.input.as_ref(),
244+
},
245+
LogicalPlan::Filter(f) => current = f.input.as_ref(),
246+
_ => return None,
247+
}
248+
}
249+
}
250+
251+
/// Decide whether to flip segment iteration order for the indexed scan.
252+
///
253+
/// Returns `true` iff the catalog has a sort declaration, the query has a top-level
254+
/// ORDER BY whose leading key matches the catalog's leading sort field by name, and
255+
/// the query's direction is the **opposite** of the catalog's. In that case the
256+
/// segments — laid down newest-last by the writer — are in reverse order from what
257+
/// the query wants, so iterating them tail-first feeds the largest values to a
258+
/// `TopK` first and parquet page stats prune the rest.
259+
///
260+
/// All comparisons are case-sensitive on the field name (matching PR #22041's
261+
/// `Column::from_name`). Direction comparison is case-insensitive on `"asc"`/`"desc"`.
262+
pub(crate) fn should_reverse_segments(
263+
top_sort: Option<&TopSort>,
264+
sort_fields: &[String],
265+
sort_orders: &[String],
266+
) -> bool {
267+
let Some(top) = top_sort else { return false };
268+
let Some(catalog_field) = sort_fields.first() else { return false };
269+
let Some(catalog_order) = sort_orders.first() else { return false };
270+
if top.column != *catalog_field {
271+
return false;
272+
}
273+
let catalog_descending = catalog_order.eq_ignore_ascii_case("desc");
274+
top.descending != catalog_descending
275+
}
276+
277+
/// Reverse the iteration order of `segments` in place. Per-segment `global_base`
278+
/// values are deliberately **left untouched** so each segment keeps its
279+
/// catalog-order shard-global row ID space.
280+
///
281+
/// Why not recompute global_base after reversing?
282+
///
283+
/// `global_base` is the additive offset used to compute the QTF `__row_id__`:
284+
/// `id = segment.global_base + position`. The fetch phase (`api::fetch_by_row_ids`)
285+
/// rebuilds segments via `build_segments` against `ShardView.object_metas` (always
286+
/// in catalog order) and reverses the mapping back via `partition_point` on
287+
/// `global_base`. For the round trip to hold, both phases must agree on each
288+
/// segment's `global_base` — and the only way to guarantee that without changing
289+
/// the fetch path is to keep query-phase `global_base` at its catalog-order value.
290+
///
291+
/// Reversing only the iteration order — i.e. the order in which chunks/RGs are
292+
/// scheduled by `compute_assignments` — is the *whole* point of this optimization
293+
/// (newest-segment-first feeds TopK earlier and lets page stats prune older
294+
/// segments). The `global_base` values are unrelated to that pruning win.
295+
pub(crate) fn reverse_segment_iteration_order(segments: &mut [SegmentFileInfo]) {
296+
segments.reverse();
297+
}
298+
204299
/// Collect all `Predicate(expr)` leaves in DFS order. Used by the
205300
/// dispatcher to build a per-leaf `PruningPredicate` cache keyed by
206301
/// `Arc::as_ptr` identity.
@@ -421,6 +516,191 @@ mod tests {
421516
]);
422517
assert!(extract_single_collector_residual(&tree).is_none());
423518
}
519+
520+
// ── analyze_top_sort / should_reverse_segments ────────────────────
521+
522+
fn build_logical_plan(sql: &str) -> datafusion::logical_expr::LogicalPlan {
523+
use datafusion::execution::SessionStateBuilder;
524+
use datafusion::execution::context::SessionContext;
525+
let state = SessionStateBuilder::new().with_default_features().build();
526+
let ctx = SessionContext::new_with_state(state);
527+
let schema = Arc::new(Schema::new(vec![
528+
Field::new("id", DataType::Int64, false),
529+
Field::new("ts", DataType::Int64, false),
530+
Field::new("v", DataType::Int32, false),
531+
]));
532+
ctx.register_batch(
533+
"t",
534+
datafusion::arrow::record_batch::RecordBatch::new_empty(schema),
535+
)
536+
.expect("register_batch");
537+
let rt = tokio::runtime::Builder::new_current_thread()
538+
.enable_all()
539+
.build()
540+
.unwrap();
541+
let df = rt.block_on(ctx.sql(sql)).expect("sql");
542+
df.into_unoptimized_plan()
543+
}
544+
545+
#[test]
546+
fn analyze_top_sort_finds_leading_desc() {
547+
let plan = build_logical_plan("SELECT id FROM t ORDER BY id DESC");
548+
let ts = analyze_top_sort(&plan).expect("expected Sort");
549+
assert_eq!(ts.column, "id");
550+
assert!(ts.descending);
551+
}
552+
553+
#[test]
554+
fn analyze_top_sort_descends_through_limit() {
555+
let plan = build_logical_plan("SELECT id FROM t ORDER BY id ASC LIMIT 10");
556+
let ts = analyze_top_sort(&plan).expect("expected Sort");
557+
assert_eq!(ts.column, "id");
558+
assert!(!ts.descending);
559+
}
560+
561+
#[test]
562+
fn analyze_top_sort_descends_through_projection() {
563+
let plan = build_logical_plan("SELECT v FROM t ORDER BY ts DESC");
564+
let ts = analyze_top_sort(&plan).expect("expected Sort");
565+
assert_eq!(ts.column, "ts");
566+
assert!(ts.descending);
567+
}
568+
569+
#[test]
570+
fn analyze_top_sort_returns_none_when_no_sort() {
571+
let plan = build_logical_plan("SELECT id FROM t");
572+
assert!(analyze_top_sort(&plan).is_none());
573+
}
574+
575+
#[test]
576+
fn analyze_top_sort_returns_none_for_function_sort_key() {
577+
// `ORDER BY abs(v)` — leading sort key isn't a plain column. Catalog monotonicity
578+
// doesn't transfer through a function, so we conservatively decline to reverse.
579+
let plan = build_logical_plan("SELECT id FROM t ORDER BY abs(v)");
580+
assert!(analyze_top_sort(&plan).is_none());
581+
}
582+
583+
#[test]
584+
fn should_reverse_segments_matches_leading_field_opposite_direction() {
585+
let top = TopSort { column: "id".to_string(), descending: true };
586+
let fields = vec!["id".to_string()];
587+
let orders = vec!["asc".to_string()];
588+
assert!(should_reverse_segments(Some(&top), &fields, &orders));
589+
}
590+
591+
#[test]
592+
fn should_reverse_segments_matches_leading_field_same_direction() {
593+
let top = TopSort { column: "id".to_string(), descending: false };
594+
let fields = vec!["id".to_string()];
595+
let orders = vec!["asc".to_string()];
596+
assert!(!should_reverse_segments(Some(&top), &fields, &orders));
597+
}
598+
599+
#[test]
600+
fn should_reverse_segments_catalog_desc_query_asc() {
601+
let top = TopSort { column: "id".to_string(), descending: false };
602+
let fields = vec!["id".to_string()];
603+
let orders = vec!["desc".to_string()];
604+
assert!(should_reverse_segments(Some(&top), &fields, &orders));
605+
}
606+
607+
#[test]
608+
fn should_reverse_segments_no_query_sort() {
609+
let fields = vec!["id".to_string()];
610+
let orders = vec!["asc".to_string()];
611+
assert!(!should_reverse_segments(None, &fields, &orders));
612+
}
613+
614+
#[test]
615+
fn should_reverse_segments_no_catalog_sort() {
616+
let top = TopSort { column: "id".to_string(), descending: true };
617+
assert!(!should_reverse_segments(Some(&top), &[], &[]));
618+
}
619+
620+
#[test]
621+
fn should_reverse_segments_query_sort_on_non_leading_catalog_field() {
622+
// Catalog: [a ASC, b ASC]; query: ORDER BY b DESC. Segments are monotonic on `a`
623+
// (the leading key), not `b`. Reversing won't help — decline.
624+
let top = TopSort { column: "b".to_string(), descending: true };
625+
let fields = vec!["a".to_string(), "b".to_string()];
626+
let orders = vec!["asc".to_string(), "asc".to_string()];
627+
assert!(!should_reverse_segments(Some(&top), &fields, &orders));
628+
}
629+
630+
#[test]
631+
fn should_reverse_segments_field_name_case_sensitive() {
632+
// Match PR #22041 — `Column::from_name` is case-sensitive. If casing differs,
633+
// we don't claim the catalog ordering applies. Safe default: no reversal.
634+
let top = TopSort { column: "ID".to_string(), descending: true };
635+
let fields = vec!["id".to_string()];
636+
let orders = vec!["asc".to_string()];
637+
assert!(!should_reverse_segments(Some(&top), &fields, &orders));
638+
}
639+
640+
// ── reverse_segment_iteration_order ───────────────────────────────
641+
642+
fn dummy_segment(max_doc: i64, global_base: u64) -> SegmentFileInfo {
643+
use datafusion::parquet::file::metadata::{FileMetaData, ParquetMetaData};
644+
// Build a minimal ParquetMetaData. We never read it back in these tests.
645+
let schema = std::sync::Arc::new(
646+
datafusion::parquet::schema::types::SchemaDescriptor::new(
647+
std::sync::Arc::new(
648+
datafusion::parquet::schema::types::Type::group_type_builder("schema")
649+
.build()
650+
.unwrap(),
651+
),
652+
),
653+
);
654+
let file_meta = FileMetaData::new(0, 0, None, None, schema, None);
655+
let pq_meta = ParquetMetaData::new(file_meta, vec![]);
656+
SegmentFileInfo {
657+
writer_generation: global_base as i64 + 1, // arbitrary, just to vary
658+
max_doc,
659+
object_path: object_store::path::Path::from(format!("seg-{}.parquet", global_base)),
660+
parquet_size: 0,
661+
row_groups: vec![],
662+
metadata: std::sync::Arc::new(pq_meta),
663+
global_base,
664+
}
665+
}
666+
667+
#[test]
668+
fn reverse_segments_preserves_global_base() {
669+
// Original: A(max_doc=10, base=0), B(max_doc=20, base=10), C(max_doc=30, base=30).
670+
// Reversal must keep each segment's original `global_base` intact so QTF row IDs
671+
// emitted in query phase remain interpretable by the fetch phase (which always
672+
// computes catalog-order bases).
673+
let mut segs = vec![
674+
dummy_segment(10, 0),
675+
dummy_segment(20, 10),
676+
dummy_segment(30, 30),
677+
];
678+
reverse_segment_iteration_order(&mut segs);
679+
assert_eq!(segs.len(), 3);
680+
// New iteration order: C, B, A.
681+
assert_eq!(segs[0].max_doc, 30);
682+
assert_eq!(segs[0].global_base, 30); // C's catalog base, unchanged.
683+
assert_eq!(segs[1].max_doc, 20);
684+
assert_eq!(segs[1].global_base, 10); // B's catalog base, unchanged.
685+
assert_eq!(segs[2].max_doc, 10);
686+
assert_eq!(segs[2].global_base, 0); // A's catalog base, unchanged.
687+
}
688+
689+
#[test]
690+
fn reverse_segments_empty_is_noop() {
691+
let mut segs: Vec<SegmentFileInfo> = vec![];
692+
reverse_segment_iteration_order(&mut segs);
693+
assert!(segs.is_empty());
694+
}
695+
696+
#[test]
697+
fn reverse_segments_single_keeps_its_base() {
698+
let mut segs = vec![dummy_segment(42, 7)];
699+
reverse_segment_iteration_order(&mut segs);
700+
assert_eq!(segs.len(), 1);
701+
assert_eq!(segs[0].global_base, 7);
702+
assert_eq!(segs[0].max_doc, 42);
703+
}
424704
}
425705

426706
/// Instruction-based indexed execution path. Consumes a pre-configured SessionContextHandle
@@ -553,6 +833,25 @@ async unsafe fn execute_indexed_with_context_inner(
553833
.map_err(|e| DataFusionError::Execution(format!("decode substrait: {}", e)))?;
554834
let logical_plan = from_substrait_plan(&ctx.state(), &plan).await?;
555835

836+
// Sort-aware segment iteration. Mirror of `ContextIndexSearcher.shouldUseTimeSeriesDescSortOptimization`
837+
// for the indexed-parquet path. When the index has `index.sort.field` and the query's leading
838+
// ORDER BY runs counter to the catalog's stored direction, reverse the segment vector so a
839+
// TopK above us pulls the highest-priority segment first and parquet page stats prune the rest.
840+
//
841+
// QTF safe: `reverse_segment_iteration_order` deliberately does NOT recompute `global_base`
842+
// — each segment retains its catalog-order base, so the row IDs query phase emits are still
843+
// interpretable by `api::fetch_by_row_ids` (which builds its own segments from
844+
// `ShardView.object_metas` in catalog order).
845+
let mut segments = segments;
846+
if should_reverse_segments(analyze_top_sort(&logical_plan).as_ref(), &sort_fields, &sort_orders) {
847+
log_debug!(
848+
"indexed_executor: reversing segment iteration (catalog leading sort={:?} {:?}, query opposite)",
849+
sort_fields.first(),
850+
sort_orders.first()
851+
);
852+
reverse_segment_iteration_order(&mut segments);
853+
}
854+
556855
let emit_row_ids = requests_row_ids;
557856
let filter_expr = extract_filter_expr(&logical_plan);
558857
let extraction = match filter_expr {

sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_table/tests_e2e/mod.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@ mod qtf_fetch_phase;
4949
mod row_id_emission;
5050
mod row_id_strategies;
5151
mod schema_drift;
52+
mod sort_reverse_row_id;
5253
mod streaming_at_scale;
5354

5455
// ── Test fixture: parquet table with 16 rows ────────────────────────

0 commit comments

Comments
 (0)