fix: hdfs read into buffer fully #2031

parthchandra · 2025-07-16T00:50:45Z

The get and read_range methods in hdfs object store implementation do not always read the data requested because the underlying hdfs call may return fewer bytes than requested.
#1992 (comment)

parthchandra · 2025-07-16T00:51:21Z

@Kontinuation, fyi

codecov-commenter · 2025-07-16T01:16:40Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 58.67%. Comparing base (f09f8af) to head (b24df14).
Report is 328 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #2031      +/-   ##
============================================
+ Coverage     56.12%   58.67%   +2.55%     
- Complexity      976     1253     +277     
============================================
  Files           119      134      +15     
  Lines         11743    13102    +1359     
  Branches       2251     2396     +145     
============================================
+ Hits           6591     7688    +1097     
- Misses         4012     4179     +167     
- Partials       1140     1235      +95

☔ 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.

andygrove · 2025-07-16T04:23:50Z

native/hdfs/src/object_store/hdfs.rs

+                .read_with_pos((range.start + total_read) as i64, buf[total_read as usize..].as_mut())
+                .map_err(to_error)?;
+            if read == -1 {
+                break;


Shouldn't this return an unexpected eof error?

Agree, otherwise the function result is probably not predictable.

Actually this implementation is incorrect. read_with_pos never returns -1. Instead, it returns 0 when hitting EOF.

libhdfs behaves strangely, and so does fs-hdfs.

References:

read_with_pos: https://github.com/datafusion-contrib/fs-hdfs/blob/hadoop-3.1.4/src/hdfs.rs#L642-L651

hdfsPread: https://github.com/datafusion-contrib/fs-hdfs/blob/hadoop-3.1.4/c_src/libhdfs/hdfs.c#L1557-L1561

Thanks for catching this @Kontinuation! I based this on the return value of FSDataInputStream, but looks like both fs-hdfs and the official hadoop libhdfs behave differently.
Let me address this.
In fs-hdfs
: a return value < 0 is an error
: a return value of 0 is OK even if the buffer is not fully read into. However, it looks like other object store implementations treat this as an error. So I will treat both as an error (maybe reintroduce the original assertion).

Kontinuation · 2025-07-16T15:15:08Z

native/hdfs/src/object_store/hdfs.rs

+                let read = file.read(buf.as_mut_slice()).map_err(to_error)?;
+                if read == -1 {
+                    break;
+                }


This is also problematic. read also return 0 when hitting EOF and never return an negative value.

comphead

Sorry @Kontinuation if I check your references https://github.com/datafusion-contrib/fs-hdfs/blob/8c03c5ef0942b75abc79ed673931355fa9552131/c_src/libhdfs/hdfs.c#L1564C15-L1564C19 like here, it actually returns -1?

Kontinuation · 2025-07-16T15:42:07Z

Sorry @Kontinuation if I check your references https://github.com/datafusion-contrib/fs-hdfs/blob/8c03c5ef0942b75abc79ed673931355fa9552131/c_src/libhdfs/hdfs.c#L1564C15-L1564C19 like here, it actually returns -1?

When EOF was hit, FSDataInputStream#read returns -1. This follows the requirement of the parent class DataInputStream#read:

return the total number of bytes read into the buffer, or -1 if there is no more data because the end of the stream has been reached.

The jVal.i < 0 branch is for handling EOF in libhdfs, it returns 0:

    if (jVal.i < 0) {
        // EOF
        destroyLocalReference(env, jbRarray);
        return 0;
    }

parthchandra · 2025-07-16T23:47:19Z

@Kontinuation @andygrove @comphead, updated based on review comments

comphead · 2025-07-17T02:19:24Z

native/hdfs/src/object_store/hdfs.rs


    fn read_range(range: &Range<u64>, file: &HdfsFile) -> Result<Bytes> {
        let to_read = (range.end - range.start) as usize;
+        let mut total_read = 0u64;


wondering would be that better to have total_read as usize?

changed to usize

Kontinuation

This patch looks good to me, and it reminds me another problem with fs-hdfs.

The HdfsErr returned by fs-hdfs read functions does not contain JVM stack traces. If there's a read failure caused by an exception in JVM, all we get is an Err(HdfsErr::Generic) with a vague error message. The stack trace will only be printed to stderr by libhdfs but not get propagated to the Java exception thrown by Comet. User has to browse the executor logs to know what happened when a query failure was caused by fs-hdfs read failure.

libhdfs has getLastTLSExceptionRootCause and getLastTLSExceptionStackTrace for retrieving detailed information about the exception, but fs-hdfs is not making use of them.

An ideal approach is to rethrow the original Java exception using env.throw, just like how Comet handles other JNI call exceptions. But this requires revising how Java exception objects are managed in libhdfs.

comphead · 2025-07-17T15:49:11Z

This patch looks good to me, and it reminds me another problem with fs-hdfs.

The HdfsErr returned by fs-hdfs read functions does not contain JVM stack traces. If there's a read failure caused by an exception in JVM, all we get is an Err(HdfsErr::Generic) with a vague error message. The stack trace will only be printed to stderr by libhdfs but not get propagated to the Java exception thrown by Comet. User has to browse the executor logs to know what happened when a query failure was caused by fs-hdfs read failure.

libhdfs has getLastTLSExceptionRootCause and getLastTLSExceptionStackTrace for retrieving detailed information about the exception, but fs-hdfs is not making use of them.

An ideal approach is to rethrow the original Java exception using env.throw, just like how Comet handles other JNI call exceptions. But this requires revising how Java exception objects are managed in libhdfs.

Actually it calls out once again of having opportunity to tweak fs-hdfs

comphead

lgtm thanks @parthchandra

parthchandra · 2025-07-17T17:42:42Z

reminds me another problem with fs-hdfs.
The HdfsErr returned by fs-hdfs read functions does not contain JVM stack traces. If there's a read failure caused by an exception in JVM, all we get is an Err(HdfsErr::Generic) with a vague error message. The stack trace will only be printed to stderr by libhdfs but not get propagated to the Java exception thrown by Comet. User has to browse the executor logs to know what happened when a query failure was caused by fs-hdfs read failure.
libhdfs has getLastTLSExceptionRootCause and getLastTLSExceptionStackTrace for retrieving detailed information about the exception, but fs-hdfs is not making use of them.
An ideal approach is to rethrow the original Java exception using env.throw, just like how Comet handles other JNI call exceptions. But this requires revising how Java exception objects are managed in libhdfs.

Actually it calls out once again of having opportunity to tweak fs-hdfs

We have three PRs open there. Depending on how many changes we end up needing to make and how quickly we get a new fs-hdfs release, we can consider making a copy and then contributing back in one chunk to save time.

parthchandra · 2025-07-17T21:43:23Z

@comphead @Kontinuation @drexler-sky Opened a tracking issue for fs-hdfs issues: #2034

parthchandra · 2025-07-17T21:44:31Z

Merged. Thank you for the reviews @andygrove @comphead @Kontinuation!

fix: hdfs read into buffer fully

f9c8c7c

parthchandra requested review from andygrove and comphead and removed request for andygrove July 16, 2025 00:50

andygrove approved these changes Jul 16, 2025

View reviewed changes

andygrove reviewed Jul 16, 2025

View reviewed changes

Kontinuation reviewed Jul 16, 2025

View reviewed changes

comphead reviewed Jul 16, 2025

View reviewed changes

parthchandra added 2 commits July 16, 2025 12:55

handle error case correctly

16b5c55

clippy

e630c1e

comphead reviewed Jul 17, 2025

View reviewed changes

Kontinuation reviewed Jul 17, 2025

View reviewed changes

comphead approved these changes Jul 17, 2025

View reviewed changes

use usize for total_read

b24df14

parthchandra mentioned this pull request Jul 17, 2025

Tracking fs-hdfs issues #2034

Open

5 tasks

parthchandra merged commit d0812d5 into apache:main Jul 17, 2025
91 checks passed

fix: hdfs read into buffer fully #2031

fix: hdfs read into buffer fully #2031

Uh oh!

Conversation

parthchandra commented Jul 16, 2025

Uh oh!

parthchandra commented Jul 16, 2025

Uh oh!

codecov-commenter commented Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

andygrove Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

comphead Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

Kontinuation Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

parthchandra Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

Kontinuation Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

comphead left a comment

Choose a reason for hiding this comment

Uh oh!

Kontinuation commented Jul 16, 2025

Uh oh!

parthchandra commented Jul 16, 2025

Uh oh!

comphead Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

parthchandra Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

Kontinuation left a comment

Choose a reason for hiding this comment

Uh oh!

comphead commented Jul 17, 2025

Uh oh!

comphead left a comment

Choose a reason for hiding this comment

Uh oh!

parthchandra commented Jul 17, 2025

Uh oh!

parthchandra commented Jul 17, 2025

Uh oh!

Uh oh!

parthchandra commented Jul 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-commenter commented Jul 16, 2025 •

edited

Loading