hhr293 commented on code in PR #12299:
URL: https://github.com/apache/gluten/pull/12299#discussion_r3432540281
##########
cpp/core/utils/tac/TypeAwareCompressCodec.cc:
##########
@@ -21,14 +21,18 @@
namespace gluten {
bool TypeAwareCompressCodec::support(int8_t tacType) {
- return tacType == tac::kUInt64;
+ return tacType == tac::kUInt64 || tacType == tac::kUInt128;
}
int64_t TypeAwareCompressCodec::maxCompressedLen(int64_t inputLen, int8_t
tacType) {
- if (!support(tacType)) {
- return 0;
+ switch (tacType) {
+ case tac::kUInt64:
+ return kPayloadHeaderSize + FForCodec::maxCompressedLength(inputLen);
+ case tac::kUInt128:
+ return kPayloadHeaderSize + FForCodec::maxCompressedLength128(inputLen);
+ default:
+ return 0;
}
Review Comment:
Non-zero bound for empty input is standard. ZSTD_compressBound(0),
snappy::MaxCompressedLength(0), LZ4_compressBound(0) all return non-zero.
maxCompressedLen is a safe upper bound, not a prediction of what compress()
writes. The early-return in compress() is an implementation detail;
coupling
the sizing function to it just makes the contract more fragile. And
kPayloadHeaderSize bytes for the empty case is not meaningful
over-allocation.
2. Length validation belongs in compress(), not the sizing function.
maxCompressedLen estimates; compress() enforces preconditions. If
non-8-byte-multiple inputs are rejected by compress(), the bound is never
consumed and the "inconsistency" has no effect. If they aren't rejected,
the
bug is in FForCodec::maxCompressedLength() itself — fix it there, not by
adding a guard one layer up.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
