Parquet Java ALP Implementation - #3397
Conversation
Implements ALP encoding for FLOAT and DOUBLE types, which converts floating-point values to integers using decimal scaling, then applies Frame of Reference (FOR) encoding and bit-packing for compression. New files: - AlpConstants.java: Constants for ALP encoding - AlpEncoderDecoder.java: Core encoding/decoding logic - AlpValuesWriter.java: Writer implementation - AlpValuesReaderForFloat/Double.java: Reader implementations Includes comprehensive unit tests and interop test infrastructure.
Restore original comment indentation that was accidentally changed.
Escape <= characters as <= in javadoc comments to avoid malformed HTML errors during documentation generation.
ALP encoding is not yet part of the parquet-format Thrift specification, so it cannot be converted to org.apache.parquet.format.Encoding. Skip it in the testEnumEquivalence test and add a clear error message in the converter for when ALP conversion is attempted.
size and add independent reader/writer verification tests
93c365a to
6d65eaa
Compare
Switch encode/decode from division-based formula to multiply-by-reciprocal using separate POW10_NEGATIVE arrays, matching C++ Arrow's approach: - Encode: fastRound(value * POW10[e] * POW10_NEGATIVE[f]) - Decode: encoded * POW10[f] * POW10_NEGATIVE[e] Add fastRound helpers with sign branching for correct negative value rounding. Remove version byte from page header (8 -> 7 bytes). Empty pages now emit a 7-byte header with numElements=0. Update all hand-crafted binary tests to match the new header format and add comprehensive end-to-end tests for overflow boundaries, large-scale data, preset caching, and NaN bit-pattern preservation.
- Rewrite TestInterOpReadAlp to use LocalInputFile instead of Hadoop FileSystem, fixing failures on Java 24+ where Subject.getSubject is removed. Tests now read C++ ALP parquet files directly without going through Hadoop security/UGI. - Add AlpExceptionCountTest with per-column exception rate reporting against the real Spotify and Arade floating-point datasets from the parquet-testing repository. Useful for comparing Java vs C++ ALP compression ratios.
- Switch findBestFloatParams/findBestDoubleParams from minimizing exception count to minimizing estimated compressed size (length * bitWidth + exceptions * (typeSize + 2 bytes)), matching the C++ ALP cost model. This closes the ~4-5% compression gap vs C++. - Rewrite sampler to collect evenly-spaced sample vectors and run findBestParams on each, then rank by win count. Matches C++ AlpSampler behavior more closely than the previous HashMap-based approach. - Minor fixes: IOExceptionUtils null check, MemoryManager volatile scale, Files utility cleanup, parquet-cli dependency update.
15bc06d to
24c23e5
Compare
- Move shared LE helper methods (getShortLE/getIntLE/getLongLE) to AlpValuesReader base class; remove duplicates from subclasses - Make EncodingParams fields package-private (remove public modifier) - Replace fully-qualified java.util.Arrays.fill calls with imported Arrays.fill in both float and double readers; add missing import to double reader - Add explanatory comments to getBufferedSize() magic numbers (3 for float, 5 for double) explaining the overhead breakdown - Add ALP enabled state to ParquetProperties.toString() - Add ALP support to DefaultV1ValuesWriterFactory for float and double columns
- Revert Files.java, IOExceptionUtils.java, MemoryManager.java, and parquet-cli/pom.xml to master state; these changes are unrelated to ALP and should be submitted in separate PRs - Clarify ParquetMetadataConverter error message: ALP encoding is defined in the ALP paper (enum value 26) but is not yet in the parquet-format Thrift spec, so ALP cannot be written through the Hadoop write path; the error message now explains what needs to happen to remove the block
prtkgaur
left a comment
There was a problem hiding this comment.
The code organization looks good to me and the code follows the spec. I looked for areas of any extra buffer allocations which might impact performance and I think it is optimally written.
I think we should add a few benchmarks and publish numbers from them.
Thanks for working on this Vinoo!
prtkgaur
left a comment
There was a problem hiding this comment.
Wanted to make sure we have the following testing.
For the cross compatibility testing are we making sure that we write both V1 and V2 pages and the implementation in other language is able to read it.
- Add build-time Perl script to patch generated Encoding.java with ALP(10) after Thrift codegen (process-sources phase), since parquet-format 2.12.0 does not yet include ALP in its Thrift spec - Remove guard in ParquetMetadataConverter.getEncoding() that blocked ALP writes; Encoding.ALP now exists in the patched Thrift enum - Add withAlpEncoding() builder methods to ParquetWriter - Add TestInterOpReadAlp: Java V1/V2 write+read round-trip tests and C++ Arrow interop tests (reads alp_spotify1.parquet, alp_arade.parquet, etc.) - Add AlpEncodingBenchmarks JMH benchmark
…d pyarrow interop test - AlpValuesWriter: stop clearing cachedPresets in reset() so preset (e,f) pairs survive page flushes; eliminates redundant full parameter search on every page after the first, cutting write time ~60% - AlpEncodingBenchmarks: clarify Javadoc that comparison is PLAIN+UNCOMPRESSED (no codec), not plain+ZSTD - parquet-benchmarks pom: add explicit annotationProcessorPaths and proc=full for jmh-generator-annprocess so BenchmarkList is generated under Java 23+ - TestInterOpReadAlp: add pyarrow cross-language compatibility test (skips if pyarrow unavailable or does not yet support ALP encoding)
- ParquetProperties: add withAlpVectorSize(int) and withAlpVectorSize(String, int) builder methods plus getAlpVectorSize(ColumnDescriptor) accessor, defaulting to AlpConstants.DEFAULT_VECTOR_SIZE (1024). - AlpConstants: promote validateVectorSize to public so the builder can validate eagerly across packages. - DefaultV1/V2 ValuesWriterFactory: pass the configured vector size to the 4-arg AlpValuesWriter constructors. - ParquetWriter.Builder: expose withAlpVectorSize facades mirroring withAlpEncoding. - TestInterOpReadAlp: add testJavaWriteAlpCustomVectorSize covering 4500 rows at vectorSize=4096 so we cross a full vector boundary and verify round-trip equality. A wrong log_vector_size byte would surface as decode garbage, so round-trip equality is sufficient proof the configured size took effect on the wire. Enables generating ALP test fixtures at different vector sizes (e.g. 4096) for cross-language compatibility testing against the C++/Rust/Go implementations.
Logging and debug output was missing the new alpVectorSize field alongside the existing 'ALP enabled' line. Cosmetic only — no behavior change.
Adds generateAlpFixturesAtMultipleVectorSizes to TestInterOpReadAlp. For each of the four source files in parquet-testing PR apache#100 (alp_spotify1, alp_arade, alp_float_spotify1, alp_float_arade), reads every row, then re-encodes as Java ALP at both vectorSize=1024 and vectorSize=4096. Output goes to ALP_OUTPUT_DIR (default ${user.dir}/alp-java-generated/), producing 8 files total named alp_java_<stem>_vs{1024,4096}.parquet. Each output is verified by reading back through the standard reader path and bit-comparing every value via doubleToRawLongBits / floatToRawIntBits — catches NaN payload and signed-zero divergence, not just numerical equality. Skips when ALP_TEST_DATA_DIR isn't set, so it stays inert in CI on machines without the source datasets. To run: git clone --branch alpFloatingPointDataset \\ https://github.com/prtkgaur/parquet-testing.git ALP_TEST_DATA_DIR=path/to/parquet-testing/data \\ mvn -pl parquet-hadoop \\ -Dtest=TestInterOpReadAlp#generateAlpFixturesAtMultipleVectorSizes \\ test
Extends generateAlpFixturesAtMultipleVectorSizes to vary writer page
version (PARQUET_1_0, PARQUET_2_0) as a third axis alongside dataset
and ALP vector size. Output grows from 8 → 16 files per run:
alp_java_<stem>_v{1,2}_vs{1024,4096}.parquet
Page version is orthogonal to ALP encoding — the page version
difference lives in the parquet protocol layer, not in the ALP
payload — but covering both axes makes the fixture set fully
symmetric for cross-language compatibility verification. C++/Rust/Go
readers can use the V1 and V2 variants to prove their decoders
handle Java-written ALP regardless of how the surrounding pages are
framed. Avoids an asymmetry where the existing PR apache#100 set has C++
at V1 and Java at V2 with no overlap.
All 16 outputs independently verified against the canonical
_expect.csv truth files from parquet-testing PR apache#100 (1.56M values,
0 mismatches).
The reader was asserting that the ALP header's num_elements equals the data page's valuesCount, but those values differ whenever a column has nulls: num_elements is the count of non-null values that went through ALP encoding, while valuesCount is the total row count of the page (which includes null positions tracked by definition levels). The strict equality check made the reader reject every optional float/double column with at least one null value. Relaxes the check to numElements > valuesCount — the header can never legitimately claim more encoded values than the page has rows, but it can claim fewer when nulls are present. The downstream code already uses numElements (not valuesCount) to drive vector allocation and decoding, so the rest of the read path is unchanged. This was surfaced by the corner-case fixture per parquet-testing issue apache#105, which exercises optional columns with null values.
Two new tests in TestInterOpReadAlp: readAllFixtureFilesIndependently Opens every alp_java_*.parquet in ALP_OUTPUT_DIR and asserts each column chunk declares Encoding.ALP and decodes through the standard reader path without error. Separate from the generator's own round-trip verification so reader correctness surfaces as a distinct signal in CI when the fixtures are present. Skips cleanly when ALP_OUTPUT_DIR is empty so it stays inert in default CI environments. generateAndVerifyCornerCaseFixture Writes a single small fixture file (alp_java_cornercases.parquet, ~60 KB) targeting the corner cases enumerated in parquet-testing issue apache#105: vectors with no exceptions, one exception per vector, all exceptions, NaN/Inf/-0.0, constant values (bit_width=0), multi-vector with differing exponents, and optional columns with nulls. Both f32 and f64 variants — 14 columns × 2048 rows total. Reads each column back and bit-exactly verifies every value against the expected pattern via doubleToRawLongBits / floatToRawIntBits. The corner-case fixture is intended as a candidate file for parquet-testing PR apache#100 once naming/design is confirmed. Generating it also surfaced (and verified the fix for) a pre-existing reader bug where optional columns with nulls couldn't be decoded — see the preceding commit.
The corner-case fixture (alp_java_cornercases.parquet) is synthetic — it isn't derived from any raw dataset in parquet-testing PR apache#100, so the existing alp_*_expect.csv files don't cover it. That left cross-language verifiers with no independent ground truth to check the parquet file against; they had to either trust the Java reader or duplicate the construction recipe in their own code. writeCornerCaseCsvTruth now dumps the expected values straight from the construction recipe into alp_java_cornercases_expect.csv next to the parquet, every time the generator runs. The CSV uses the same format conventions as the existing _expect.csv files (comma- separated, header row, no quoting) plus two extensions: • Empty field = null cell (for optional columns) • Special values printed via Java's standard toString: "NaN", "Infinity", "-Infinity", "-0.0". These all parse via C++ std::stod / std::stof per the standard (case-insensitive, "inf" and "infinity" both accepted). The Arrow C++ ALP decoder reads the parquet and compares against this CSV bit-exactly: 27306 non-null cells + 1366 null cells across 14 columns × 2048 rows, 0 mismatches. This makes the corner-case fixture self-documenting and verifiable by any future cross-language tooling without rerunning the Java generator to discover what the expected values are.
Replaces the two separate ColumnProperty fields in ParquetProperties with a single ColumnProperty<AlpConfig>. Public withAlpEncoding/withAlpVectorSize setters and the isAlpEnabled/getAlpVectorSize getters are unchanged; the merge into one per-column config happens at build time. Adds ColumnProperty.getColumnPaths() so the two independent setters can be combined.
Int loop now covers width 32 and long loop covers width 64, with the top bit of each width exercised. Special-cases the max-width max value since (1L << 64) wraps to 0.
Covers the float and double factories producing the ALP writers under V1 and V2, the per-column case, and ALP taking precedence over byte stream split.
Adds always-run round-trip tests for values spanning a wide signed FOR range (deeply negative frame minimum, high bit width) and for all-null pages (num_elements 0). Adds matching wide-range columns to the interop corner-case fixture.
Drops the encode/decode one-liner javadocs that only restated the code (the IEEE 754 ordering rationale is already in the class javadoc). Rewords the negative-power-array comment to explain reference-layout interop rather than singling out C++.
Adds testFloatPartialNullPage / testDoublePartialNullPage: the writer sees fewer values than the page row count (num_elements < valueCount, i.e. some rows null), and the reader initialized with the larger page count serves exactly num_elements values back.
…a-implementation Resolved conflicts: - ParquetProperties.java: kept both the new CompressionCodecName import and the ColumnPath import; merged the ALP toString line into master's new per-column-codec result string. - parquet-benchmarks/pom.xml: kept <proc>full</proc> for JMH annotation processing.
The patch-alp-encoding.pl script (which injects ALP(10) into the generated Encoding enum until parquet-format ships ALP) was missing the Apache license header, so RAT flagged it as an unapproved file. Adds the standard header.
Real-data cross-language fixtures only exercised low-bit-width FOR frames. Adds coverage for extreme values that force the full FOR bit width: exact integer-valued doubles near the ~2^63 encoding limit (64-bit FOR delta, signed max-min overflows) and floats near ~2^31 (32-bit), both with zero exceptions. Added as always-run unit tests (testDouble/FloatExtremeForBitWidth) and as f64_extreme_for_64bit / f32_extreme_for_32bit columns in the interop corner-case fixture.
Complements the 64-bit (overflow) case with a 63-bit case: exact integer-valued doubles near +/- 2^61 whose FOR delta needs 63 bits without overflowing the signed max-min subtraction. Added as testDoubleExtremeFor63BitWidth and the f64_extreme_for_63bit corner-case fixture column, so coverage spans both the 63-bit non-overflow and 64-bit overflow high-bit-width paths.
…sion, stats, nested Hardens the lossless invariant and covers production paths the existing suite skipped: - Full-bit-space fuzz (double/float): random raw bit patterns incl. all NaN payloads, subnormals, +/-0, +/-Inf; strict raw-bit round-trip (the existing helpers were NaN-lenient). - Distribution shift within a row group: preset-cache stays lossless when later vectors differ from the sampled ones. - Reader robustness: malformed/truncated/corrupt pages fail cleanly (no crash/hang/OOB/OOM). - Dictionary->ALP fallback: dictionary enabled + overflow forces fallback to ALP mid-column (the real default path, previously untested since all tests disabled the dictionary). - ALP under Snappy/Gzip/Zstd compression. - Statistics with NaN (excluded from min/max) + nulls (counted) on an optional column. - ALP on a repeated double field (repetition/definition levels).
- Deterministic allocator-leak test: a counting ByteBufferAllocator wraps the writer over 200 write/getBytes/reset page cycles (~1M values); asserts outstanding buffers == 0 after close (catches the apacheGH-3628-style unreleased-buffer bug) and peak > 0 so it is not vacuous. - 2M-value unit round-trip (scale + no OOM). - 500k rows through the full write/read pipeline with a small row-group size (many row groups) and Snappy, streaming verification so the read side is memory-safe at scale too.
…a-implementation # Conflicts: # parquet-hadoop/src/test/java/org/apache/parquet/format/converter/TestParquetMetadataConverter.java
The merge of origin/master combined upstream's JUnit4 -> AssertJ migration of this file with the branch's added test methods, which still called Assert.assertEquals. Git merged both sides without a conflict, leaving references to an import upstream had removed, so the module failed to compile. Convert the six calls to the file's AssertJ idiom.
Upstream added an enforcer rule (enforce-banned-junit4-imports) that fails the build on any org.junit.** import in test code. All ALP tests used JUnit 4, so parquet-column no longer built. Conversions: - org.junit.Test -> org.junit.jupiter.api.Test - Assert.assertEquals/assertTrue/assertFalse/assertNotNull -> AssertJ assertThat(...), with the JUnit 4 leading message argument becoming .as(...) - assertEquals(expected, actual, delta) -> isCloseTo(expected, within(delta)), or isEqualTo when the delta was zero - Assume.assumeTrue(message, condition) -> Assumptions.assumeTrue(condition, message); note JUnit 5 reverses the argument order - @test(expected = X.class) -> assertThrows around the specific throwing call, so the surrounding try/finally cleanup still runs - @test(timeout = 30000) -> @timeout(value = 30, unit = TimeUnit.SECONDS) - @rule TemporaryFolder -> @tempdir plus a newFolder() helper, preserving TemporaryFolder#newFolder()'s fresh-directory-per-call semantics Test counts are unchanged: parquet-column 154 tests / 0 failures / 3 skipped, and the two parquet-hadoop interop tests 21 / 0 / 12, matching the pre-migration baseline including the skip counts driven by assumeTrue.
This file still used org.junit.Assert and org.junit.Test, which the enforce-banned-junit4-imports rule rejects, so parquet-column failed the enforcer on master with no local changes present. Convert its 22 assertEquals calls to AssertJ and switch to the Jupiter @test annotation, matching the migration applied to the rest of the test sources. All 21 tests in the class pass unchanged.
Addresses review feedback to put this on BytesUtils rather than keeping an ALP-local helper: there is nothing ALP-specific about it, and DeltaBinaryPackingValuesWriterForLong already hand-rolls the same 64 - Long.numberOfLeadingZeros(x) expression. Add BytesUtils.getWidthFromMaxLong as the long counterpart to the existing getWidthFromMaxInt, and drop AlpEncoderDecoder.bitWidthForLong. Its maxDelta == 0 guard was dead code, since Long.numberOfLeadingZeros(0) is 64 and the subtraction already yields 0, so the two size estimators that inlined the same expression now call the helper directly instead of guarding with a ternary. The unit test moves to TestBytesUtil alongside the int version, and gains coverage the ALP-local test lacked: a bound above the int range and a negative bound, which occupies the full 64 bits.
wgtmac
left a comment
There was a problem hiding this comment.
Sorry for a late review. I haven't reviewed tests yet but the production code looks pretty solid. I've left some comments. Please let me know what you think.
| </execution> | ||
| </executions> | ||
| </plugin> | ||
| <!-- |
There was a problem hiding this comment.
We should probably remove these lines by either releasing a new version of parquet-format, or after merging #3709
There was a problem hiding this comment.
Your timing on this was good. #3709 merged a couple of days after you left the comment, so parquet.thrift is now inlined in the repo and I was able to take the approach you suggested.
In 229e0d9, ALP = 10 is declared directly in the Encoding enum, and the perl script and the exec-maven-plugin execution that ran it are both gone. I verified the generated Encoding.java still carries ALP(10) after BYTE_STREAM_SPLIT(9) with no patch step involved.
There is one wrinkle I want your opinion on. dev/update-parquet-thrift.sh overwrites parquet.thrift wholesale from upstream, so the next time anyone runs it the ALP entry will disappear silently. It also rewrites the parquet-format.version sidecar, so a note there would not survive either. For now I have put a clearly marked notice in the enum itself saying it is a local addition that needs re-applying until ALP is accepted into parquet-format, on the grounds that at least the loss shows up in a diff. If you would prefer this handled a different way, such as a guard in the update script, I am glad to change it.
|
|
||
| this.vectorSize = 1 << logVectorSize; | ||
| this.totalCount = numElements; | ||
| this.numVectors = (numElements + vectorSize - 1) / vectorSize; |
There was a problem hiding this comment.
This int arithmetic can overflow for a forged numElements. The result can be negative or trigger a huge allocation.
There was a problem hiding this comment.
Thanks for catching this, it's a good find. You're right that numElements is only bounded by valuesCount, and since a forged file controls that value it can go all the way up to Integer.MAX_VALUE. Two separate things went wrong from there. The + vectorSize - 1 overflowed into a negative count, and even when it didn't overflow, a large count drove a big allocation before anything had been validated.
While fixing it I found the allocation is genuinely reachable rather than theoretical. MultiBufferInputStream.slice() calls ByteBuffer.allocate(length) before it checks for EOF, so a small forged page really can ask for hundreds of megabytes.
This is fixed in 338fb99. numVectors is computed in long now and checked against the bytes actually present before anything gets allocated. Since every vector needs a 4 byte offset entry plus at least its ALP and FOR headers, that gives a firm ceiling on how many vectors the remaining bytes could possibly describe, and anything above it throws a ParquetDecodingException that says how far short the page is.
| this.offsetArraySize = numVectors * Integer.BYTES; | ||
| ByteBuffer offsetBuf = stream.slice(offsetArraySize).order(ByteOrder.LITTLE_ENDIAN); | ||
| this.vectorOffsets = new int[numVectors]; | ||
| for (int v = 0; v < numVectors; v++) { |
There was a problem hiding this comment.
These offsets are trusted without validating the first entry, ordering, or bounds. Decode also ignores the next offset, so malformed bytes can silently produce the wrong vector.
There was a problem hiding this comment.
You're right on all three counts, thank you. Fixed in 338fb99.
initFromPage now validates the whole offset array before any vector is decoded. The first offset has to equal the offset array size, offsets have to increase by at least one vector's worth of fixed headers, and each one has to start early enough to leave room for those headers before the body ends.
The observation about decode ignoring the next offset was the more important half of this, since that's the case that quietly returns wrong values instead of failing. decodeVector now takes each vector's end position from the following offset, or from the end of the body for the last vector, and rejects a packed body or exception block that runs past it.
|
|
||
| @Override | ||
| public void skip(int n) { | ||
| if (n < 0 || pageValueIndex + n > totalCount) { |
There was a problem hiding this comment.
This addition can overflow. A large skip can pass the check and make pageValueIndex negative.
There was a problem hiding this comment.
Good catch, thank you. Fixed in 338fb99 by comparing against the remaining count with n > totalCount - pageValueIndex rather than adding to pageValueIndex, so a large n can no longer overflow past the check and leave the reader at a negative index. I added testSkipRejectsOverflowingCount to cover it.
| this.sampledParams = new ArrayList<>(); | ||
| // Space samples evenly: one sample every jump vectors across the rowgroup. | ||
| // Math.max(1, ...) guards against very small rowgroups or large vector sizes. | ||
| this.rowgroupSampleJump = |
There was a problem hiding this comment.
With the default 20K-row page limit, this cadence yields only about two samples per page, while eight are required. The preset cache never builds on normal pages.
There was a problem hiding this comment.
Thank you for working through the arithmetic on this one, it turned out to be the most valuable comment in the review. I checked it and the numbers come out exactly as you described. At the default 20k row page limit with a 1024 vector size a page holds roughly 19 vectors, and rowgroupSampleJump works out to 15, so each page contributes about two samples. Because reset() cleared sampledParams and vectorsProcessed at every page boundary, the count never got near SAMPLER_SAMPLE_VECTORS_PER_ROWGROUP, buildPresetCache() never ran, and findBest*ParamsWithPresets was effectively dead code. Every vector was paying for a full parameter search.
Fixed in eb0e594. The sampler is meant to operate over a rowgroup rather than a page, so the sampling state now survives reset() and samples accumulate across pages the way the design intended. I added testPresetCacheBuildsAcrossPageBoundaries, which fails without the change.
One consequence I want to flag, since it isn't obvious. Now that the preset path is genuinely reachable, encoded output can shift slightly, because the search narrows to the top MAX_PRESET_COMBINATIONS pairs instead of trying everything. That is the intended ALP sampling behavior and it matches the C++ implementation. Round tripping is still exact, since the chosen pair is recorded in each vector's header and exceptions cover anything it cannot represent.
| } | ||
|
|
||
| /** Float writer. Buffers one vector at a time, encodes and flushes when full. */ | ||
| public static class FloatAlpValuesWriter extends AlpValuesWriter { |
There was a problem hiding this comment.
Why the writer implementations are nested static class but the readers are in the separate files? Should we make them consistent?
There was a problem hiding this comment.
Fair question, and the honest answer is that I was not making a deliberate choice here so much as following the layout that was already in the codebase. ByteStreamSplit is structured exactly the same way: ByteStreamSplitValuesWriter nests all five typed writers as public static classes, while its readers live in their own files as ByteStreamSplitValuesReaderForDouble, ByteStreamSplitValuesReaderForFloat and so on. ALP mirrors that, with the two writers nested and AlpValuesReaderForDouble and AlpValuesReaderForFloat separate.
So I agree the asymmetry is odd on its own terms, but it is the existing convention rather than something new, and matching the neighbouring encoding seemed more useful than being internally tidy in a way nothing else in the package is.
That said I do not feel strongly about it. If you would rather ALP be consistent within itself, splitting the two writers into their own files is a small and low risk change and I am happy to do it. It would just leave ALP looking different from ByteStreamSplit instead.
| * <pre> | ||
| * ┌─────────┬──────────────────────┬──────────────┬──────────────┬─────┐ | ||
| * │ Header │ Offset Array │ Vector 0 │ Vector 1 │ ... │ | ||
| * │ 7 bytes │ 4B × numVectors │ (interleaved)│ (interleaved)│ │ |
There was a problem hiding this comment.
This line is not aligned on my editor.
There was a problem hiding this comment.
Thanks for spotting that. The cause is that the cell contains 4B × numVectors, and the HTML entity is seven characters in the source but renders as a single glyph, so the box could never line up in both your editor and the rendered javadoc at the same time.
Fixed in 78c1d18 by using the literal character and padding the cell to the right width, so every row is 73 characters in both views. The same diagram had been copied into AlpValuesReader with the same problem, so I fixed it there too.
| private static final String[] CPP_FLOAT_FILES = {"alp_float_spotify1.parquet", "alp_float_arade.parquet"}; | ||
|
|
||
| private java.nio.file.Path getTestDataDir() { | ||
| String dir = System.getProperty("ALP_TEST_DATA_DIR"); |
There was a problem hiding this comment.
This does not follow other interop tests in the repo to download it from parquet-testing repo.
There was a problem hiding this comment.
You're right, and I would like to move this over to the standard harness. The blocker is that the ALP fixture files are not in parquet-testing yet. They are proposed in apache/parquet-testing#100, which is still open.
Once that merges I will convert this to use InterOpTester.GetInterOpFile with a pinned changeset, the same as TestInterOpReadByteStreamSplit and the other interop tests. Until then the test reads from a local directory and skips when it is not present, which does mean it is effectively a no-op in CI, so I understand it is not carrying its weight right now.
If you have any influence over getting parquet-testing #100 looked at, that would help a lot. Cross language read back is the part of this work I am least able to demonstrate on my own, so it matters for the vote more than for this test alone.
| buildPresetCache(); | ||
| } | ||
| } else { | ||
| params = AlpEncoderDecoder.findBestFloatParamsWithPresets(vectorBuffer, 0, vectorLen, cachedPresets); |
There was a problem hiding this comment.
vectorsProcessed resets for every page. With the default page size, we only collect about two samples, but need eight to build the cache. So cachedPresets is never used in the normal path.
There was a problem hiding this comment.
This is the same underlying problem as your comment on the sampling cadence, and it's fixed in eb0e594. I've written up the details in that thread rather than repeating them here. Thanks for flagging it from both angles, it made the problem much easier to pin down.
| * @param vectorSize the vector size | ||
| * @return this builder for method chaining. | ||
| */ | ||
| public Builder withAlpVectorSize(int vectorSize) { |
There was a problem hiding this comment.
Why not directly use withAlp(AlpConfig)? It looks a little bit over-complicated with the current API, especially buildAlp().
There was a problem hiding this comment.
You're right that this was more complicated than it needed to be, thanks for pushing on it. The builder was keeping the enabled flag and the vector size as two separate ColumnProperty scaffolds, merging them into an AlpConfig in buildAlp(), and then splitting them apart again in the copy constructor. Nothing actually needed that round trip.
Fixed in 99ae547. There is now a single ColumnProperty, with withAlp(AlpConfig) and withAlp(columnPath, AlpConfig) as the primary API. I kept withAlpEncoding and withAlpVectorSize as thin read-modify-write conveniences on top, since they match the ergonomics of withByteStreamSplitEncoding and it means no existing caller had to change, including ParquetWriter.Builder. buildAlp and the copy constructor split are both gone.
I also moved the vector size validation into the AlpConfig constructor, which felt like the right home for it since an invalid size should be rejected however the config was built. That had the nice side effect of removing the last reference to AlpConstants from ParquetProperties, which made your visibility comment straightforward to act on.
One thing I want to flag for you. To let the convenience setters modify an existing config rather than replace it, I added getDefaultValue and getValue to ColumnProperty.Builder. That class is package private so it is not public API, but it is a shared file rather than something ALP specific, so please say if you would rather I found another way around it.
…a-implementation Resolves parquet-format-structures/pom.xml: master moved parquet.thrift in-repo (src/main/thrift) and dropped the maven-dependency-plugin unpack step; kept the ALP Encoding.java patch plugin on top of that.
The ALP page header's num_elements is only checked against the page's valuesCount, which a forged file controls, so it could be as large as Integer.MAX_VALUE. Three consequences: - (numElements + vectorSize - 1) overflowed to a negative vector count, producing a NegativeArraySizeException rather than a decoding error. - Even without overflow, a large count drove a multi-hundred-megabyte offset-array allocation before any byte was validated. ByteBufferInput Stream.slice() allocates before its EOF check on the multi-buffer path, so a small page could trigger it. - The offset array itself was trusted: nothing checked the first entry, ordering, or bounds, and decode ignored the following offset, so forged offsets could read another vector's bytes and silently decode wrong values. Bound the vector count against the bytes actually present before any allocation, validate the offset array up front, and confine each vector's reads to the region the next offset delimits. Also compare skip(n) against the remaining count instead of adding to pageValueIndex, which overflowed and left the reader at a negative index. Tests: forge the header's num_elements directly rather than inflating valuesCount, which never reached the allocation path. Replace catchAny with catchClean so Errors propagate; catching Throwable let an OutOfMemoryError from an allocation bomb, or a failed assertion, count as a clean rejection.
The sampler's unit is a rowgroup: it takes one sample every SAMPLER_ROWGROUP_SIZE / SAMPLER_SAMPLE_VECTORS_PER_ROWGROUP / vectorSize vectors and needs SAMPLER_SAMPLE_VECTORS_PER_ROWGROUP of them before buildPresetCache() can lock in the winning (exponent, factor) combinations. But reset() runs at every page boundary, which is far smaller: at the default 20k row page limit and a 1024 vector size a page holds about 19 vectors, so with a sample jump of 15 it contributes only two samples before reset() cleared them again. The threshold was never reached, cachedPresets stayed null for the life of the writer, and findBest*ParamsWithPresets was dead code -- every vector paid for a full exponent/factor search. Let the sampling state survive reset() so samples accumulate across pages, which is what the rowgroup-scoped design intended. Encoded output can differ slightly now that the preset path is reachable: it narrows the search to the top MAX_PRESET_COMBINATIONS pairs, so a vector may pick a different (exponent, factor) than an exhaustive search would. That is the intended ALP sampling behaviour and matches the C++ implementation; round-trip fidelity is unaffected, since the chosen pair is recorded in each vector's header and exceptions cover anything it cannot represent exactly.
e85ef00 to
8f90f25
Compare
The builder tracked the ALP enabled flag and vector size as two independent ColumnProperty scaffolds, merged them into a per-column AlpConfig in buildAlp(), then split them apart again in the copy constructor. Nothing needed that round trip. Hold one ColumnProperty<AlpConfig> instead and expose withAlp(AlpConfig) / withAlp(columnPath, AlpConfig) directly. withAlpEncoding and withAlpVectorSize stay as read-modify-write conveniences over it, so every existing caller -- including ParquetWriter.Builder -- is unchanged. buildAlp and the copy-constructor split are both gone. Vector size validation moves into the AlpConfig constructor, which is where an invalid size should be rejected regardless of how the config was built. That also removes ParquetProperties' last reference to AlpConstants. ColumnProperty.Builder gains getDefaultValue/getValue so a caller can modify one field of an existing value rather than replace the whole thing. The class is package-private, so this is not public API.
Addresses several review comments in one pass, no behaviour change: - AlpConstants and every member drop to package-private. Only ParquetProperties referenced it from outside the package, and the preceding commit removed that, so none of this needs to be public API. - Constants move to the class that uses them. The sampler constants (SAMPLER_*, MAX_PRESET_COMBINATIONS) are writer-only and move to AlpValuesWriter; the rounding magic numbers and power-of-ten tables are codec-only and move to AlpCodec. AlpConstants is left holding just the wire format: header/metadata sizes, mode and encoding markers, vector size bounds, and per-type exponent limits. - Rename AlpEncoderDecoder to AlpCodec, and its test to match. - Replace the wildcard static imports of AlpConstants with explicit ones in all five files. - Fix the interleaved-page-layout box in the AlpValuesWriter and AlpValuesReader javadoc. The "4B × numVectors" cell was seven source characters wide for a glyph that renders as one, so the box could not line up in both the editor and the rendered javadoc. Using the literal glyph makes every row 73 characters in both.
Adding ALP to the Encoding enum previously meant post-processing the thrift-generated Encoding.java with a perl script wired into the build, because parquet.thrift was unpacked from the released parquet-format jar and could not be edited. Since parquet.thrift is now inlined in the repo (apache#3709), ALP = 10 can be declared in the enum directly. Removes src/main/perl/patch-alp-encoding.pl and the exec-maven-plugin execution that ran it. Verified the generated Encoding.java still carries ALP(10) after BYTE_STREAM_SPLIT(9) with no patch step involved. The entry is marked as a local addition: dev/update-parquet-thrift.sh overwrites this file from upstream and would silently drop ALP, so the notice tells whoever re-runs it to re-apply the entry until the encoding is accepted into parquet-format, at which point both the entry's notice and this divergence go away.
isFloatException has to encode the value to check the round trip, so calling it and then encodeFloat did the work twice. The writer's loop was worse: exception check, encode again, plus a placeholder scan that encoded a third time. tryEncodeFloat and tryEncodeDouble now return the exception flag and the encoded value together through a caller-owned EncodeResult. A holder rather than an Optional keeps the per-value path allocation free. Encoded bytes are unchanged, and a new test asserts the new path agrees with the old exception-check-then-encode pair bit for bit. The benchmark is a wash (197.9 vs 194.2 ms/op, error bars overlap), since encoding is small next to compression and page assembly.
3bc0835 to
8e743c6
Compare
cc @julienledem @alamb @emkornfield @prtkgaur
Rationale for this change
Reworks the ALP encoding implementation to address emkornfield's architectural feedback on PR #3390. The original buffered all values in memory and decoded eagerly. This makes the writer incremental (encode per-vector as values arrive) and the reader lazy (decode on demand), matching how other Parquet encodings work.
Builds on Julien Le Dem's original implementation (#3390). File structure, integration points, core math, and interop test infrastructure all come from his work. The rework focused on the internal writer/reader plumbing.
What changes are included in this PR?
Architecture (addressing review feedback):
AlpValuesReaderabstract base class for shared decode logic (float/double readers only implementdecodeBody).Spec compliance:
ceil(n * bitWidth / 8)(reusingBytesUtils.paddedByteCountFromBits).max - minoverstated the span on mixed-sign vectors).ByteBuffer's typed getters on aLITTLE_ENDIAN-ordered buffer.BytePackerinstead of custom bit-packing.num_exceptions.> 32float /> 64double throw).Configuration:
withAlpEncoding(...)andwithAlpVectorSize(...)onParquetProperties.Builderand the HadoopParquetWriter.Builder, globally or per-column.DefaultV1ValuesWriterFactoryandDefaultV2ValuesWriterFactoryso per-column overrides work.AlpConstantsmin/max bounds eagerly at builder time.AlpConfiginsideParquetProperties(oneColumnProperty<AlpConfig>rather than two); public setters/getters are unchanged.Reader null tolerance (bug fix):
AlpValuesReaderwas assertingnum_elements == page.valuesCount, which fails on optional columns with nulls (num_elements is the encoded non-null count, valuesCount is the page row count including nulls). Relaxed tonum_elements <= valuesCount.Integration:
DefaultV1ValuesWriterFactoryandDefaultV2ValuesWriterFactoryas a fallback data-page encoding for FLOAT/DOUBLE.Are these changes tested?
Yes, extensively. The full
parquet-columnmodule (840 tests) and all downstream non-ALP modules (arrow/avro/protobuf/thrift/variant/cli, 908 tests) pass; the parquet-hadoop ALP interop tests pass. Coverage spans:Correctness & spec:
Integration (production paths):
null_countcorrect.Robustness & scale:
ByteBufferAllocatorverifies all off-heap buffers are released across 200 write/reset page cycles.Cross-language verification
The Arrow C++ ALP decoder (apache/arrow#48345) reads every Java-written fixture bit-exact against the canonical
_expect.csvtruth tables. Local verification covers the full {V1, V2} × {vs1024, vs4096} matrix plus the corner-case and extreme-value columns: >1.5M values, 0 mismatches. Six representative fixtures are submitted as a stacked PR (prtkgaur/parquet-testing#1) toward apache/parquet-testing#100 for the other-language readers to verify in CI.Test fixtures for #100:
generateAlpFixturesAtMultipleVectorSizesre-encodes the source datasets as Java ALP across page version / vector size / dataset, each verified bit-exact;generateAndVerifyCornerCaseFixturewrites a small synthetic file whose columns each hit a specific corner case (no/one/all exceptions, NaN/Inf/−0.0, constant, differing exponents, nulls, wide and extreme FOR ranges) with a_expect.csvsidecar emitted from the construction recipe.Are there any user-facing changes?
ParquetProperties.withAlpEncoding()(or the HadoopParquetWriter.Builder), globally or per-column.withAlpVectorSize(int), also globally or per-column. Default is 1024.Known limitation: ALP is not yet in the parquet-format spec
ALP is not yet in the parquet-format Thrift spec (apache/parquet-format#533, PR #548). As a temporary bridge,
parquet-format-structures/src/main/perl/patch-alp-encoding.plruns at build time to injectALP(10)into the generatedEncodingenum, andTestParquetMetadataConverterskips ALP in its enum round-trip. Both are clearly marked to be removed once parquet-format ships ALP and parquet-java bumps its dependency. TheALP = 10value is provisional: if the spec assigns a different value, files written now would need regeneration. Flagging this explicitly so it can be weighed in the vote.