feat: Sort-aware Iceberg reads in Comet via a per-partition streaming merge

[parthchandra]

Which issue does this PR close?

Closes #5337

Rationale for this change

When Comet reads a sorted Iceberg table, the native scan throws away the ordering to get
parallelism. Spark then can't tell the data is already sorted and re-sorts on every read — in
joins, aggregates, windows, and order-by queries — even though the sort was done at write time.

This PR makes the native scan preserve and report that ordering so Spark can drop the redundant
sorts. It builds on Iceberg's SupportsReportOrdering (apache/iceberg#14948): when Iceberg reports
a sort order, Comet merges the sorted files per partition and tells Spark the result is sorted.

What changes are included in this PR?

For each Spark partition, the scan reads every sorted file as its own stream and k-way-merges them
into one sorted stream with DataFusion's SortPreservingMergeExec.

Summary of changes:

• Proto: IcebergScanCommon carries the reported sort order (table_sort_orders) and the
  merge cap (max_files_per_partition) to the native side.
• Scala (rule + serde + scan exec): report the sort order to Spark, gated to the safe v1 case —
  identity-transform sort fields on top-level columns that are in the projection. Anything else
  (a transform sort key, a UUID sort key whose Iceberg byte order differs from Spark's string order,
  or a schema that can't be read) reports nothing and stays on Spark, so the result is always
  correct. The gate is evaluated and bound to proto once at planning time, so a binding failure
  falls back cleanly instead of erroring at task start.
• Native (Rust): IcebergScanExec becomes multi-partition when an ordering is present (one
  sorted stream per file), and the planner wraps it in SortPreservingMergeExec. Above
  maxFilesPerPartition files in a partition it instead reads unordered and wraps a spillable
  SortExec, which bounds concurrently-open readers. Both paths produce sorted output. No changes
  to iceberg-rust.

Config flags:

• spark.comet.scan.icebergNative.sortMerge.enabled (default on) — do the per-partition merge.
  When off, the scan stays native and still honours the reported order via a spillable sort instead
  of the k-way merge (equivalent to maxFilesPerPartition = 0). Only does anything when Iceberg's
  spark.sql.iceberg.planning.preserve-data-ordering is on (off by default).
• spark.comet.scan.icebergNative.sortMerge.maxFilesPerPartition (default 64) — the most files
  in one partition Comet will k-way merge before falling back to the spillable sort.

Note: a global ORDER BY keeps its final sort — a per-partition merge isn't a cluster-wide order —
so that case is unchanged.

How are these changes tested?

• Native planner tests in planner.rs: merge-vs-sort at the maxFilesPerPartition boundary, and
  that the cap is read from the proto (a low cap flips a 3-file scan to the sort path).
• End-to-end suite CometIcebergSortMergeReadSuite over real Iceberg tables (local Hadoop
  catalog, sort order set via the Iceberg Java API, one file per insert), plus unit tests of the
  reportability gate. Correctness (checkSparkAnswer) runs on any Iceberg build; the
  sort/shuffle-elimination assertions self-cancel on the published Iceberg that doesn't report an
  ordering.

Key edits: dropped the key-grouped-partitioning paragraph and the reportPartitioning.enabled flag (both gone with the outputPartitioning override), added maxFilesPerPartition and the enabled=false semantics, fixed the malformed Closes # link, and refreshed the tests section. Double-check #5337 is the right issue — the old body had a malformed link; I preserved the number.

[parthchandra]

This PR has some followups -

• Sort keys not in the projection. Today, if a sort column isn't selected, we skip the merge and read unordered. Todo: read the sort columns anyway (even when not selected), merge on them, then drop them before returning.
• Transform sort orders (bucket/truncate/etc.). We only handle plain-column sort orders for now. todo: also handle transformed sort keys, matching Iceberg's per-type comparison exactly.
• Read concurrency. The merge reads files more lazily than today's unordered read. As a followup benchmark, and if many-small-file reads regress, read files ahead into small buffers.
• Single-file partitions. A partition with one file still gets wrapped in the merge (a no-op). Skip the wrapper in that case.
• Storage-partitioned join confidence tests, then default reportPartitioning on. Add tests for: (a) a heavily skewed key so the replicate path runs, (b) different bucket counts per side (e.g. bucket(8) vs bucket(4)) so a partition reducer kicks in, (c) three-way and self joins on the same key. Once green, flip the flag on by default.

[parthchandra]

@anuragmantri, @peter-toth , you might be interested in looking at this. k-way merge to maintain the ordered property of sorted Iceberg tables. Feedback, especially about test coverage, would be highly appreciated.

[parthchandra]

Added more followup issues in parent issue - #5323

[anuragmantri]

Thanks for pinging here @parthchandra. It is great to see there is interest in this optimization in the comet project.

I reviewed the general implementation and the test coverage in this PR. I don't have any major comments, most are clarifications and minor suggestions. That said, I'm new to Comet codebase and would like someone more familiar to also take a look.

[anuragmantri]

As identified in the Iceberg PR and the design doc #5323, UUID orders differently in Iceberg than in Spark's comparator, and the identity case for UUID needs to follow Iceberg's byte ordering specifically. This gate doesn't check the sort column's type at all. I believe we could rely on upstream apache/iceberg#16750 to not report ordering on UUID. Is my understanding correct?

[parthchandra]

I was planning to address this in a follow up. But now I've added a type check rather than relying only on upstream. IcebergReflection.orderingUnsafeColumns returns UUID column names, and reportableOrdering now refuses any sort key in that set

[anuragmantri]

I believe using scanExec.ordering is to bypass the check in Spark 3.4 and 3.5 which was fixed in Spark 4.2 by SPARK-55715 and this is needed to support all these Spark versions. @peter-toth, would you please also take a look at this to see if this is safe?

[peter-toth]

Seems good to me, scanExec.ordering is the order reported by the source and that's what we need to keep with k-way merge if possible, and return as scanExec.outputOrdering so as Spark can use it to elimiate sorts.

[peter-toth]

So as we discussed offline CometIcebergNativeScanExec.scala.outputOrdering and this whole logic might not be needed if BatchScanExec -> CometIcebergNativeScanExec translation happend after EnsureRequirements runs (i.e. we eliminated the SortExecs based on BatchScanExec.outputOrdering).

And same for CometIcebergNativeScanExec.outputPartitioning...

[parthchandra]

@anuragmantri Correct. DataSourceV2ScanExecBase.outputOrdering blanks the ordering when a partition has more than one file on Spark 3.4–4.1 (relaxed in 4.2 by SPARK-55715). We need to keep this as long as we support versions before 4.2.

[parthchandra]

So as we discussed offline CometIcebergNativeScanExec.scala.outputOrdering and this whole logic might not be needed if BatchScanExec -> CometIcebergNativeScanExec translation happend after EnsureRequirements runs (i.e. we eliminated the SortExecs based on BatchScanExec.outputOrdering).

And same for CometIcebergNativeScanExec.outputPartitioning...

@peter-toth I dug in deeper and you are correct. EnsureRequirements does run before Comet converts the scan (confirmed in AdaptiveSparkPlanExec.queryStagePreparationRules and the non-AQE order), so it eliminates the shuffle on the BatchScanExec. An experiment confirmed it: with partitioning reporting off, the SMJ still has no Exchange. So I removed the outputPartitioning override and the reportPartitioning flag entirely.
outputOrdering is still needed because pre 4.2 Spark still has the check that blanks out BatchScanExec.outputOrdering for multi-file partitions. So we need to include the check to eliminate sort.

[parthchandra]

Made more one change. Added a check to fall back to spark if iceberg reports ordering and native cannot support it (without that we will silently get wrong results because Spark would have removed the sort already).

[anuragmantri]

Claude flagged this:

BaselineMetrics::new(metrics, 0) hardcodes partition 0, but this operator is now multi-partition in the ordered path. Should this thread through the actual partition index from execute()?

[parthchandra]

Good catch Claude! Updated this.

[anuragmantri]

I reviewed the tests, very nice coverage already. I would also a test that deletes some rows from one file in a multi-file sorted partition, to cover the merge alongside MOR deletes.

[parthchandra]

added new test for this case

[andygrove]

One thing I'd like to dig into more before this lands: the reader fan-out in the ordered path.

SortPreservingMergeExec calls execute() on all of its input partitions up front, and IcebergScanExec::execute eagerly builds the ArrowReader and kicks off the read, so we end up with a live Parquet reader per data file in the Spark partition plus a buffered batch per stream inside the merge. data_file_concurrency_limit doesn't bound any of that anymore.

The comment in iceberg_scan.rs describes the fan-out as intrinsic to a k-way merge. That's true of the number of streams, but not of how many readers we have decoding concurrently or how much we buffer — a cascaded merge, or just falling back to the unordered read when a partition has too many files, would bound both.

The tables in the tests are fine, but the workload this targets is a sorted table, and a sorted table that has accumulated a lot of small commits is exactly where you get hundreds of files in one task. Since the merge reserves against Comet's memory pool, hitting the limit there is a query failure rather than a slowdown, which is a worse failure mode than the extra sort we're trying to avoid.

Could we add a config for the maximum files per partition we're willing to merge, and fall back to the unordered read above it? That keeps the default safe and lets people opt into deeper merges once we have a better sense of the memory profile.

[andygrove]

Following up on test coverage, because I want to make sure I'm reading this right.

As far as I can tell SupportsReportOrdering is what populates BatchScanExec.ordering, and that field is the only trigger for everything in this PR. Iceberg's implementation of it is apache/iceberg#16750, which is still open and targeted at Spark 4.1, so on the published Iceberg versions in our poms (1.5.2, 1.8.1, 1.10.0, 1.11.0) ordering is always None. That means reportableOrdering returns Nil, table_sort_orders goes over empty, native takes ordering = None, and we get the old single-partition unordered scan with no SortPreservingMergeExec.

If that's right, the effect in CI is bigger than the canceled plan assertions. The k-way merge itself never runs — every checkSparkAnswer in the new suite is exercising the read path we already had, so the only CI coverage of the new native code is the two Rust tests asserting partition_count().

There's a related wrinkle even on a reporting build: a global ORDER BY keeps its final sort, which would repair any mis-ordering the merge produced, so those tests catch dropped or duplicated rows but not wrong order. The order-sensitive assertions are all in the assume-gated SMJ/aggregate/window tests.

Am I understanding the situation correctly, or is there something in the CI setup I'm missing that does exercise the merge?

[andygrove]

One more, on the gate in serializePartitions:

val reportable = reportableOrdering(scanExec.ordering, output)
if (reportable.nonEmpty) {
  val protoOrders = reportable.map(exprToProto(_, output))
  if (protoOrders.forall(_.isDefined)) {
    commonBuilder.addAllTableSortOrders(protoOrders.map(_.get).asJava)
  }
}

The comment above it argues the forall always holds, and I agree that's true today. My worry is what happens if it ever stops being true, because the guard makes the unsafe direction the silent one. If we merge without telling Spark, we've wasted some work and nothing else. If we tell Spark and then don't merge, Spark drops the Sort and we return wrong results with no error.

That's not purely hypothetical, because the gate gets evaluated twice against two different points in time. outputOrdering is a lazy val forced during planning, this runs later off serializedPartitionData, and reportableOrdering reads COMET_ICEBERG_SORT_MERGE_ENABLED out of the active SQLConf each time. Two dynamic conf reads on separate threads at separate stages is the kind of thing that holds until it doesn't.

serializePartitions only has the one caller, so could we just pass outputOrdering in from the exec and have a single evaluation? Failing that I'd rather the mismatch threw than silently produced a proto that disagrees with what we told Spark.

[parthchandra]

That's not purely hypothetical, because the gate gets evaluated twice against two different points in time. outputOrdering is a lazy val forced during planning, this runs later off serializedPartitionData, and reportableOrdering reads COMET_ICEBERG_SORT_MERGE_ENABLED out of the active SQLConf each time. Two dynamic conf reads on separate threads at separate stages is the kind of thing that holds until it doesn't.

Fixed. This is evaluated once now.

serializePartitions only has the one caller, so could we just pass outputOrdering in from the exec and have a single evaluation? Failing that I'd rather the mismatch threw than silently produced a proto that disagrees with what we told Spark.

Fixed. This throws if a reported order can't be serialized, instead of the old silent if (forall(_.isDefined)) drop

[parthchandra]

Following up on test coverage, because I want to make sure I'm reading this right.

As far as I can tell SupportsReportOrdering is what populates BatchScanExec.ordering, and that field is the only trigger for everything in this PR. Iceberg's implementation of it is apache/iceberg#16750, which is still open and targeted at Spark 4.1, so on the published Iceberg versions in our poms (1.5.2, 1.8.1, 1.10.0, 1.11.0) ordering is always None. That means reportableOrdering returns Nil, table_sort_orders goes over empty, native takes ordering = None, and we get the old single-partition unordered scan with no SortPreservingMergeExec.

If that's right, the effect in CI is bigger than the canceled plan assertions. The k-way merge itself never runs — every checkSparkAnswer in the new suite is exercising the read path we already had, so the only CI coverage of the new native code is the two Rust tests asserting partition_count().

There's a related wrinkle even on a reporting build: a global ORDER BY keeps its final sort, which would repair any mis-ordering the merge produced, so those tests catch dropped or duplicated rows but not wrong order. The order-sensitive assertions are all in the assume-gated SMJ/aggregate/window tests.

Am I understanding the situation correctly, or is there something in the CI setup I'm missing that does exercise the merge?

You're reading this right. I cannot think of a way to mock Iceberg without the actual implementation (which is still in review). The tests cover the fallback, and are somewhat forward looking. In a way this feature is itself forward looking - once this feature is released in Iceberg and Spark starts to eliminate sorts based on what iceberg-java reports, Comet will produce absolutely garbage results unless we have this implementation in place.

[parthchandra]

One thing I'd like to dig into more before this lands: the reader fan-out in the ordered path.

SortPreservingMergeExec calls execute() on all of its input partitions up front, and IcebergScanExec::execute eagerly builds the ArrowReader and kicks off the read, so we end up with a live Parquet reader per data file in the Spark partition plus a buffered batch per stream inside the merge. data_file_concurrency_limit doesn't bound any of that anymore.

The comment in iceberg_scan.rs describes the fan-out as intrinsic to a k-way merge. That's true of the number of streams, but not of how many readers we have decoding concurrently or how much we buffer — a cascaded merge, or just falling back to the unordered read when a partition has too many files, would bound both.

The tables in the tests are fine, but the workload this targets is a sorted table, and a sorted table that has accumulated a lot of small commits is exactly where you get hundreds of files in one task. Since the merge reserves against Comet's memory pool, hitting the limit there is a query failure rather than a slowdown, which is a worse failure mode than the extra sort we're trying to avoid.

Could we add a config for the maximum files per partition we're willing to merge, and fall back to the unordered read above it? That keeps the default safe and lets people opt into deeper merges once we have a better sense of the memory profile.

I was planning to address this properly in a followup (#5343). But I can implement that in this PR if you think I should address it right away.

[mbutrovich]

First pass, looks to be in pretty good shape.

[mbutrovich]

Every test in this suite inserts 2 to 4 files per partition. Given the memory concern in iceberg_scan.rs is specifically about a partition with a large number of files (a live Parquet reader plus a buffered batch per file, all opened at once), would it be worth adding one test that inserts something like 50 to 100 single-row files into one partition? It would give the merge path a correctness check under the actual shape of the workload this feature targets, not just its happy-path shape, and would be a natural place to assert on memory pool usage or peak concurrent readers once #5343 lands.

[mbutrovich]

Following up on the fan-out discussion: I don't think this needs a wholly new algorithm, but it does need more than IcebergScanExec alone, since the operator that decides when to poll each partition is SortPreservingMergeExec, not this one. That operator has no notion of a bound and primes every input stream up front because polling is the only signal it has for "what's in this stream." Making IcebergScanExec::execute lazy internally doesn't avoid that, since SortPreservingMergeExec still calls poll_next on every partition almost immediately to seed its loser tree.

What would actually bound this is a custom merge operator that takes each file's min/max bound on the sort key alongside its FileScanTask, keeps a small active set merged the way SortPreservingMergeExec does today, and holds a priority queue of unopened files ordered by min bound, only calling execute/poll_next on the next pending file once its min bound could produce the next output value. That bounds concurrently-open readers by the overlap width of the file ranges at a given point in the merge rather than by total file count, which is the number that actually blows up on a sorted table with a lot of small commits.

The blocker today is data, not algorithm: FileScanTask in iceberg-rust doesn't carry column-level bounds. They exist upstream on the manifest-entry DataFile (lower_bounds/upper_bounds, already used for predicate pushdown), just not threaded down into the task struct the native scan gets. So this needs plumbing in iceberg-rust or an extra field fetched at scan-planning time on the JVM side, either way before a bound-driven admission scheme can work.

Given the risk, could we land @andygrove's simpler mitigation in this PR, a config for the max files to merge per partition that falls back to the unordered read above it, and keep the bound-driven design above as the concrete plan for #5343? Sort-merge ships on by default, and the workload it targets (a sorted table with accumulated small commits) is exactly where a partition ends up with hundreds of files, so I'd rather the default be safe now and the deeper optimization follow once the stats plumbing exists.

[parthchandra]

Agreed, and added the fix recommended by @andygrove to limit max files per partition with a change. We cannot fallback to unordered because by this time Spark planning would have removed the downstream sort and not producing ordered results will produce wrong results. So now, beyond the threshold, we will fall back to a spillable SortExec so we always produce correct results

[parthchandra]

@andygrove verified this with a local build of iceberg patched with support for SupportsReportOrdering and all the tests added in this PR pass. One test is skipped/canceled because it operates on an unpartitioned table and Iceberg reports (correctly) that the data may not be ordered.
Also added tests to check we fallback correctly to Spark for cases where we do not support this.
cc @anuragmantri - fyi.

[andygrove]

Thanks for working through the earlier rounds. The fan-out cap, the single-evaluation of the proto gate, the require instead of the silent drop, and dropping the outputPartitioning override after peter-toth's point all landed well, and the new tests for MOR deletes, the 70-file merge, the 70-file sort fallback, and the three Spark-fallback cases close most of what I was worried about on coverage. I also liked that you pushed back on my "fall back to the unordered read" phrasing. You're right that by that point Spark has dropped the sort, so it has to be a native sort.

A few things I checked and am happy with, so nobody else has to redo them. The fallback SortExec uses the same SortExec::new plus create_sort_expr as OpStruct::Sort, so it sorts identically to the sort it replaces. DiskManagerMode::Directories is configured, so that sort can spill rather than fail. Moving the metrics off partition 0 is safe because update_comet_metric uses aggregate_by_name(). And on types beyond UUID, I worked through the other Iceberg-to-Spark mappings: binary and fixed compare unsigned on both sides, and float/double are safe in the direction that matters, since Iceberg's total order refines Spark's nanSafeCompare where -0.0 == 0.0, and a refinement of a weaker order is still non-decreasing under it. UUID does look like the only real hazard today.

I have six things I'd like addressed. Five are inline. The first two are correctness, and the rest are about keeping this maintainable.

The sixth has no diff line to attach to: docs/source/user-guide/latest/iceberg.md. The "Current limitations" list is where people look to find out why a scan fell back, and this PR adds three new entries to it: a transform sort key, a UUID sort key, and sort-merge disabled while Iceberg still reports an ordering. Could we add those? The ### Tuning section above already documents dataFileConcurrencyLimit and looks like the right home for sortMerge.enabled and sortMerge.maxFilesPerPartition, including the note that this only does anything with Iceberg's spark.sql.iceberg.planning.preserve-data-ordering turned on. configs.md is generated so that one is fine.

[andygrove]

This returns Set.empty when the reflection fails, which the callers read as "no unsafe columns", so they go on to report the ordering. That's the fail-open direction on a decision where being wrong means silently mis-ordered rows, because Spark has already dropped the Sort by the time we get here.

The premise of this method is that a UUID sort key is invisible at the Spark type level. If we couldn't read the schema, we can't rule one out. Could the failure path make the ordering unreportable instead? Returning something like None and having reportableOrdering treat that as "refuse" would keep the safe direction the default.

The nativeIcebergScanMetadata == null branch in CometIcebergNativeScanExec.outputOrdering has the same shape.

[parthchandra]

Agreed. orderingUnsafeColumns now returns None when the schema can't be read, and the caller treats None as "refuse the ordering" — a reflection failure makes it un-reportable instead of assuming no UUID. The nativeIcebergScanMetadata == null branch is gone too; see the single-evaluation change below.

[andygrove]

Thanks for collapsing the proto and outputOrdering down to one evaluation. I think this guard reintroduces the same hazard one level up, though.

This is a second evaluation of reportableOrdering, and it's the one that decides whether Comet converts the scan at all. It reads COMET_ICEBERG_SORT_MERGE_ENABLED out of the active SQLConf and runs orderingUnsafeColumns reflection independently of the exec's lazy val. If this one says honorable, Comet converts and Spark drops the Sort. If the later one returns Nil, native reads unordered and we return wrong results with no error. The reverse mismatch just wastes some work.

You already have honorable right here. Could we stash it on nativeIcebergScanMetadata and have CometIcebergNativeScanExec.outputOrdering read that instead of re-running the gate? Then there's one evaluation for the whole decision, and no conf read after the point where Spark has committed to dropping the sort.

[parthchandra]

Good point. Changes so we evaluate the gate once and save the result on nativeIcebergScanMetadata; outputOrdering and the proto serde both read that field.

[andygrove]

The doc says "When disabled, files are read unordered as before", but with the new orderingHonored guard that isn't what happens. Disabling this makes reportableOrdering return Nil, which makes orderingHonored false, which keeps the scan on Spark entirely. Your own two fallback: sort-merge disabled ... tests assert exactly that. So the escape hatch for a merge bug also costs all Comet acceleration on every sorted Iceberg table, which is a steep price for the knob people would reach for when something looks wrong.

I don't think it has to work that way. maxFilesPerPartition = 0 already gives us "report the ordering, never merge, honor it with the native SortExec", which is correct and keeps Comet on the scan. Could sortMerge.enabled=false mean that instead? Either way the doc needs to describe the actual behavior.

[parthchandra]

You're right, doing what you suggest: enabled=false now behaves like maxFilesPerPartition=0 — the scan stays native and still honors the reported order via the spillable SortExec, it just skips the k-way merge. The two disabled-fallback tests are now "stays native" tests.

[andygrove]

data_file_concurrency_limit is the same kind of setting and it rides on IcebergScanCommon, set from CometConf in the serde, so there's exactly one default. This one goes through the Spark config map plus a SessionConfig extension with 64 written on both sides of JNI, and nothing catches it if the two drift.

Is there a reason not to put max_files_per_partition on IcebergScanCommon next to data_file_concurrency_limit? That removes the duplicated default and the extension plumbing in one go.

Related: both iceberg_scan_merges_when_files_within_limit and iceberg_scan_falls_back_to_sort_above_file_limit use PhysicalPlanner::default(), so they only ever test the compiled-in constant. If the extension weren't reaching the planner both would still pass, and the Scala test that sets the conf to 1000 would quietly exercise the sort fallback instead of the 70-way merge it's written to cover. Worth one test that builds the planner from a SessionConfig carrying IcebergSortMergeConfig { max_files_per_partition: 2 } and asserts a 3-file scan takes the SortExec path.

[parthchandra]

Moved max_files_per_partition onto IcebergScanCommon next to data_file_concurrency_limit, so there's one default (in CometConf) and removed the SessionConfig extension. Added iceberg_scan_honors_max_files_per_partition_from_proto, which sets the proto field to 2 and asserts a 3-file scan takes SortExec — so the value is actually read from the plan now, not the compiled-in constant

[andygrove]

This is the paragraph @mbutrovich and I were arguing with, and the code has moved on. It still says the fan-out "is intrinsic to a k-way merge" and "is the natural granularity for a sorted Iceberg table", with no mention that the planner now caps it at sortMerge.maxFilesPerPartition and drops to a spillable SortExec above that. Could you point it at the cap, and at #5343 for the bound-driven admission scheme? Otherwise the next person to read this concludes the fan-out is unbounded by design.

[parthchandra]

Comment updated

[parthchandra]

ocs/source/user-guide/latest/iceberg.md. The "Current limitations" list is where people look to find out why a scan fell back, and this PR adds three new entries to it: a transform sort key, a UUID sort key, and sort-merge disabled while Iceberg still reports an ordering. Could we add those? The ### Tuning section above already documents dataFileConcurrencyLimit and looks like the right home for sortMerge.enabled and sortMerge.maxFilesPerPartition, including the note that this only does anything with Iceberg's spark.sql.iceberg.planning.preserve-data-ordering turned on. configs.md is generated so that one is fine.

Added the transform-sort-key and UUID-sort-key fallbacks to "Current limitations", and documented sortMerge.enabled + sortMerge.maxFilesPerPartition under Tuning, including the preserve-data-ordering pre-requisite

[andygrove]

A few more from another pass over the current head:

On the require(protoOrders.forall(_.isDefined), ...) in serializePartitions — thanks for closing the silent-drop hole, but the failure mode is now an IllegalArgumentException at every executor task rather than a planning-time fallback to Spark. If the output binding ever drifts between CometScanRule.reportableOrdering (where the gate ran and stashed metadata.reportedOrdering) and this call — an aliasing pass, an exprId rewrite, something along those lines — a query that would previously have executed on Spark hard-fails at exec startup. Could this raise an unchecked exception higher up so the whole scan falls back cleanly, rather than require at task start?

Also, COMET_ICEBERG_SORT_MERGE_MAX_FILES_PER_PARTITION has no checkValue bound, and the proto field is uint32. A user passing -1 (or any negative int) casts to 4294967295 on the wire and the cap is effectively gone — a partition with tens of thousands of files opens all of them concurrently, which is exactly what the cap was added to prevent. The sibling COMET_ICEBERG_DATA_FILE_CONCURRENCY_LIMIT has .checkValue(v => v > 0, ...); want to add the same here?

Small one: the doc string for COMET_ICEBERG_SORT_MERGE_ENABLED reads "When disabled, the scan native and the ordering is still reported and honoured" — missing a verb after "scan". That description gets surfaced in config listings and generated docs.

[parthchandra]

Thanks Andy. Addressed all three -

On the require(protoOrders.forall(_.isDefined), ...) in serializePartitions — thanks for closing the silent-drop hole, but the failure mode is now an IllegalArgumentException at every executor task rather than a planning-time fallback to Spark. If the output binding ever drifts between CometScanRule.reportableOrdering (where the gate ran and stashed metadata.reportedOrdering) and this call — an aliasing pass, an exprId rewrite, something along those lines — a query that would previously have executed on Spark hard-fails at exec startup. Could this raise an unchecked exception higher up so the whole scan falls back cleanly, rather than require at task start?

good point — moved the binding to planning time. CometScanRule now binds the reported ordering to proto against the scan's output up front (serializeReportedOrdering), and if that fails it's just another reason orderingHonored is false, so the scan falls back to Spark during planning instead of hard-failing at exec startup. The serde now writes the already-bound protos, so the require is gone.

Also, COMET_ICEBERG_SORT_MERGE_MAX_FILES_PER_PARTITION has no checkValue bound, and the proto field is uint32. A user passing -1 (or any negative int) casts to 4294967295 on the wire and the cap is effectively gone — a partition with tens of thousands of files opens all of them concurrently, which is exactly what the cap was added to prevent. The sibling COMET_ICEBERG_DATA_FILE_CONCURRENCY_LIMIT has .checkValue(v => v > 0, ...); want to add the same here?

added .checkValue(v => v >= 0, ...), matching the sibling. 0 stays meaningful ("never merge, honor via sort"); negatives are now rejected instead of wrapping to 4294967295 and disabling the cap.

Small one: the doc string for COMET_ICEBERG_SORT_MERGE_ENABLED reads "When disabled, the scan native and the ordering is still reported and honoured" — missing a verb after "scan". That description gets surfaced in config listings and generated docs.

fixed — "the scan stays native"

[andygrove]

A few things from a pass over the current head. I ran the new suite locally against the published Iceberg (26 passed, 6 canceled, 0 failed) and spent most of the time on the native side, since that is where CI cannot reach today.

Three are inline. The fourth has no diff line to attach to: the PR description still describes the key-grouped partitioning reporting and spark.comet.scan.icebergNative.reportPartitioning.enabled, both of which went away when the outputPartitioning override was dropped, and it does not mention sortMerge.maxFilesPerPartition. Since this becomes the squash commit message, could you refresh it? The "Summary of changes" bullets are otherwise still accurate.

One thing I want to record as a positive, so nobody re-derives it: I probed the null ordering round-tripping through the proto in all four direction and null-ordering combinations, on both the merge path and the maxFilesPerPartition sort-fallback path, and it is correct in every one. Multi-column and descending orderings are fine too.

[andygrove]

execute_with_tasks builds a new ArrowReaderBuilder(...).build() on every call, a few lines below this one, and that is where iceberg-rust constructs the CachingDeleteFileLoader. Its delete_filter is the shared state the crate documents as existing "to allow caching loaded deletes across multiple calls to load_deletes (e.g., across multiple file scan tasks)". In the ordered path execute runs once per file, so that cache is now per-file, and a delete file shared across the partition gets downloaded and parsed once per data file instead of once for the whole partition. fill_delete_file_sizes dedups within a single call too, so it also issues one HEAD per data file for the same delete file now. At the default cap that is up to 64x on a merge-on-read table, and equality deletes are the worst case since they are always partition-scoped and the expensive ones to parse.

I measured it with two data files sharing one positional delete file. bytes_scanned goes from 2717 on the unordered path to 4256 on the ordered path, and the 1539-byte delta is exactly the delete file being read a second time. Rows are correct either way, so this is purely IO and CPU rather than a correctness problem.

The fix looks like the same shape as the file_io hoist on this line: hold one ArrowReader on IcebergScanExec and clone it per partition. It needs batch_size off the TaskContext, so it has to be a OnceLock filled on first execute rather than built in new, but ArrowReader is Clone, and read() calls ScanMetrics::new() per invocation and re-bases the loader's metrics through with_scan_metrics, so per-partition metrics stay separate while the delete cache is shared. With that prototype the ordered path drops back to 2717 with identical rows. Would you rather do it here or fold it into #5343?

[parthchandra]

Nice catch. Let me take care of this in #5343

[andygrove]

merge_ints and the two spm_merges_* tests below build a MemorySourceConfig and merge it, so they never touch IcebergScanExec, the planner or the proto. Every symbol they use predates this PR, which means they compile and pass verbatim on main. I checked with a mutation: changing IcebergScanExec::execute to let tasks = self.tasks.clone() so every partition reads every file leaves both of them green.

The useful part is that the gap looks closable without waiting on Iceberg. I tried writing probes that write real Parquet into a tempdir and drive the ordered path through IcebergScanExec and through create_plan, and the whole set runs in about 50ms. Merging three real files ascending, descending, and with duplicate keys across them gives tests that the mutation above does kill. Going through create_plan and asserting the merge path and the maxFilesPerPartition sort-fallback path produce identical output on the same data covers the proto to create_sort_expr to LexOrdering translation, which nothing exercises today. Round-tripping the null ordering through the proto in all four direction and null-ordering combinations covers ASC NULLS LAST and DESC NULLS FIRST, which are not Iceberg's defaults but are legal SortField values.

All of those pass on your branch, so this is a coverage gap rather than a bug report. Happy to hand you the patch if it saves you time.

[parthchandra]

Can you point me to your patch, I'll merge it in. Thank you!

[andygrove]

Sorry, this slipped through. The patch is one commit on top of your current head: andygrove@4f5fbee. curl -L https://github.com/andygrove/datafusion-comet/commit/4f5fbee3418cdac4f5d1ecae936403cc93e38703.patch | git am should pick it up. It applies cleanly at 1c5f95b, and fmt and clippy are clean.

The three tests in iceberg_scan.rs write real Parquet files and merge them through IcebergScanExec. The three in planner.rs go through create_plan: they check that the merge path and the maxFilesPerPartition sort fallback produce identical output, and that all four direction and null-ordering combinations survive the proto. On your current head, making every partition read every file fails all six while the existing tests stay green. Forcing Iceberg's default null ordering in the planner fails the null-ordering one.

There's also an ignored test for #6524 that asserts the shared delete file is read once. It fails today with the same 2717 vs 4256 bytes as the issue, so it can become the regression guard once that's fixed. Feel free to drop it if you'd rather keep this PR focused.

[andygrove]

I've pushed this to your branch as 987d3c6 instead, as you asked in #5331 (comment), so there's nothing to apply. The only differences from the patch above are the test names and a couple of comments.

[andygrove]

serializeReportedOrdering opens with the same if (reportedOrdering.isEmpty) Some(Nil) check that this branch does, so one of the two is unreachable. It has a single caller, so dropping the guard here reads a little better since the callee is the one that documents the contract.

[parthchandra]

Done.

[parthchandra]

the PR description still describes the key-grouped partitioning reporting and spark.comet.scan.icebergNative.reportPartitioning.enabled, both of which went away when the outputPartitioning override was dropped, and it does not mention sortMerge.maxFilesPerPartition. Since this becomes the squash commit message, could you refresh it? The "Summary of changes" bullets are otherwise still accurate.

Updated the PR description

[parthchandra]

@andygrove any more comments?

[andygrove]

I built Iceberg from apache/iceberg#16750 plus apache/iceberg#14948 and ran this suite against it with a probe at the native merge-or-sort decision. Every test on an unpartitioned table runs the old unordered read, because isOrderingEnabled in the Iceberg PR needs a non-empty grouping key type and Iceberg's own testNoMergeReaderForUnpartitionedSortedTable asserts exactly that. That covers "merges multiple sorted files", the duplicate-key, MOR-delete, descending, multi-column, single-file, on-versus-off and order-by-limit tests, and both 70-file tests. The above-the-cap sort fallback therefore has no end-to-end coverage at all.

I also ran two mutations. Making every ordered partition read every file was caught by the NULLS FIRST, NULLS LAST, partitioned-table, SMJ and group-by tests only. Flipping the merge direction so rows come out complete but mis-ordered was caught by the SMJ test alone, since every test with a global ORDER BY has its order repaired by Spark's final sort.

Could the merge tests move to the shape the NULLS tests already use, a table partitioned by a constant column with preserve-data-grouping on, and add an assumeOrderingReported after the correctness check so a reporting build proves the merge ran? For the 70-file tests the same change would make the cap boundary real. And for order sensitivity, a join, a window, or a SortAggregate over the merged input is what actually verifies the order, since ORDER BY cannot.

One thing to be aware of on the Iceberg side: with those two PRs as they stand, Spark's own answer is nondeterministically wrong. The window test failed for me with Spark returning row_number 2 for the first row, and a run with Comet's sort path untouched had Spark return 4 join rows where 6 is correct. Which revisions did you validate with?

[parthchandra]

Thanks for thanks for building the fork and measuring this. I verified this PR against against a personal build of Spark 3.4.3 + Iceberg build whose isOrderingEnabled is correct, which is why I didn't hit the nondeterminism you saw (your window/join getting wrong Spark answers is an Iceberg-side issue, not this PR).
You're also right that the merge tests were on unpartitioned tables. I've moved all of them to the partitioned-by-constant pattern with assumeOrderingReported after the correctness check, so a build that reports ordering proves the merge ran.
The above-the-cap sort fallback is now partitioned too. And the ascending/descending merge tests now verify order with a ROW_NUMBER() window (no ORDER BY to repair it), matching the existing window test.
PTAL.

[andygrove]

Another pass over the current head. Most of what I raised earlier has landed and reads well: the single evaluation of the gate, the planning-time proto binding, the checkValue bound, the maxFilesPerPartition cap, enabled=false meaning "sort instead of merge" rather than "give up the scan", and moving the merge tests onto partitioned tables with assumeOrderingReported plus a ROW_NUMBER() order check. Thanks for working through all of it.

Six things left. Five are inline. The first of those is new and is the one I care most about. Two are threads from earlier passes that never quite closed.

The sixth has no diff line to attach to. The branch is CONFLICTING. It is based on 4c2ab9686, from before #5933, and CometScanRule.scala has taken three commits on main since then. The conflict is with #5732, which removed complexTypePredicatesSupported from the same eligibility conjunction that this PR adds orderingHonored to, so the resolution is mechanical. #5732 also reworked IcebergReflection, though not the method this PR refactors, so that file merges clean. Worth re-running the Iceberg matrices after the merge rather than trusting the current green, since it was measured against a tree three weeks behind main.

[andygrove]

I went and read the Iceberg side to work out when orderingHonored actually fires, and I think it costs us the whole native scan in a case where it does not have to.

In apache/iceberg#16750, SparkPartitioningAwareScan.outputOrdering() returns Spark3Util.toOrdering(table().sortOrder()) whenever isOrderingEnabled() holds. SortOrderAnalyzer.canReportOrdering checks four things: the table has a sort order, the sort order has no nested fields, each partition key maps to exactly one task group, and every file carries the current sort order id. None of them looks at the projection. On the Spark side, V2ScanPartitioningAndOrdering.ordering resolves those references against relation, which is the unpruned relation, and unlike the partitioning branch directly above it there is no references.subsetOf(d.outputSet) guard.

So for SELECT data FROM sorted_table where the sort key is id, scanExec.ordering comes back non-empty carrying id's exprId, which is not in scanExec.output. isIdentityProjected fails, reportedOrdering is Nil, orderingHonored is false, and the whole Iceberg scan goes back to Spark.

I do not think that buys any safety. A required ordering can only reference attributes in the scan's output, and SortOrder.orderingSatisfies matches position by position starting at index 0, so an ordering whose first field is an unprojected attribute satisfies nothing above the scan. Spark has dropped no Sort, and reading unordered would be correct.

Could we keep the scan native when the first reported sort field references an attribute outside scanExec.output, and report no ordering in that case? The general version is to honour the longest prefix of identity-projected fields, since that is exactly as far as any satisfiable required ordering can reach. Declining is still the right answer when the blocking field is a projected UUID, because a required ordering can reach into it, so the UUID path would not change.

This matters because it is the common shape. Once preserve-data-ordering is on, any query on a sorted table that does not select every sort key loses the native scan. The follow-up bullet in the PR description still describes this case as "we skip the merge and read unordered", which was true before the orderingHonored gate went in.

[parthchandra]

Good catch, fixed. When the sort key isn't one of the selected columns, we no longer send the whole scan back to Spark. We keep the native scan and simply report no ordering (nothing above the scan can ask for an order on a column that isn't selected, so Spark hasn't dropped a sort). We still report the longest run of sort columns we can honour, and we only fall back to Spark when a selected column is unsafe — a UUID or a transform — or when we can't read the table schema. See CometScanRule.scala:1000-1021 and the new orderingDecision in CometIcebergNativeScan.scala:1000.

[andygrove]

Following on from the comment on CometScanRule, I do not think this test pins the behaviour it is named for.

val (_, plan) = checkSparkAnswer(s"SELECT c2 FROM $cat.db.t WHERE c3 = 'P1'")
nativeScans(plan).foreach { scan =>
  assert(scan.outputOrdering.isEmpty, ...)
}

On a reporting build this query falls back to Spark, so nativeScans(plan) is empty and the foreach asserts nothing. On the published Iceberg nothing was reported in the first place, so the assertion is trivially true there too. Either way it passes, and the title says "falls back to an unordered read" while the behaviour is a fallback to Spark.

Could this assert the outcome directly, whichever one we settle on above? If the scan should stay native with no ordering, assert(nativeScans(plan).nonEmpty) first and then the outputOrdering.isEmpty check. If it should stay a Spark fallback, assertFellBackToSpark already exists and says so.

[parthchandra]

You're right, the old test asserted nothing. Rewritten to check the real outcome: it now asserts the scan stays native and reports no ordering (CometIcebergSortMergeReadSuite.scala:643). I also added unit tests for the decision itself — stays-native for an unselected key, falls back for a selected UUID/transform, and the "unselected key before a UUID still stays native" case — starting at line 138.

[andygrove]

The transform sort key and the UUID sort key landing here is exactly what I was after last round, thank you.

The unprojected sort key belongs in this list too, since as things stand today that is also a fallback to Spark and it will be the most common one people hit. Whatever we settle on in the CometScanRule comment, the list should describe it.

[andygrove]

This docstring explains the choice of scanExec.ordering over scanExec.outputOrdering with "Spark blanks outputOrdering when a partition holds more than one file", and I think that has the mechanism backwards in a way worth fixing, because this is the safety-critical read.

Spark blanks it when multiple InputPartitions share a partition key, not when a partition holds multiple files. DataSourceV2ScanExecBase.outputOrdering is ordering.filter(_ => groupedPartitions.forall(_.groupedParts.forall(_.parts.length <= 1))). Iceberg's SortOrderAnalyzer.hasUniquePartitionKeys refuses to report in exactly that same case, with a comment giving the same reason. So on the Iceberg path the two values are always equal, and the many-files-in-one-partition case this merge targets is one where Spark does not blank at all.

That means reading outputOrdering instead would be behaviour-preserving today, and it would stop us depending on an Iceberg-side invariant to stay safe. If a future Iceberg relaxes hasUniquePartitionKeys, or another source reports an ordering Spark then blanks, reading ordering has us pay a k-way merge, or a full spillable SortExec above the cap, underneath a Sort that Spark kept anyway. Would you rather switch the read, or keep ordering and have the comment name the Iceberg check it is leaning on?

[parthchandra]

Fixed the comment.

[andygrove]

Picking this thread back up. merge_ints and the two spm_merges_* tests below it still build a MemorySourceConfig and merge it, so they never touch IcebergScanExec, the planner, or the proto, and they still compile and pass verbatim on main. The mutation I described holds: changing IcebergScanExec::execute to let tasks = self.tasks.clone() so every partition reads every file leaves both green.

The three planner tests added alongside them are real coverage of the merge-versus-sort decision and of the cap arriving over the proto, so the gap is narrower than it was. What is still uncovered is the data path, reading actual sorted files through IcebergScanExec and checking what comes out.

You asked me for the patch and I dropped it, sorry about that. The shape is three real Parquet files written into a tempdir and driven through IcebergScanExec, ascending, descending, and with duplicate keys across files, then the same data through create_plan asserting the merge path and the maxFilesPerPartition sort path agree, plus the null ordering round-tripped through the proto in all four direction and null-ordering combinations. It runs in about 50ms and the mutation above kills it. Say the word and I will push it to your branch.

[parthchandra]

Yes please — go ahead and push that test to the branch. I'd rather take your version that you've already measured than reproduce it. Thank you!

[andygrove]

Sorry for the slow turnaround. It's pushed to your branch as 987d3c6, on top of 1c5f95b. Three tests in iceberg_scan.rs write real Parquet files and merge them through IcebergScanExec: ascending, descending, and with duplicate keys across files. Three in planner.rs go through create_plan. They check that the merge path and the maxFilesPerPartition sort fallback return the same rows, and that all four direction and null-ordering combinations survive the proto. Making every partition read every file fails all six, and the existing tests stay green.

There's also an ignored test for #6524 that asserts the shared delete file is read once. Once that's fixed it can become the regression guard. Feel free to drop it if you'd rather keep this PR focused.

[andygrove]

On the per-file CachingDeleteFileLoader, you said you would fold this into #5343. Reading that issue, its body is entirely about bounding how many files the merge opens at once, and it does not mention the delete loader being rebuilt per execute, or the measurement: bytes_scanned going from 2717 to 4256 with two data files sharing one positional delete file, and up to 64x on a merge-on-read table at the default cap.

Could you add it to #5343, or file it separately? Once this lands, the shape of the regression is not recoverable from the code without redoing the measurement.

[parthchandra]

Fair point. Logged a new issue for this - #6524

[sunchao]

Summary

• Prior state and problem: Native Iceberg scans discarded file ordering, preventing consumers from safely exploiting sorted tables.
• Design approach: Carry reported ordering through Scala and protobuf, then merge sorted file streams within each Spark partition.
• Correctness / compatibility analysis: Found one new P1 issue with composite floating-point sort keys. Compared ordering semantics across Spark 3.4.3, 3.5.9, 4.0.4, 4.1.3 and the current 4.2 development source, plus Iceberg’s comparators and ordering-reporting implementation.
• Key design decisions: Planning-time binding keeps advertised and serialized ordering aligned. Reusing SortPreservingMergeExec and SortExec keeps the design straightforward.
• Implementation sketch: Each file becomes a native input partition below the merge. Above the configured cap, the scan reads unordered and uses a spillable sort.
• Behavioral changes worth calling out: Disabling merging still preserves ordering through sorting. Existing concerns about unnecessary Spark fallback for unprojected sort keys and repeated loading of shared delete files remain substantiated and unresolved.
• Suggested improvements: Route unsafe composite floating-point orderings through the existing full-sort path and add the reproduced signed-zero case as a regression test. Address the existing projection fallback and delete-cache concerns without duplicating those threads.

Reviewed the entire base-relative diff from 4c2ab9686526f30bc782462353e3dc97e3954b3f to 53d50136507a44e5d8c941f55b1852c8c4d34bab. The PR is not a draft. Applied .ai/skills/review-comet-pr/SKILL.md; no sibling skill applies.

Exact-head CI: 55 successful checks and 9 skips, with no failures. Rust CI passed 1,458 tests. The new Scala suite passed 17 tests and canceled 15 because the published Iceberg build does not report ordering.

Local validation: All 13 focused native scan/planner tests passed with --no-default-features. A disposable test using real Parquet files reproduced the finding through the native planner and scan. The default-feature build was blocked by missing jni.h; no local JVM integration run against an ordering-reporting Iceberg build was performed. Temporary source instrumentation was removed, and the checkout is clean.

[sunchao]

[P1] Use the full-sort path when a floating-point key precedes another sort key. With reported ordering (a DOUBLE ASC, b INT ASC), an Iceberg-sorted file can contain (-0.0, 2), (+0.0, 1): Iceberg's comparator distinguishes the two zeros. However, create_sort_expr normalizes them to the same value, so Spark requires the row with b = 1 first. This branch selects SortPreservingMergeExec despite its inputs being unsorted under that composite comparator, and the merge preserves the incorrect order. This can change query results: with multiple Spark partitions, CometTakeOrderedAndProjectExec trusts the newly advertised ordering and takes a local prefix for ORDER BY a, b LIMIT 2, potentially discarding the correct row before the final sort. Route these orderings to the existing spillable SortExec unless Spark-compatible ordering within every file can be established.

Evidence: A disposable native test wrote two real Parquet files containing [(-0.0, 2), (+0.0, 1)] and [(-1.0, 0), (1.0, 0)], then constructed an Iceberg scan protobuf ordered by both columns and executed it through PhysicalPlanner::create_plan. With max_files_per_partition = 64, the actual output was [(-1.0, 0), (-0.0, 2), (+0.0, 1), (1.0, 0)]. With the same files and cap 0, SortExec produced [(-1.0, 0), (+0.0, 1), (-0.0, 2), (1.0, 0)]. The equality assertion failed reproducibly. Iceberg Comparators uses Comparator.naturalOrder() for doubles, while Spark SQLOrderingUtil.compareDoubles treats signed zeros as equal across the checked versions. Probe source and output remain at /tmp/comet-5331-probe/native_test_fragment.rs and /tmp/comet-5331-real-files-probe.log.

[parthchandra]

Fixed. When a float or double column is a sort key but is not the last one, we no longer do the streaming merge — we read the files unordered and run the spillable sort instead, which sorts the way Spark does (treating -0.0 and +0.0 as equal). That removes the case where the merge trusted Iceberg's file order (which keeps -0.0 before +0.0) and produced a wrong result for things like ORDER BY ... LIMIT. A float/double that is the only key, or the last key, is still safe, so those still take the merge. See the decision at planner.rs:1898-1908, and three new plan tests: leading float → sort, trailing float → merge, single float → merge.

[andygrove]

This is a light fully automated review since there are so many PRs open.

1. The new paragraph in iceberg.md says a merge opens one reader per file, but docs/source/user-guide/latest/tuning.md:212 still says the native Iceberg scan reads each task's data files one at a time by default, and the dataFileConcurrencyLimit doc at spark/src/main/scala/org/apache/comet/CometConf.scala:178 describes it as the number of files read concurrently within a task. Neither holds on the merge path. Each file is its own partition under the SortPreservingMergeExec, so a task has up to sortMerge.maxFilesPerPartition (64 by default) readers open at once, whatever dataFileConcurrencyLimit is set to. Someone lowering that limit to cap scan memory on a sorted table would be turning the wrong knob. Could both say the limit only bounds the unordered read, and point at sortMerge.maxFilesPerPartition for the merge?

2. Two comments still describe earlier revisions. native/core/src/execution/planner.rs:1852 says table_sort_orders is empty unless sortMerge is on, but the ordering is bound in CometScanRule and written by the serde regardless of that flag, and enabled=false only sets max_files_per_partition to 0. That is what keeps the disabled case correct, since an empty list there would mean an unordered read under a Sort that Spark has already dropped. The reportableOrdering scaladoc at spark/src/main/scala/org/apache/comet/serde/operator/CometIcebergNativeScan.scala:899 and :906 still says two callers share the gate and that a transform key falls through to Nil and we read unordered. Today CometScanRule is the only caller, and Nil with a reported ordering keeps the scan on Spark. Could these be brought in line, so nobody later changes the serde to match the planner comment and stops sending the ordering when the merge is off?

[sunchao]

Summary

• Prior state and problem: Native Iceberg scans discarded file ordering, preventing safe use of sorted input by downstream operators.
• Design approach: Carry Iceberg’s reported ordering through Scala and protobuf, then merge sorted file streams within each Spark partition.
• Correctness / compatibility analysis: No additional introduced P1/P2 issues found within this review. The existing P1 composite floating-point ordering issue remains reproducible. Iceberg can order (-0.0, 2) before (+0.0, 1), while Spark treats the zeros as equal and requires the opposite order on the second key. The merge does not repair this, potentially producing incorrect top-K results. Checked relevant Spark sources for 3.4.3, 3.5.9, 4.0.4, 4.1.3 and current 4.2 development, plus Iceberg’s comparators and ordering reporter.
• Key design decisions: Planning-time binding aligns advertised and serialized ordering. Reusing SortPreservingMergeExec and SortExec keeps the implementation straightforward without introducing a custom merge abstraction.
• Implementation sketch: Each file becomes a native input partition beneath the merge. Above maxFilesPerPartition, the planner uses an unordered scan followed by a spillable sort.
• Behavioral changes worth calling out: Disabling merging still preserves ordering through sorting. The existing unprojected-key fallback and repeated shared-delete loading concerns remain substantiated and unresolved. The latter rebuilds the delete cache per file, retaining the documented I/O regression.
• Suggested improvements: Resolve the existing threads by sorting unsafe composite floating-point orderings, preserving native scans where unprojected ordering cannot satisfy a consumer, and sharing delete-loading state across file streams.

Reviewed all 12 changed files across the full diff from 4c2ab9686526f30bc782462353e3dc97e3954b3f to 53d50136507a44e5d8c941f55b1852c8c4d34bab. The PR is not a draft. Read existing discussions and applied .ai/skills/review-comet-pr/SKILL.md. No sibling skill applies.

Exact-head CI: 55 successful checks, 9 skips, no failures. Rust CI reports 1,458 passed tests and 5 skips. The new Scala suite passed 17 tests and canceled 15 because published Iceberg does not report ordering.

Validation: Reused recorded exact-head evidence for 13 passing focused native tests with --no-default-features and the real-Parquet reproduction, verifying the saved planner matches this checkout. Reran the saved bounded merge probe and confirmed the signed-zero ordering mismatch. No fresh full native/JVM build or integration run with an ordering-reporting Iceberg build was performed in this pass. The checkout remains clean. No GitHub state was changed.

[parthchandra]

This is a light fully automated review since there are so many PRs open.

1. The new paragraph in iceberg.md says a merge opens one reader per file, but docs/source/user-guide/latest/tuning.md:212 still says the native Iceberg scan reads each task's data files one at a time by default, and the dataFileConcurrencyLimit doc at spark/src/main/scala/org/apache/comet/CometConf.scala:178 describes it as the number of files read concurrently within a task. Neither holds on the merge path. Each file is its own partition under the SortPreservingMergeExec, so a task has up to sortMerge.maxFilesPerPartition (64 by default) readers open at once, whatever dataFileConcurrencyLimit is set to. Someone lowering that limit to cap scan memory on a sorted table would be turning the wrong knob. Could both say the limit only bounds the unordered read, and point at sortMerge.maxFilesPerPartition for the merge?
2. Two comments still describe earlier revisions. native/core/src/execution/planner.rs:1852 says table_sort_orders is empty unless sortMerge is on, but the ordering is bound in CometScanRule and written by the serde regardless of that flag, and enabled=false only sets max_files_per_partition to 0. That is what keeps the disabled case correct, since an empty list there would mean an unordered read under a Sort that Spark has already dropped. The reportableOrdering scaladoc at spark/src/main/scala/org/apache/comet/serde/operator/CometIcebergNativeScan.scala:899 and :906 still says two callers share the gate and that a transform key falls through to Nil and we read unordered. Today CometScanRule is the only caller, and Nil with a reported ordering keeps the scan on Spark. Could these be brought in line, so nobody later changes the serde to match the planner comment and stops sending the ordering when the merge is off?

Both fixed. (1) The dataFileConcurrencyLimit docs now say it only bounds the unordered read, and point to sortMerge.maxFilesPerPartition for the merge path, since on the merge each file is its own reader regardless of that limit — CometConf.scala:179 and tuning.md:350. (2) Brought the stale comments in line: planner.rs now says the sort order is written regardless of the sortMerge.enabled flag (disabling just sets the cap to 0), and the reportableOrdering comment now says there's one caller and that an empty result with a reported ordering keeps the scan on Spark.

[parthchandra]

Summary

• Prior state and problem: Native Iceberg scans discarded file ordering, preventing safe use of sorted input by downstream operators.
• Design approach: Carry Iceberg’s reported ordering through Scala and protobuf, then merge sorted file streams within each Spark partition.
• Correctness / compatibility analysis: No additional introduced P1/P2 issues found within this review. The existing P1 composite floating-point ordering issue remains reproducible. Iceberg can order (-0.0, 2) before (+0.0, 1), while Spark treats the zeros as equal and requires the opposite order on the second key. The merge does not repair this, potentially producing incorrect top-K results. Checked relevant Spark sources for 3.4.3, 3.5.9, 4.0.4, 4.1.3 and current 4.2 development, plus Iceberg’s comparators and ordering reporter.
• Key design decisions: Planning-time binding aligns advertised and serialized ordering. Reusing SortPreservingMergeExec and SortExec keeps the implementation straightforward without introducing a custom merge abstraction.
• Implementation sketch: Each file becomes a native input partition beneath the merge. Above maxFilesPerPartition, the planner uses an unordered scan followed by a spillable sort.
• Behavioral changes worth calling out: Disabling merging still preserves ordering through sorting. The existing unprojected-key fallback and repeated shared-delete loading concerns remain substantiated and unresolved. The latter rebuilds the delete cache per file, retaining the documented I/O regression.
• Suggested improvements: Resolve the existing threads by sorting unsafe composite floating-point orderings, preserving native scans where unprojected ordering cannot satisfy a consumer, and sharing delete-loading state across file streams.

Reviewed all 12 changed files across the full diff from 4c2ab9686526f30bc782462353e3dc97e3954b3f to 53d50136507a44e5d8c941f55b1852c8c4d34bab. The PR is not a draft. Read existing discussions and applied .ai/skills/review-comet-pr/SKILL.md. No sibling skill applies.

Exact-head CI: 55 successful checks, 9 skips, no failures. Rust CI reports 1,458 passed tests and 5 skips. The new Scala suite passed 17 tests and canceled 15 because published Iceberg does not report ordering.

Validation: Reused recorded exact-head evidence for 13 passing focused native tests with --no-default-features and the real-Parquet reproduction, verifying the saved planner matches this checkout. Reran the saved bounded merge probe and confirmed the signed-zero ordering mismatch. No fresh full native/JVM build or integration run with an ordering-reporting Iceberg build was performed in this pass. The checkout remains clean. No GitHub state was changed.

Thanks @sunchao . Status on the three open items: the floating-point composite sort key is fixed (see the planner.rs thread — it now takes the full sort path). The unprojected sort key no longer forces a Spark fallback — the scan stays native and just doesn't report an order (see the CometScanRule thread). The repeated delete-file loading now has its own issue, #6524, so it isn't lost.

[sunchao]

Summary

• Prior state and problem: Native Iceberg scans discarded file ordering, preventing consumers from safely relying on sorted input.
• Design approach: Carry reported ordering through Scala and protobuf, then merge sorted file streams within each Spark partition.
• Correctness / compatibility analysis: No additional introduced P1/P2 issues found within this review. The earlier composite floating-point ordering and unprojected-key fallback concerns are addressed. Compared relevant Spark sources across 3.4.3, 3.5.9, 4.0.4, 4.1.3 and 4.2.0, plus Iceberg comparators and ordering reporting.
• Key design decisions: Planning-time binding keeps advertised and serialized ordering aligned. Reusing SortPreservingMergeExec and SortExec avoids a custom merge implementation.
• Implementation sketch: Each file becomes a native input partition beneath the merge. Above the configured file cap, or for unsafe composite floating-point orderings, an unordered scan feeds a spillable sort.
• Behavioral changes worth calling out: Disabling merging still honors ordering through sorting. The existing shared-delete loading regression remains unresolved. Each file stream still constructs its own ArrowReader and delete cache, repeating shared-delete reads and stat calls. The recorded two-file reproduction increased bytes_scanned from 2,717 to 4,256. Filing #6524 tracks this P2 regression but does not fix it.
• Suggested improvements: Resolve that existing performance blocker before merge by sharing reader/delete-cache state and deduplicating delete-file stat requests across file streams.

Reviewed full PR scope at 1c5f95b8b8754a28430148c9f79a49f17353282b against base 9d3fb37bb2594b4de4a865a77500fab94dd9dfb9. The PR is not a draft. Examined the 31-file direct comparison and verified that 18 extra files reflect newer base commits, not PR changes.

Routed skills: review-comet-pr, review-comet-expression-pr, review-comet-memory-pr, and review-comet-iceberg-write-pr under .ai/skills/.

Exact-head CI: 57 successful checks, 11 skips, no failures. All four Iceberg matrices passed. Rust CI passed 1,941 tests with 5 skips. The new Scala suite passed 25 tests and canceled 15 because published Iceberg does not report ordering. Spark SQL matrices and macOS were skipped.

Local validation: 18 focused native tests passed with --no-default-features, including a disposable real-Parquet probe confirming the signed-zero fix and a safe trailing-float merge. The default-feature build failed because jni.h is unavailable. No local JVM integration run against an ordering-reporting Iceberg build was performed. Temporary source changes were removed, and the checkout is clean. No GitHub state was changed.