zeroshade commented on code in PR #1035:
URL: https://github.com/apache/arrow-go/pull/1035#discussion_r3660790408
##########
parquet/schema/node.go:
##########
@@ -319,10 +319,13 @@ func (p *PrimitiveNode) Equals(rhs Node) bool {
// to store the values in this column.
func (p *PrimitiveNode) PhysicalType() parquet.Type { return p.physicalType }
-// SetTypeLength will change the type length of the node, has no effect if the
-// physical type is not FixedLength Byte Array
+// SetTypeLength will change the type length of the node, and has no effect if
the
+// physical type is not FixedLength Byte Array. It panics if length is not
positive.
func (p *PrimitiveNode) SetTypeLength(length int) {
if p.PhysicalType() == parquet.Types.FixedLenByteArray {
+ if length <= 0 {
+ panic("parquet: fixed length byte array length must be
positive")
+ }
Review Comment:
Placement is right — inside the FLBA branch, so non-fixed-width primitives
keep their documented no-op behavior, and the guard precedes the assignment so
a rejected call leaves `typeLen` untouched (your test asserts exactly that).
This also makes the setter agree with `NewFixedLenByteArrayNode` at line
627, which already panics on the same condition through `MustPrimitive`. Worth
noting the payoff concretely: without this, `toThrift` at line 360 would
happily serialize a negative `type_length` into the footer.
--
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]
