zeroshade commented on code in PR #571:
URL: https://github.com/apache/iceberg-go/pull/571#discussion_r2363895023
##########
table/sorting.go:
##########
@@ -125,24 +126,102 @@ const (
)
// A default Sort Order indicating no sort order at all
-var UnsortedSortOrder = SortOrder{OrderID: UnsortedSortOrderID, Fields:
[]SortField{}}
+var UnsortedSortOrder = SortOrder{orderID: UnsortedSortOrderID, fields:
[]SortField{}}
// SortOrder describes how the data is sorted within the table.
//
// Data can be sorted within partitions by columns to gain performance. The
// order of the sort fields within the list defines the order in which the
// sort is applied to the data.
type SortOrder struct {
- OrderID int `json:"order-id"`
- Fields []SortField `json:"fields"`
+ orderID int
+ fields []SortField
+}
+
+func (s SortOrder) OrderID() int {
+ return s.orderID
+}
+
+func (s SortOrder) Fields() iter.Seq[SortField] {
+ return slices.Values(s.fields)
+}
+
+func (s SortOrder) Len() int {
+ return len(s.fields)
+}
+
+func (s SortOrder) MarshalJSON() ([]byte, error) {
+ type Alias struct {
+ OrderID int `json:"order-id"`
+ Fields []SortField `json:"fields"`
+ }
+
+ return json.Marshal(Alias{
+ s.orderID,
+ s.fields,
+ })
+}
+
+func (s *SortOrder) UnmarshalJSON(b []byte) error {
+ type Alias struct {
+ OrderID int `json:"order-id"`
+ Fields []SortField `json:"fields"`
+ }
+ aux := Alias{-1, nil}
+
+ if err := json.Unmarshal(b, &aux); err != nil {
+ return err
+ }
+ s.orderID = aux.OrderID
+ s.fields = aux.Fields
+
+ if len(s.fields) == 0 {
+ s.fields = []SortField{}
+ s.orderID = 0
+
+ return nil
+ }
+
+ if s.orderID == -1 {
+ s.orderID = InitialSortOrderID // initialize default sort order
id
+ }
Review Comment:
Do we want to have the same error as `NewSortOrder` if `order-id` ==
`UnsortedSortOrderID` and `len(fields) == 0`?
##########
table/sorting.go:
##########
@@ -125,24 +126,102 @@ const (
)
// A default Sort Order indicating no sort order at all
-var UnsortedSortOrder = SortOrder{OrderID: UnsortedSortOrderID, Fields:
[]SortField{}}
+var UnsortedSortOrder = SortOrder{orderID: UnsortedSortOrderID, fields:
[]SortField{}}
// SortOrder describes how the data is sorted within the table.
//
// Data can be sorted within partitions by columns to gain performance. The
// order of the sort fields within the list defines the order in which the
// sort is applied to the data.
type SortOrder struct {
- OrderID int `json:"order-id"`
- Fields []SortField `json:"fields"`
+ orderID int
+ fields []SortField
+}
+
+func (s SortOrder) OrderID() int {
+ return s.orderID
+}
+
+func (s SortOrder) Fields() iter.Seq[SortField] {
+ return slices.Values(s.fields)
+}
+
+func (s SortOrder) Len() int {
+ return len(s.fields)
+}
+
+func (s SortOrder) MarshalJSON() ([]byte, error) {
+ type Alias struct {
+ OrderID int `json:"order-id"`
+ Fields []SortField `json:"fields"`
+ }
+
+ return json.Marshal(Alias{
+ s.orderID,
+ s.fields,
+ })
+}
+
+func (s *SortOrder) UnmarshalJSON(b []byte) error {
+ type Alias struct {
+ OrderID int `json:"order-id"`
+ Fields []SortField `json:"fields"`
+ }
+ aux := Alias{-1, nil}
+
+ if err := json.Unmarshal(b, &aux); err != nil {
+ return err
+ }
+ s.orderID = aux.OrderID
+ s.fields = aux.Fields
+
+ if len(s.fields) == 0 {
+ s.fields = []SortField{}
+ s.orderID = 0
+
+ return nil
+ }
+
+ if s.orderID == -1 {
+ s.orderID = InitialSortOrderID // initialize default sort order
id
+ }
+
+ return nil
+}
+
+func NewSortOrder(orderID int, fields []SortField) (SortOrder, error) {
+ if orderID == UnsortedSortOrderID && len(fields) == 0 {
+ return SortOrder{}, fmt.Errorf("sort order ID %d is reserved
for unsorted order", UnsortedSortOrderID)
+ }
+ if fields == nil {
+ fields = []SortField{}
+ }
+
+ for idx, field := range fields {
+ if field.Transform == nil {
+ return SortOrder{}, fmt.Errorf("sort field at index %d
has no transform", idx)
Review Comment:
we should have a `%w` with `ErrInvalid` or something of that sort
##########
table/sorting.go:
##########
@@ -125,24 +126,102 @@ const (
)
// A default Sort Order indicating no sort order at all
-var UnsortedSortOrder = SortOrder{OrderID: UnsortedSortOrderID, Fields:
[]SortField{}}
+var UnsortedSortOrder = SortOrder{orderID: UnsortedSortOrderID, fields:
[]SortField{}}
// SortOrder describes how the data is sorted within the table.
//
// Data can be sorted within partitions by columns to gain performance. The
// order of the sort fields within the list defines the order in which the
// sort is applied to the data.
type SortOrder struct {
- OrderID int `json:"order-id"`
- Fields []SortField `json:"fields"`
+ orderID int
+ fields []SortField
+}
+
+func (s SortOrder) OrderID() int {
+ return s.orderID
+}
+
+func (s SortOrder) Fields() iter.Seq[SortField] {
+ return slices.Values(s.fields)
+}
+
+func (s SortOrder) Len() int {
+ return len(s.fields)
+}
+
+func (s SortOrder) MarshalJSON() ([]byte, error) {
+ type Alias struct {
+ OrderID int `json:"order-id"`
+ Fields []SortField `json:"fields"`
+ }
+
+ return json.Marshal(Alias{
+ s.orderID,
+ s.fields,
+ })
+}
+
+func (s *SortOrder) UnmarshalJSON(b []byte) error {
+ type Alias struct {
+ OrderID int `json:"order-id"`
+ Fields []SortField `json:"fields"`
+ }
+ aux := Alias{-1, nil}
+
+ if err := json.Unmarshal(b, &aux); err != nil {
+ return err
+ }
+ s.orderID = aux.OrderID
+ s.fields = aux.Fields
+
+ if len(s.fields) == 0 {
+ s.fields = []SortField{}
+ s.orderID = 0
+
+ return nil
+ }
+
+ if s.orderID == -1 {
+ s.orderID = InitialSortOrderID // initialize default sort order
id
+ }
+
+ return nil
+}
+
+func NewSortOrder(orderID int, fields []SortField) (SortOrder, error) {
+ if orderID == UnsortedSortOrderID && len(fields) == 0 {
+ return SortOrder{}, fmt.Errorf("sort order ID %d is reserved
for unsorted order", UnsortedSortOrderID)
+ }
+ if fields == nil {
+ fields = []SortField{}
+ }
+
+ for idx, field := range fields {
+ if field.Transform == nil {
+ return SortOrder{}, fmt.Errorf("sort field at index %d
has no transform", idx)
+ }
+ if field.Direction != SortASC && field.Direction != SortDESC {
+ return SortOrder{}, ErrInvalidSortDirection
+ }
+ if field.NullOrder != NullsFirst && field.NullOrder !=
NullsLast {
+ return SortOrder{}, ErrInvalidNullOrder
+ }
Review Comment:
as with the transform, you should use `fmt.Errorf("%w: sort field at index
%d", ErrInvalidSortDirection)` 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]
