Skip to content

fix: qualify Iceberg names in public error macros - #1026

Open
manuzhang wants to merge 4 commits into
apache:mainfrom
manuzhang:qualify-macro-names
Open

manuzhang wants to merge 4 commits into
apache:mainfrom
manuzhang:qualify-macro-names

Conversation

@manuzhang

Copy link
Copy Markdown
Member

What

  • Qualify the Iceberg names that ICEBERG_RETURN_UNEXPECTED, ICEBERG_ASSIGN_OR_RAISE, ICEBERG_PRECHECK, and ICEBERG_CHECK in iceberg/util/macros.h, and the ICEBERG_BUILDER_* macros in iceberg/util/error_collector.h, use with ::iceberg::.
  • Change the iceberg:: prefixes in the exception macros (ERROR_TO_EXCEPTION, used by ICEBERG_THROW_NOT_OK and ICEBERG_ASSIGN_OR_THROW) to ::iceberg::, as the logging macros already do.
  • Add error_macros_test.cc, which uses each of these macros from code outside namespace iceberg.

Why

The macros named Error, InvalidArgument, ValidationFailed, and ErrorKind without their namespace. Those names resolve only in code inside namespace iceberg, so an application that includes the installed headers could not use the macros. For example, ICEBERG_ASSIGN_OR_RAISE in an application failed with 'Error' was not declared in this scope.

Behavior change

Code outside namespace iceberg can now use these macros. Code inside namespace iceberg compiles and behaves as before, because the qualified names refer to the same functions and types.

Testing

  • util_test passes (419 tests), including the new ErrorMacrosTest cases for ICEBERG_ASSIGN_OR_RAISE, ICEBERG_RETURN_UNEXPECTED, ICEBERG_PRECHECK, ICEBERG_CHECK, ICEBERG_ASSIGN_OR_THROW, and the builder macros. The new test file does not compile against the previous headers.
  • metadata_table_test passes (10 tests), which also compiles the bundle library against the changed headers.

🤖 Generated with Claude Code

ICEBERG_RETURN_UNEXPECTED, and through it ICEBERG_ASSIGN_OR_RAISE, named
Error without its namespace. ICEBERG_PRECHECK and ICEBERG_CHECK did the
same with InvalidArgument and ValidationFailed, and the ICEBERG_BUILDER_*
macros with ErrorKind. Those names resolve only inside namespace iceberg,
so code that includes the installed headers could not use the macros.

Qualify them with ::iceberg::, as the logging macros already do, and use
::iceberg:: in the exception macros as well.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings October 16, 2026 10:20

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Comment thread src/iceberg/util/macros.h
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings October 20, 2026 00:25

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Comment thread src/iceberg/util/error_collector.h Outdated

@zhjwpku zhjwpku left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings October 20, 2026 15:14

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings October 20, 2026 15:23

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@manuzhang
manuzhang requested a review from zhjwpku October 20, 2026 15:43

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants