zeroshade commented on code in PR #1134:
URL: https://github.com/apache/arrow-go/pull/1134#discussion_r3752527525
##########
arrow/compare.go:
##########
@@ -74,16 +74,20 @@ func TypeEqual(left, right DataType, opts
...TypeEqualOption) bool {
}
return l.n == right.(*FixedSizeListType).n && l.elem.Nullable
== right.(*FixedSizeListType).elem.Nullable
case *MapType:
- if !TypeEqual(l.KeyType(), right.(*MapType).KeyType(), opts...)
{
+ r := right.(*MapType)
+ if !TypeEqual(l.KeyType(), r.KeyType(), opts...) {
return false
}
- if !TypeEqual(l.ItemType(), right.(*MapType).ItemType(),
opts...) {
+ if !TypeEqual(l.ItemType(), r.ItemType(), opts...) {
return false
}
- if l.KeyField().Nullable !=
right.(*MapType).KeyField().Nullable {
+ if l.KeysSorted != r.KeysSorted {
return false
}
- if l.ItemField().Nullable !=
right.(*MapType).ItemField().Nullable {
+ if l.KeyField().Nullable != r.KeyField().Nullable {
+ return false
+ }
+ if l.ItemField().Nullable != r.ItemField().Nullable {
return false
}
if cfg.metadata {
Review Comment:
The refactor missed the two assertions inside this block (lines 94 and 97) —
`right.(*MapType)` is still spelled out there while `r` has been in scope since
line 77:
```go
if cfg.metadata {
if !l.KeyField().Metadata.Equal(r.KeyField().Metadata) {
return false
}
if !l.ItemField().Metadata.Equal(r.ItemField().Metadata) {
return false
}
}
```
With those, the case uses a single assertion throughout, matching the
`*StructType` case directly below.
##########
arrow/compare_test.go:
##########
@@ -395,3 +395,18 @@ func TestTypeEqual(t *testing.T) {
})
}
}
+
+func TestTypeEqualMapKeysSorted(t *testing.T) {
Review Comment:
Minor: `compare_test.go` already has a table-driven `TestTypeEqual` with a
`want bool` field, and two rows there would express this directly — one pair
differing only in `KeysSorted` expecting `false`, one pair agreeing expecting
`true`.
The standalone test works and is readable; it just makes this file carry two
patterns for the same thing, and the table is where someone adding a
type-equality case will look first.
##########
arrow/compare.go:
##########
@@ -74,16 +74,20 @@ func TypeEqual(left, right DataType, opts
...TypeEqualOption) bool {
}
return l.n == right.(*FixedSizeListType).n && l.elem.Nullable
== right.(*FixedSizeListType).elem.Nullable
case *MapType:
- if !TypeEqual(l.KeyType(), right.(*MapType).KeyType(), opts...)
{
+ r := right.(*MapType)
+ if !TypeEqual(l.KeyType(), r.KeyType(), opts...) {
return false
}
- if !TypeEqual(l.ItemType(), right.(*MapType).ItemType(),
opts...) {
+ if !TypeEqual(l.ItemType(), r.ItemType(), opts...) {
return false
}
- if l.KeyField().Nullable !=
right.(*MapType).KeyField().Nullable {
+ if l.KeysSorted != r.KeysSorted {
Review Comment:
Not a change request — a question worth answering before this lands.
Does Arrow C++ compare `keys_sorted` in its map type equality? If it
doesn't, this introduces a cross-implementation divergence in schema matching:
the same pair of map types would compare equal in C++ and unequal here, which
tends to surface far downstream in Flight or IPC schema negotiation rather than
at the comparison itself.
I'd expect C++ does compare it, given our fingerprint already encodes it,
but I haven't verified and it seems worth checking rather than assuming.
--
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]
