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:
1. Non-zero bound for empty input is standard. 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. 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]
