Skip to content

[AURON #2229] bugfix Silent fallback to empty partitions may cause data loss #2230

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
guixiaowen wants to merge 5 commits into
base: master
Choose a base branch
from
+11 −2
Open

[AURON #2229] bugfix Silent fallback to empty partitions may cause data loss #2230

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
12d3b91
[AURON #2229] Silent fallback to empty partitions may cause data loss…
May 3, 2026
d90aae5
[AURON #2229] Silent fallback to empty partitions may cause data loss
May 3, 2026
b22d938
[AURON #2229] Silent fallback to empty partitions may cause data loss
May 6, 2026
b21d100
[AURON #2229] Silent fallback to empty partitions may cause data loss
May 6, 2026
673d101
[AURON #2229] Silent fallback to empty partitions may cause data loss
May 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions .../auron-iceberg/src/main/scala/org/apache/spark/sql/auron/iceberg/IcebergScanSupport.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,14 @@ object IcebergScanSupport extends Logging {
return None
}

val partitions = inputPartitions(exec)
val partitions =
try {
inputPartitions(exec)
} catch {
case e: Throwable =>
logWarning(s"Get Partition error: ${e.getMessage}")
return None
}
Comment on lines +82 to +88

@weiqingy weiqingy Jun 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This catch converts the IllegalStateException thrown at L200 into return None, so on a planning failure the effective behavior is a fallback to Spark's scan path (plus two WARN logs) rather than the "fail fast with a clear exception" the description mentions — no exception reaches the caller. Falling back to Spark is arguably the safer outcome, since the query still returns correct results, so this may well be what you want — worth confirming it's intentional rather than a hard failure.

If Spark-fallback is the intent, the throw-at-L200-then-catch-here round-trip is using an exception purely for control flow. Would signaling failure through the return type read more directly — e.g. inputPartitions returning Option[Seq[InputPartition]], with None = planning failed and Some(empty) = genuinely empty table, and plan() matching on it? That also answers the question on the other thread about whether this catch is removable: it isn't — dropping it would route a failure back into the empty-partition branch and reintroduce the empty-plan case.

Minor: this catches Throwable, while the sibling catch at L196 uses NonFatal. Matching NonFatal here would avoid folding fatal errors like OutOfMemoryError into a silent Spark fallback.

// Empty scan (e.g. empty table) should still build a plan to return no rows.
if (partitions.isEmpty) {
logWarning(s"Native Iceberg scan planned with empty partitions for $scanClassName.")
Expand Down Expand Up @@ -190,7 +197,9 @@ object IcebergScanSupport extends Logging {
logWarning(
s"Failed to obtain input partitions via reflection for ${exec.getClass.getName}.",
t)
Comment on lines 197 to 199

@weiqingy weiqingy Jun 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This catch now throws on a reflective failure, which is what routes plan() to the Spark fallback. The V2 batch path earlier in this method (the exec.scan.toBatch / planInputPartitions() block, ~L158-165) still catches Throwable and returns Seq.empty, then falls through to reflection. So if V2 planning fails for a real reason — an fs or metadata error — that failure is masked, and if reflection then yields empty, plan() builds a native scan with empty partitions: the same silent-empty outcome #2229 is about, one level up. Is the V2 swallow-to-empty intentional, or should a V2 planning failure surface the same way this one now does?

Seq.empty
throw new IllegalStateException(

@ShreyeshArangath ShreyeshArangath May 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are throwing an exception anyways, do we need to catch it in L189?

s"Cannot resolve input partitions for ${exec.getClass.getName}",
t)
}
}

Expand Down
Loading