You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "zeroshade (via GitHub)" <gi...@apache.org> on 2025/09/24 20:14:32 UTC
[PR] fix: unsupported dictionary types in pqarrow [arrow-go]
zeroshade opened a new pull request, #520:
URL: https://github.com/apache/arrow-go/pull/520
### Rationale for this change
fixes #490
### What changes are included in this PR?
When using `PutDictionary` to write an arrow dictionary array to Parquet directly, we will now normalize the data type to a parquet physical storage type to avoid type problems. e.g. A dictionary with `int8` values will get cast to `int32` values before we process it so that all of the remaining encoding code can continue to be based on the physical storage type. This alleviates panics when using `pqarrow` to write Dictionary arrays.
### Are these changes tested?
Yes, the unit tests are updated to include tests for int8/uint8/etc. as dictionary value types.
### Are there any user-facing changes?
No, just fixing panics.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(parquet/pqarrow): unsupported dictionary types in pqarrow [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #520:
URL: https://github.com/apache/arrow-go/pull/520#discussion_r2383183064
##########
parquet/internal/encoding/types.go:
##########
@@ -113,6 +113,14 @@ type DictEncoder interface {
// of [0,dictSize) and is not validated. Returns an error if a non-integral
// array is passed.
PutIndices(arrow.Array) error
+ // NormalizeDict takes an arrow array and normalizes it to a parquet
+ // native type. e.g. a dictionary of type int8 will be cast to an int32
+ // dictionary for parquet storage.
+ //
+ // The returned array must be released by the caller if different from
+ // the input array. If no normalization is needed, the input array
+ // may be returned as-is.
Review Comment:
fair enough, i'll update to that.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(parquet/pqarrow): unsupported dictionary types in pqarrow [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #520:
URL: https://github.com/apache/arrow-go/pull/520#issuecomment-3339971017
> Does pqarrow smuggle Arrow schemas inside Parquet metadata so users can round-trip?
If you set the `StoreSchema` option, then yes it does add the Arrow schema inside the parquet metadata.
> This PR doesn't introduce any breakage of course but it strikes me that arrow -> parquet -> arrow won't round-trip well if the arrow input has to be cast like this.
This is the case no matter what type you pass (even if it's not dictionary encoded) because Parquet only has int32/int64 as physical storage types. In the case where the original Arrow schema is in the metadata, we properly cast on the way out so there isn't an issue.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(parquet/pqarrow): unsupported dictionary types in pqarrow [arrow-go]
Posted by "amoeba (via GitHub)" <gi...@apache.org>.
amoeba commented on PR #520:
URL: https://github.com/apache/arrow-go/pull/520#issuecomment-3340088422
Thanks for the answers to my questions.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(parquet/pqarrow): unsupported dictionary types in pqarrow [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #520:
URL: https://github.com/apache/arrow-go/pull/520#discussion_r2383173914
##########
parquet/pqarrow/encode_arrow_test.go:
##########
@@ -1352,6 +1352,7 @@ var fullTypeList = []arrow.DataType{
}
var dictEncodingSupportedTypeList = []arrow.DataType{
+ arrow.PrimitiveTypes.Int8,
Review Comment:
testing at least one is generally sufficient, but I can add the other types easily to confirm.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: unsupported dictionary types in pqarrow [arrow-go]
Posted by "amoeba (via GitHub)" <gi...@apache.org>.
amoeba commented on code in PR #520:
URL: https://github.com/apache/arrow-go/pull/520#discussion_r2382697001
##########
parquet/pqarrow/encode_arrow_test.go:
##########
@@ -1352,6 +1352,7 @@ var fullTypeList = []arrow.DataType{
}
var dictEncodingSupportedTypeList = []arrow.DataType{
+ arrow.PrimitiveTypes.Int8,
Review Comment:
I'm not that familiar with the Go package yet, should this list include more of the possible types?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(parquet/pqarrow): unsupported dictionary types in pqarrow [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade merged PR #520:
URL: https://github.com/apache/arrow-go/pull/520
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(parquet/pqarrow): unsupported dictionary types in pqarrow [arrow-go]
Posted by "amoeba (via GitHub)" <gi...@apache.org>.
amoeba commented on code in PR #520:
URL: https://github.com/apache/arrow-go/pull/520#discussion_r2383243999
##########
parquet/pqarrow/encode_arrow_test.go:
##########
@@ -1352,6 +1352,7 @@ var fullTypeList = []arrow.DataType{
}
var dictEncodingSupportedTypeList = []arrow.DataType{
+ arrow.PrimitiveTypes.Int8,
Review Comment:
Thanks. I like having more tests when they're cheap.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: unsupported dictionary types in pqarrow [arrow-go]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #520:
URL: https://github.com/apache/arrow-go/pull/520#discussion_r2378083568
##########
parquet/internal/encoding/types.go:
##########
@@ -113,6 +113,14 @@ type DictEncoder interface {
// of [0,dictSize) and is not validated. Returns an error if a non-integral
// array is passed.
PutIndices(arrow.Array) error
+ // NormalizeDict takes an arrow array and normalizes it to a parquet
+ // native type. e.g. a dictionary of type int8 will be cast to an int32
+ // dictionary for parquet storage.
+ //
+ // The returned array must be released by the caller if different from
+ // the input array. If no normalization is needed, the input array
+ // may be returned as-is.
Review Comment:
That's kind of annoying, right? You have to branch. Maybe it should always require releasing (normalize can call retain)?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org