fix: [spark34] preserve empty Iceberg partitions after dynamic pruning - #6814

From reading Spark 3.4.3 BatchScanExec.inputRDD, preserving the one empty partition matches what Spark runs, and the native early return looks consistent with it. One inline comment on the credential test. The aggregate regression only reproduces on Spark 3.4 and the PR tier runs the default profile, so could a maintainer add run-all-spark-profiles to get CometIcebergNativeSuite run on 3.4 before merge?

Reviewed the full three-file diff from 00a4b422f0ed6560ef76dce004b94c3697613108 to db5e06cfaf0f2955b9c171e60ed59e760956bebd, including the original fix and test revision. The PR is not a draft.

Summary

• Prior state and problem: When DPP removes the only Iceberg split, Spark 3.4 preserves one empty execution partition. Comet discarded it, so a global aggregate without an exchange never ran and returned no rows instead of (0, NULL) for COUNT(*) and SUM(amount).
• Design approach: Preserve the partitions supplied by Spark, serialize an empty scan for each, and return a schema-bearing empty native stream before initializing storage.
• Correctness: Spark 3.4.3 source confirms the SinglePartition optimization and the empty partition in BatchScanExec.inputRDD. HashAggregateExec, Count, and Sum confirm the expected aggregate result. I traced the empty protobuf through common-data injection, native planning, and DataFusion's EmptyRecordBatchStream. The required schema survives and no file tasks are introduced. No introduced P1/P2 correctness issue was identified.
• Compatibility analysis: Checked the relevant scan sources for supported Spark profiles 3.4.3, 3.5.9, 4.0.4, 4.1.3, and 4.2.0. The newer profiles no longer infer SinglePartition from a single input split, explaining the Spark 3.4-specific regression. Following Spark's actual execution partitions works without new version branches or API changes.
• Key design decisions: Using other.partitions preserves Spark's execution contract without re-planning Iceberg's already-filtered scan. Checking tasks.is_empty() before load_file_io also prevents unnecessary executor-side policy-location requests.
• Implementation sketch: serializePartitions appends default IcebergScan messages for the preserved partitions. Existing injection supplies their common schema and configuration. execute_with_tasks returns EmptyRecordBatchStream for zero tasks. Two aggregate regressions cover AQE on and off, and a direct native execution test checks zero rows and zero policy-location calls.
• Performance: The empty path avoids storage construction, credential-provider interaction, and reader initialization. The nonempty path adds a constant-time emptiness check. Preserving the execution task is necessary for aggregate semantics. No benchmark was run, and no evidence-backed performance regression was found.
• Design: The fix addresses the partition-count mismatch at serialization and handles empty execution locally in the scan. It preserves the existing DPP lifecycle and keeps the change small and easy to follow.
• Abstraction & complexity: Existing protobuf messages, common-data injection, and DataFusion's standard empty stream provide the required behavior. No additional abstraction is introduced. The revised credential test uses the existing provider's public counters and removes the reflection discussed in the earlier review.
• Behavioral changes worth calling out: Compared with branch-1.1 at e9efd9f764ee0a59b7898ff028d6985d4a7a28e1, the intended changes are correct empty global-aggregate results on Spark 3.4 and no executor storage initialization for taskless scans. Other differences in the touched files were already present in the supplied base.
• Suggested improvements: No additional change meeting the P1/P2 reporting bar was identified. The earlier test-simplification feedback is addressed. The existing coverage discussion does not establish an unresolved correctness blocker.

Routed skills: review-comet-pr, review-comet-expression-pr for the serde path, and review-comet-ffi-pr for native stream and iterator behavior. Read AGENTS.md, relevant contributor guidance, and all supplied reviews, conversation comments, inline comments, and threads.

CI and validation: At the exact reviewed SHA, GitHub reports only a successful label check, with no build/test verdict. I independently inspected the fork Spark 3.4 run. Its SHA, 521d11df549deef1d6dbb31c35ed296e7cf74434, is a direct child of the reviewed head and adds only the workflow, so project code and tests are identical. Logs confirm a native build and CometIcebergNativeSuite on Spark 3.4, Scala 2.12, and JDK 17: 109 passed, zero failed, and 11 version-gated cancellations. All three new regressions passed.

Validation limits: No local native/JVM build or test execution was performed. This checkout has no built artifacts. Runtime results for the other Spark profiles and the separate upstream Spark SQL/Iceberg suites were not verified. git diff --check passed, and the working tree remains clean.

No introduced P1/P2 issues found within this review. No substantiated existing P1/P2 blockers remain.

I ran this locally on a merge with main. On Spark 3.4.3 the new tests pass with AQE on and off, along with some extra probes of other keyless aggregates and a storage-partitioned join. With the serde change reverted, both aggregate tests fail as they should. Reverting only the native guard fails just the policy-location test, so the guard is the storage and credential saving and the serde change is the correctness fix. The full CometIcebergNativeSuite also passes on the default 4.1 profile.

I left two inline comments. One asks for a test of a second symptom this PR also fixes, and the other is about the wording of the comment on the ParallelCollectionRDD branch.

Thanks for the fix and for the thorough tests. I re-checked the new join cases and the reworded comment on the ParallelCollectionRDD branch, and both address my earlier comments. Preserving Spark's empty execution partition matches what Spark 3.4 runs, and the native early return before load_file_io keeps the empty scan from touching storage or credentials. I have no further concerns.

On your CI question: yes, tagging a maintainer is the right thing to do. Your latest push was waiting on first-time-contributor approval, so I have approved the Comet CI and CodeQL runs. I also added run-all-spark-profiles so the suites run on every Spark profile, alongside the run-iceberg-tests and run-spark-3.4-tests labels that were already there. The earlier red run was a Maven bootstrap digest-mismatch in Spark 4.1 [expressions] and not related to your change. Re-running the failed jobs from the Actions page is the usual fix for that, and a maintainer can do it for you.

Approving on the basis of the code and the fork CI results. Please keep an eye on the new runs and ping a maintainer if anything fails for an unrelated reason.