Fix GcsPinotFS null safety and PinotFS contract compliance by Akanksha-kedia · Pull Request #18360 · apache/pinot

Akanksha-kedia · 2026-04-28T13:52:17Z

Description

Fix several null-safety issues and PinotFS contract violations in GcsPinotFS. The GCS SDK's Storage.get(BlobId) returns null when a blob does not exist (unlike the S3 SDK which throws NoSuchKeyException), so callers must handle null explicitly.

Fixes #17714

Changes Made

1. `lastModified()` — return 0L for missing files

The PinotFS contract specifies that lastModified() should return 0L if the file does not exist or if an I/O error occurs. Previously this method called getBlob().getUpdateTime() without a null check, resulting in an NPE. Now returns 0L for missing blobs, consistent with LocalPinotFS.

2. `touch()` — create an empty file when the blob does not exist

The PinotFS contract specifies that touch() should create an empty file if the file does not exist. Previously this method assumed the blob exists and called blob.getUpdateTime() / blob.toBuilder(), resulting in an NPE. Now creates an empty blob for missing files, consistent with S3PinotFS and LocalPinotFS.

3. `open()` — throw a descriptive IOException for missing files

Previously called blob.reader() on a potentially null blob, resulting in an NPE with no context. Now throws a descriptive IOException that includes the URI.

4. `copyFile()` — null check on source blob and cleanup on failure

Previously called blob.copyTo() on a potentially null source blob. Also, a zero-byte destination blob is created before the copy starts — if the copy fails, this empty blob was left behind. Now guards against null source and cleans up the destination blob on failure.

5. Guarded all `getUpdateTime()` calls

BlobInfo.getUpdateTime() can return null for directory entries or newly created blobs. Added null guards on all call sites.

Testing Done

mvn compile passes for the pinot-gcs module
mvn spotless:apply — no formatting changes needed
mvn license:check passes

The existing GcsPinotFSTest is an integration test that requires GCS credentials (env vars GOOGLE_APPLICATION_CREDENTIALS, GCP_PROJECT, GCS_BUCKET). The null-safety fixes address runtime paths that are triggered when blobs don't exist — these are inherently guarded by the GCS SDK's behavior and are consistent with how S3PinotFS and LocalPinotFS handle the same cases.

codecov-commenter · 2026-04-28T14:33:58Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 63.69%. Comparing base (1881884) to head (c8ffd40).
⚠️ Report is 112 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #18360      +/-   ##
============================================
+ Coverage     63.40%   63.69%   +0.29%     
- Complexity     1679     1684       +5     
============================================
  Files          3253     3266      +13     
  Lines        198767   199836    +1069     
  Branches      30791    31023     +232     
============================================
+ Hits         126034   127293    +1259     
+ Misses        62659    62399     -260     
- Partials      10074    10144      +70

Flag	Coverage Δ
custom-integration1	`100.00% <ø> (ø)`
integration	`100.00% <ø> (ø)`
integration1	`100.00% <ø> (ø)`
integration2	`0.00% <ø> (?)`
java-21	`63.69% <ø> (+0.29%)`	⬆️
temurin	`63.69% <ø> (+0.29%)`	⬆️
unittests	`63.69% <ø> (+0.29%)`	⬆️
unittests1	`55.84% <ø> (+0.48%)`	⬆️
unittests2	`34.94% <ø> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. 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:

📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Akanksha-kedia · 2026-04-28T14:35:33Z

@xiangfu0 please review

noob-se7en

Lets add tests catching these exceptions.

Akanksha-kedia · 2026-05-05T10:45:48Z

@xiangfu0 @noob-se7en please review

Akanksha-kedia · 2026-05-05T11:12:38Z

@xiangfu0 @noob-se7en please review

Akanksha-kedia · 2026-05-10T09:18:13Z

@noob-se7en please review and merge

- lastModified(): return 0L when blob does not exist instead of throwing NPE, matching the PinotFS contract and LocalPinotFS behavior - touch(): create an empty file when the blob does not exist instead of throwing NPE, matching the PinotFS contract (S3PinotFS, LocalPinotFS) - open(): throw a descriptive IOException when the blob does not exist instead of throwing NPE from blob.reader() - copyFile(): guard against null source blob and clean up the empty destination blob on copy failure to prevent leaked zero-byte objects - Guard all getUpdateTime() calls against null return values Fixes apache#17714

…copy, fix deprecated getUpdateTime - Use FileNotFoundException (not IOException) in open() and copyFile() when a blob is missing, as FileNotFoundException is the standard Java exception for missing files and is a subtype of IOException so callers need not change - Drop String.format in exception messages (ref: apache#14404); use string concatenation - copyFile: use blob.copyTo(BlobId) directly instead of pre-creating an empty destination blob; this removes the redundant _storage.create() call, the cleanup try-catch, and the unnecessary blob.exists() call on the return path - Replace deprecated Blob.getUpdateTime() (returns Long) with Blob.getUpdateTimeOffsetDateTime() (returns OffsetDateTime) in lastModified(), touch(), and both listFilesWithMetadata() overloads - Add GcsPinotFSNullSafetyTest with unit tests verifying that open() and copyDir() throw FileNotFoundException (not NullPointerException) when blobs are missing

- Move java.time.OffsetDateTime import after java.net.URI to fix spotless import ordering violation - Replace TestNG assertThrows (returns void) with try-catch blocks so the caught FileNotFoundException message can be verified Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…t mocks The production code calls blob.getUpdateTimeOffsetDateTime() to populate FileMetadata.lastModifiedTime, but mockBlob() only stubbed getUpdateTime() (Long). Mockito returned null for the unstubbed OffsetDateTime getter, causing lastModifiedTime to always be 0. Stub both methods so the testFileMetadataAttributes assertion passes. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

… timestamp comparison The previous comparison `newUpdateTime > updateTime` incorrectly returned false when either the original or post-update blob had a null updateTime (both fell back to 0L, making 0 > 0 always false). As suggested in review, return true directly after _storage.update() succeeds since any GCS objects.patch call advances updateTime server-side.

Akanksha-kedia · 2026-05-11T03:31:34Z

@noob-se7en please review

noob-se7en

1 medium scope creep.

…exists() in mkdir - copy(): replace String.format with string concatenation per project style (issue apache#14404) - mkdir(): _storage.create() throws on failure, so returning blob.exists() after a successful create is redundant; return true directly

noob-se7en reviewed Apr 29, 2026

View reviewed changes

noob-se7en added pinot-filesystem Related to PinotFS abstraction (S3, GCS, ADLS, HDFS) bug Something is not working as expected GCP Related to Google Cloud-specific features or deployment labels Apr 29, 2026

Akanksha-kedia force-pushed the fix-gcs-pinot-fs-null-safety branch from 0b5b8f7 to 4de1ffd Compare May 5, 2026 11:11

noob-se7en reviewed May 5, 2026

View reviewed changes

Comment thread ...pinot-file-system/pinot-gcs/src/main/java/org/apache/pinot/plugin/filesystem/GcsPinotFS.java Outdated

Akanksha-kedia and others added 7 commits May 10, 2026 19:06

Fix spotless import order: move OffsetDateTime after java.nio imports

d0d4d33

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Fix: declare throws IOException on open() test methods

eea358c

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Akanksha-kedia force-pushed the fix-gcs-pinot-fs-null-safety branch from 29091ee to 3bb323d Compare May 10, 2026 13:37

noob-se7en reviewed May 12, 2026

View reviewed changes

Comment thread pinot-compatibility-verifier/src/main/java/org/apache/pinot/compat/StreamOp.java Outdated

Akanksha-kedia mentioned this pull request May 13, 2026

[fix] Fix NPE in StreamOp.fetchExistingTotalDocs when exceptions array is present #18491

Open

6 tasks

Akanksha-kedia force-pushed the fix-gcs-pinot-fs-null-safety branch from 7aec6fc to c8ffd40 Compare May 14, 2026 12:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix GcsPinotFS null safety and PinotFS contract compliance#18360

Fix GcsPinotFS null safety and PinotFS contract compliance#18360
Akanksha-kedia wants to merge 8 commits into
apache:masterfrom
Akanksha-kedia:fix-gcs-pinot-fs-null-safety

Akanksha-kedia commented Apr 28, 2026

Uh oh!

codecov-commenter commented Apr 28, 2026 •

edited

Loading

Uh oh!

Akanksha-kedia commented Apr 28, 2026

Uh oh!

noob-se7en left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Akanksha-kedia commented May 5, 2026

Uh oh!

Akanksha-kedia commented May 5, 2026

Uh oh!

Uh oh!

Akanksha-kedia commented May 10, 2026

Uh oh!

Akanksha-kedia commented May 11, 2026

Uh oh!

noob-se7en left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

Akanksha-kedia commented Apr 28, 2026

Description

Changes Made

1. lastModified() — return 0L for missing files

2. touch() — create an empty file when the blob does not exist

3. open() — throw a descriptive IOException for missing files

4. copyFile() — null check on source blob and cleanup on failure

5. Guarded all getUpdateTime() calls

Testing Done

Uh oh!

codecov-commenter commented Apr 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Akanksha-kedia commented Apr 28, 2026

Uh oh!

noob-se7en left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Akanksha-kedia commented May 5, 2026

Uh oh!

Akanksha-kedia commented May 5, 2026

Uh oh!

Uh oh!

Akanksha-kedia commented May 10, 2026

Uh oh!

Akanksha-kedia commented May 11, 2026

Uh oh!

noob-se7en left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

1. `lastModified()` — return 0L for missing files

2. `touch()` — create an empty file when the blob does not exist

3. `open()` — throw a descriptive IOException for missing files

4. `copyFile()` — null check on source blob and cleanup on failure

5. Guarded all `getUpdateTime()` calls

codecov-commenter commented Apr 28, 2026 •

edited

Loading