Conversation
|
Thanks for opening a pull request! If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project. Then could you also rename the pull request title in the following format? or See also: |
github-actions
Bot
added
Component: Parquet
Component: C++
awaiting review
1b78a5c to
d563ce0
Compare
|
Thanks @prtkgaur -- it is super exciting to see this movement. Unfortunately, I am not familiar with the C/C++ codebase to give this a realistic review. I started the CI checks on this PR and had some comments about the testing. |
prtkgaur
changed the title
| std::string tarball_path = std::string(__FILE__); | ||
| tarball_path = tarball_path.substr(0, tarball_path.find_last_of("/\\")); | ||
| tarball_path = tarball_path.substr(0, tarball_path.find_last_of("/\\")); | ||
| tarball_path += "/arrow/cpp/submodules/parquet-testing/data/floatingpoint_data.tar.gz"; |
There was a problem hiding this comment.
@Reviewer the data sits in the parquet-testing submodule
apache/parquet-testing#100
|
|
||
| // Unsafe resize without initialization - use only when you will immediately | ||
| // overwrite the memory (e.g., before memcpy). Only safe for POD types. | ||
| void UnsafeResize(size_t n) { |
There was a problem hiding this comment.
Using this over resize gave us around 2-3% performance improvement
prtkgaur
changed the title
prtkgaur
changed the title
0c035b7 to
1cb0852
Compare
prtkgaur
changed the title
|
Talked offline and wanted to capture notes on high-level changes:
|
35f1ad7 to
0908342
Compare
Thanks for the feedback @emkornfield. We have addressed
|
prtkgaur
changed the title
kou
changed the title
|
|
|
|
||
| // Slow path: partial read - decode to intermediate buffer | ||
| // ALP Bit unpacker needs batches of 64 | ||
| if (needs_decode_) { |
There was a problem hiding this comment.
TODO(prateek) : check with Antoine and other reviewers if there is a way to relax this constraint. Though this has negligible impact on performance.
There was a problem hiding this comment.
Please check cpp/src/arrow/util/alp/ALP_Encoding_Specification_terse.md for a more terse spec of the encoding.
There was a problem hiding this comment.
Also this file will be removed once the spec in parquet format repository is merged.
|
|
||
| ## 2. Data Layout | ||
|
|
||
| ALP encoding consists of a page-level header followed by one or more encoded vectors. Each vector contains up to 1024 elements. |
There was a problem hiding this comment.
Replace 1024 with the constant specified in AlpConstant file.
1b08599 to
f5f5011
Compare
MSVC treats the double-to-size_t narrowing as an error. Integer ceiling division gives the same count.
clang cannot attach a tparam to a member template declared in its class, so the doc build rejects it.
The marker on a class does not reach a member template, so the Windows link could not find the instantiations.
CompressVector takes int32_t, so passing a size_t narrows and clang rejects it with -Wshorten-64-to-32 under -Werror.
Their definitions live in implementation files, so a separate test executable cannot reach them across a shared library boundary without the marker.
The index is 64-bit, so a 32-bit shift operand makes MSVC warn that the shift may have been meant to be 64-bit, and CHECKIN treats that as an error.
The new ALP test file defines a write-then-read helper called DoRoundtrip in an anonymous namespace. arrow_reader_writer_test.cc, which shares its test target, already has a DoRoundtrip whose trailing parameters are defaulted, so a four-argument call matches both signatures exactly. Separate translation units never see each other's helper, so the collision is invisible until the two files are compiled as one, which is what the Windows build does with unity builds enabled. Rename ours after what it does.
AlpEncodedVectorInfo is exported, so for anything linking against the shared library its static constexpr kStoredSize is an import. Reading the value is fine, because a constexpr initializer is folded at compile time, but binding a reference to it asks for an address, and a constexpr member has no out-of-line definition in the library to take the address of. Windows GCC then fails to link the test. The same assertion on AlpEncodedForVectorInfo two lines below is unaffected: that one is a class template, so its static members are instantiated in the translation unit that uses them. Only the non-template class needs the value read by hand, and the test already checks it through GetStoredSize on the next line, which returns by value.
An empty vector's data() may be null, and memcpy and memset may not be handed a null pointer even when the length is zero. Both endian helpers, the two copies of the packed values and the zero fill for a bit width of zero all reached that case: a vector with no exceptions or one whose values the frame captures entirely. UndefinedBehaviorSanitizer reports each as a nonnull violation and the sanitizer build makes it fatal. Copying zero bytes never did anything, so skipping the call leaves the encoded form and the decoded values unchanged.
The bounds admitted the largest float below 2^31 and the largest double below 2^63. Fast rounding can carry a value up by one ulp, so those two values round to exactly 2^31 and 2^63, which the target integer types cannot represent. Pull each bound down by one ulp, which is both the minimum and the maximum headroom the rounding needs. The two affected values become encoding exceptions and still round-trip exactly, so only the internal representation changes.
66eadd1 to
7462b91
Compare
The ALP conformance fixture is already on parquet-testing main, so Arrow picks it up on any routine submodule bump; skip the tests until the pin moves.
alp_internal.h opened with an eighty-four line flowchart drawing the compression and decompression pipelines in ASCII boxes, one step per box, each step restating what the function that implements it already says. Nothing else under cpp/src draws comments this way -- the style appears in no other Arrow C++ file -- and the drawing had drifted from the code it described: two boxes were a character short of closing, and it carried subscripts and arrows outside ASCII. The five steps are now a paragraph. The page layout was drawn three times, in two files, each copy followed by the same three bullets about random access and parallel decompression. It is now drawn once, in the file that reads and writes it, with the box closed and the reason given in a sentence. Subscripts and arrows are spelled in ASCII throughout, leaving one em dash in prose as the only byte above 126. The tests drew their section headings as boxes. Arrow writes a single rule of seventy dashes, so twenty-two headings lose their equals signs and the twenty-two closing rules go. Comments only. Stripping comments leaves all four files byte-identical to their previous revision, clang-format reports no new warnings, and no line crosses ninety columns.
Comment-only. Both files are byte-identical to their previous contents once comments are stripped, and clang-format reports the same zero warnings before and after. Four comments described what the code used to be rather than what it has to guarantee, so a reader learned the history of a fix instead of the constraint the test pins. Each keeps its technical content: - The preamble on the view-load test said LoadView "was previously vulnerable" and that "the old code used reinterpret_cast". It now states the constraint: exception_positions (uint16_t) and exceptions (T) sit at offsets that depend on bit_packed_size, an odd size leaves them misaligned, reading through them is undefined behavior that ubsan reports, and the view copies both into aligned storage. - "this was where the ubsan error occurred" becomes a note that the load is where the alignment constraint applies. - "previously spans that could be misaligned" becomes the aligned members the decompress path actually exercises. - The random-data test no longer contrasts against a range it used to use; it states that a range staying inside the encodable window never reaches the fallback. Two more restated the code directly below them: a comment naming the element count already in the expression, and a "we need to process in chunks" line above the chunked calls.
Split the ALP implementation into constants, metadata, compression, sampler, and codec units. Simplify the internal APIs and update the CMake/Meson build integration. Remove the separate writer opt-in flag. ALP is selected explicitly with Encoding::ALP while dictionary encoding is disabled. Harden encoding and decoding: - validate page headers, vector metadata, offset chains, bit widths, element counts, and exception positions; - reject pages that leave ALP values unconsumed after all levels are read; - reuse pool-backed scratch buffers and cache partially decoded vectors; - use typed power-of-ten constants for exact decode semantics; - emit a valid header-only page for all-null input. Rework the ALP tests around the production APIs, remove redundant cases, and add malformed-input, boundary, exception, and all-null coverage. Move the end-to-end tests to arrow_encoding_test.cc and enable real parquet-testing interoperability coverage. Update the submodule pin, benchmarks, and C++ documentation.
|
I've finished a full round of review on my end. Instead of posting a lot of comments, I went ahead to create prtkgaur#3 against your fork. Please let me know what you think, @prtkgaur. |
The layout tables for AlpInfo, AlpForInfo and the serialized vector were lost when alp_internal.h was split up. Put them back next to the classes they describe, alongside the page and header tables in alp_codec.cc. The old ForInfo table gave 6 and 10 bytes for the float and double specializations. Both are one byte smaller than that: a frame of reference plus a bit width, with no padding on the wire.
AlpEncoder's static_assert only fires if the template is instantiated, and for an unsupported physical type the encoder and decoder factories throw before they instantiate it. Test that runtime path directly for the six other physical types. Also restore the note about falling back to PLAIN. ALP expands a few columns in the paper's datasets and nothing in the writer decides against it.
Preview status is already noted, but not the behaviour it relies on: no write path chooses ALP, so a column carries it only where encoding() names it.
|
@prtkgaur it looks like CI failures are due to warnings, could you fix. @wgtmac after the merge of prtkgaur#3 do you have more concerns or want to review again? |
|
I went over this PRs comments again and I can't see any additional outstanding items. Are we waiting on anything else to approve this PR? (perhaps @HuaHuaY or @wgtmac could comment as they are listed as code owners and have reviewed this PR before) One thing I did find was @emkornfield 's suggestion to
Maybe that would be good to file a follow on ticket |
Move the decode target restriction from requires clauses to static assertions. This keeps invalid target types as compile-time errors without encoding constraints into exported symbol names. GCC can now match the explicit instantiations, and Clang shared builds resolve the same symbols. The sanitizer target links and all 74 ALP tests pass.
|
MinGW failures seem possibly related: For JSON columns parquet I think. Maybe something with branch updates? |
Ack checking |
@emkornfield, not this does not look related to my change. AMD64 macOS 15-intel C++ <--- this is a timeout unrelated to my change. 81 - arrow-s3fs-test (Timeout) arrow-tests filesystem unittest. ARM64 macOS GLib & Ruby <--- Again doesn't look like this PR. |
Yes this I plan to handle in a follow up PR. |
|
We have issues for these failures: |
8 participants
Co-authored-by: dhirhan17@gmail.com
Rationale for this change
Adaptive Lossless Floating-Point (ALP)
is designed for floating-point data that commonly represents decimal values.
For these workloads, ALP can provide better compression and faster decoding
than general-purpose compression or existing Parquet encodings.
This PR adds ALP support for Parquet
FLOATandDOUBLEcolumns in the ArrowC++ implementation.
Specification
AlpEncoding.md(merged through Add ALP support proposal parquet-format#539; currently in Preview)
What changes are included in this PR?
This PR adds:
FLOATandDOUBLE.ALP is opt-in on the write path. It is used only when the writer explicitly
selects
Encoding::ALPand dictionary encoding is disabled. Arrow does notcurrently select ALP automatically based on the input data.
Are these changes tested?
Yes. Test coverage includes:
parquet-testing.FLOATandDOUBLEround trips.Are there any user-facing changes?
Yes. Arrow C++ can read Parquet pages encoded with ALP. Writers can explicitly
select ALP for supported floating-point columns.
ALP requires reader support and is not selected automatically, so existing
writer behavior remains unchanged unless users opt in.