Skip to content

fix(sql): use signed 64-bit Iceberg snapshot IDs - #7590

Merged
srilman merged 5 commits into
Eventual-Inc:mainfrom
Solomon-mithra:fix/sql-iceberg-signed-snapshot-id
Oct 17, 2026
Merged

srilman merged 5 commits into
Eventual-Inc:mainfrom
Solomon-mithra:fix/sql-iceberg-signed-snapshot-id

Conversation

@Solomon-mithra

Copy link
Copy Markdown
Contributor

Changes Made

Use Option<i64> for SQL read_iceberg snapshot IDs and both scan-builder configurations, following the signedness decision in #7477. This removes pointer-width dependence and lets absent negative IDs reach Iceberg's snapshot lookup instead of failing unsigned-option parsing. Other unsigned table-function options retain their existing validation.

Add deterministic SQL/Python parity tests for IDs above the 32-bit range and at i64::MAX, negative missing-snapshot diagnostics, and invalid literal rejection. Document the ID type. The general SQL number parser is unchanged, including its existing exact-i64::MIN literal limitation.

Validation on macOS ARM64:

  • Before the fix, the new regression selection produced 4 expected failures in negative-ID cases and 7 passes.
  • After the fix, the SQL Iceberg, table-function and local Iceberg-read suites passed: 71 passed, 1 skipped.
  • Native extension build and cargo check -p daft-sql -p daft-logical-plan --no-default-features passed.
  • cargo fmt --all -- --check, pinned Ruff 0.14.10 lint/format on the changed Python file, and git diff --check passed.

The native build used a local -C strip=none workaround for macOS linker output rejection; no build configuration changes are included. No 32-bit, Ray or full-workspace CI run was performed locally.

AI disclosure: Codex generated the implementation and regression tests, reviewed the diff, and ran the baseline and post-fix checks above. A second agent independently reviewed the patch; test results described here are agent-run validation.

Related Issues

Closes #7477.

@Solomon-mithra
Solomon-mithra requested a review from a team as a code owner October 8, 2026 20:44
@github-actions github-actions Bot added the fix label Oct 8, 2026
@greptile-apps

greptile-apps Bot commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

[High impact] The PR appears safe to merge.

Summary

Changes SQL Iceberg snapshot IDs from usize to i64 in the table-function arguments and both scan-builder configurations.

  • Removes dependence on pointer width.
  • Lets negative IDs reach Iceberg's snapshot lookup.
  • Adds SQL/Python parity tests for wide IDs, missing negative IDs, and invalid literals.
  • Documents the signed 64-bit ID type.

No actionable issues were found. Solomon-mithra explicitly deferred the existing exact i64::MIN literal limitation.

Reviews (1) · Last reviewed commit: "fix(sql): use signed 64-bit Iceberg snap..." · Reviewed by Greptile

@srilman srilman left a comment

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.

LGTM, just one thing

Comment thread docs/sql/index.md Outdated
@srilman
srilman merged commit b8ec8c4 into Eventual-Inc:main Oct 17, 2026
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SQL read_iceberg types snapshot_id as usize instead of a 64-bit integer

2 participants