zeroshade commented on code in PR #824:
URL: https://github.com/apache/iceberg-go/pull/824#discussion_r3018247502
##########
table/sorting.go:
##########
@@ -84,15 +105,36 @@ func (s *SortField) MarshalJSON() ([]byte, error) {
}
}
+ if len(s.SourceIDs) > 1 {
+ return json.Marshal(struct {
+ SourceIDs []int `json:"source-ids"`
+ Transform iceberg.Transform `json:"transform"`
+ Direction SortDirection `json:"direction"`
+ NullOrder NullOrder `json:"null-order"`
+ }{s.SourceIDs, s.Transform, s.Direction, s.NullOrder})
+ }
+
type Alias SortField
return json.Marshal((*Alias)(s))
}
Review Comment:
I wonder if we need to push the table version down here somehow and just
*always* use either the single value or the `source-ids` with multiple for the
appropriate table version rather than switching based on `len(s.SourceIDs)`? Or
does the spec say both are allowed?
##########
partitions.go:
##########
@@ -41,8 +41,13 @@ var UnpartitionedSpec = &PartitionSpec{id: 0}
// PartitionField represents how one partition value is derived from the
// source column by transformation.
type PartitionField struct {
- // SourceID is the source column id of the table's schema
+ // SourceID is the source column id of the table's schema.
+ // For multi-argument transforms, this is the first source column;
+ // use SourceIDs to get all source columns.
SourceID int `json:"source-id"`
+ // SourceIDs contains all source column ids for multi-argument
transforms.
+ // For single-argument transforms this is nil and SourceID should be
used.
+ SourceIDs []int `json:"-"`
Review Comment:
Same as above, should we just eliminate the `SourceID` field entirely and
have everything else work in terms of `SourceIDs`
##########
table/sorting.go:
##########
@@ -51,8 +51,13 @@ var (
// SortField describes a field used in a sort order definition.
type SortField struct {
- // SourceID is the source column id from the table's schema
+ // SourceID is the source column id from the table's schema.
+ // For multi-argument transforms, this is the first source column;
+ // use SourceIDs to get all source columns.
SourceID int `json:"source-id"`
Review Comment:
should we just get rid of this field entirely and then have the json switch
on the fly below for marshal/unmarshal? It would simplify the rest of the code
that would only ever need to look at `SourceIDs` etc.
--
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]
