[MINOR][CORE][SQL] Inline argument checks and drop the network.util.JavaUtils dependency in ReadAheadInputStream and VectorizedDeltaBinaryPackedReader - #58739
Open
david-mollitor-db wants to merge 1 commit into
Conversation
…avaUtils dependency in ReadAheadInputStream and VectorizedDeltaBinaryPackedReader
`ReadAheadInputStream` (core) and `VectorizedDeltaBinaryPackedReader`
(sql/core) validated their arguments through
`org.apache.spark.network.util.JavaUtils.checkArgument(check, msg, args...)`,
passing an already-concatenated message string:
JavaUtils.checkArgument(bufferSizeInBytes > 0,
"bufferSizeInBytes should be greater than 0, but the value is " + bufferSizeInBytes);
This has two problems:
- The failure message is built eagerly on every call and then thrown away
on the (overwhelmingly common) success path.
- It pulls in a cross-package dependency on the shuffle/network module's
`JavaUtils` purely for a one-line precondition, and passes an
already-interpolated string as a `String.format` template (a stray `%`
in the interpolated value would throw).
Replace each call with an inline `if (!cond) { throw new
IllegalArgumentException(...); }`, which builds the message only on failure
and removes the `network.util.JavaUtils` import from both files. The
exception type and message text are unchanged, and the inline form matches
the `if (...) throw new ParquetDecodingException(...)` style already used
in `VectorizedDeltaBinaryPackedReader`.
Co-authored-by: Isaac <no-reply@databricks.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
ReadAheadInputStream(core) andVectorizedDeltaBinaryPackedReader(sql/core) validated theirarguments through
org.apache.spark.network.util.JavaUtils.checkArgument(check, msg, args...),passing an already-concatenated message string, e.g.:
This PR replaces each of the three call sites with an inline
if (!cond) { throw new IllegalArgumentException(...); }and removes the now-unusedorg.apache.spark.network.util.JavaUtilsimport from both files. The exception type and messagetext are unchanged.
Why are the changes needed?
The
checkArgument(boolean, String, Object...)overload is designed for deferred%stemplating(
String.format(msg, args)is evaluated only on failure). Passing an already-concatenated stringmisuses it in two ways:
common) success path —
initFromPageruns once per Parquet page.JavaUtilspurely for aone-line precondition, and passes an already-interpolated string as a
String.formattemplate,so a stray
%in the interpolated value would throw aFormatFlagsConversionMismatchException(or similar) instead of the intended
IllegalArgumentException.The inline form builds the message only on failure, drops the cross-package dependency, and in
VectorizedDeltaBinaryPackedReadermatches theif (...) throw new ParquetDecodingException(...)style already used throughout that file.
Does this PR introduce any user-facing change?
No. The same
IllegalArgumentExceptionwith the same message is thrown on the same conditions.How was this patch tested?
Existing suites pass:
ReadAheadInputStreamSuite(8 tests) andParquetDeltaEncodingInteger+ParquetDeltaEncodingLong(32 tests). Checkstyle is clean on bothcoreandsql/core.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Opus 4.8
This pull request and its description were written by Isaac.