Skip to content

fix: preserve original error in IcebergIterator._seek_to_usable_file#5092

Open
mengw15 wants to merge 1 commit into
apache:mainfrom
mengw15:fix/5091-iceberg-iterator-preserve-error
Open

fix: preserve original error in IcebergIterator._seek_to_usable_file#5092
mengw15 wants to merge 1 commit into
apache:mainfrom
mengw15:fix/5091-iceberg-iterator-preserve-error

Conversation

@mengw15
Copy link
Copy Markdown
Contributor

@mengw15 mengw15 commented May 16, 2026

What changes were proposed in this PR?

IcebergIterator._seek_to_usable_file previously swallowed every error during file-scan setup:

except Exception:
    print("Could not read iceberg table:\n")
    raise Exception

The bare raise Exception (no args, no from) constructs a fresh Exception with empty str() and no __cause__. Callers that do except Exception as e: log.error(str(e)) see only an empty class name — the original error type, message, and traceback are all lost. The print also bypasses the project logger.

This PR replaces the bare re-raise with a true re-raise of the original exception and routes the diagnostic message through loguru, matching the existing except Exception as err: logger.exception(err) pattern used in data_processor.py:125, main_loop.py:422, and input_port_materialization_reader_runnable.py:169:

except Exception as err:
    logger.exception(err)
    raise

Callers now see the actual underlying exception (catalog auth failure, S3 IO error, manifest corruption, etc.) with its full class name, message, and traceback.

Any related issues, documentation, discussions?

Closes #5091.

How was this PR tested?

Added amber/src/test/python/core/storage/iceberg/test_iceberg_iterator_error_paths.py, a slim mocked regression test: it patches load_table_metadata to return a Mock whose refresh() raises RuntimeError("Catalog auth failure: token expired"), drives next(IcebergIterator(...)), and asserts the caller observes the original RuntimeError (via pytest.raises(RuntimeError, match=...)). Locks in the contract that the except clause must not swallow the underlying exception's type/message.

Run locally:

python -m pytest amber/src/test/python/core/storage/iceberg/test_iceberg_iterator_error_paths.py -v

Result: 1 passed.

Was this PR authored or co-authored using generative AI tooling?

Generated-by: Claude Code (claude-opus-4-7)

@mengw15 mengw15 marked this pull request as draft May 16, 2026 04:30
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 16, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 43.15%. Comparing base (e27b98a) to head (094d060).

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #5092   +/-   ##
=========================================
  Coverage     43.15%   43.15%           
  Complexity     2209     2209           
=========================================
  Files          1045     1045           
  Lines         40260    40261    +1     
  Branches       4250     4250           
=========================================
+ Hits          17373    17376    +3     
+ Misses        21815    21813    -2     
  Partials       1072     1072           
Flag Coverage Δ *Carryforward flag
access-control-service 39.53% <ø> (ø) Carriedforward from e27b98a
agent-service 33.72% <ø> (ø) Carriedforward from e27b98a
amber 43.80% <ø> (ø) Carriedforward from e27b98a
computing-unit-managing-service 0.00% <ø> (ø) Carriedforward from e27b98a
config-service 0.00% <ø> (ø) Carriedforward from e27b98a
file-service 32.18% <ø> (ø) Carriedforward from e27b98a
frontend 34.05% <ø> (ø) Carriedforward from e27b98a
python 90.49% <100.00%> (+0.05%) ⬆️
workflow-compiling-service 56.81% <ø> (ø) Carriedforward from e27b98a

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mengw15 mengw15 marked this pull request as ready for review May 16, 2026 04:36
@mengw15 mengw15 force-pushed the fix/5091-iceberg-iterator-preserve-error branch 2 times, most recently from 62aae9c to 33ecec5 Compare May 16, 2026 04:49
@mengw15 mengw15 requested a review from aglinxinyuan May 16, 2026 04:50
@aglinxinyuan aglinxinyuan requested a review from Copilot May 16, 2026 04:50
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates Iceberg table scan error handling so _seek_to_usable_file logs failures through Loguru and re-raises the original exception instead of replacing it with an empty generic Exception.

Changes:

  • Adds loguru.logger import to iceberg_document.py.
  • Replaces print(...) and raise Exception with logger.exception(err) and bare raise.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread amber/src/main/python/core/storage/iceberg/iceberg_document.py
The bare `raise Exception` (no args, no `from`) constructed a fresh
empty exception, discarding the underlying error type, message, and
traceback. Callers doing `except Exception as e: log.error(str(e))`
saw only an empty string instead of the real diagnostic (catalog auth
failure, S3 IO error, manifest corruption, etc.). The `print` also
bypassed the project logger.

Replace with a re-raise of the original exception and route the
message through loguru.

Closes apache#5091.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@mengw15 mengw15 force-pushed the fix/5091-iceberg-iterator-preserve-error branch from 33ecec5 to 094d060 Compare May 16, 2026 06:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

IcebergIterator._seek_to_usable_file swallows original error via bare raise Exception

3 participants