rdblue commented on a change in pull request #589: [WIP] Extend metadata with SortOrder URL: https://github.com/apache/incubator-iceberg/pull/589#discussion_r344517672
########## File path: api/src/main/java/org/apache/iceberg/SortOrder.java ########## @@ -0,0 +1,165 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.iceberg; + +import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.Lists; +import java.io.Serializable; +import java.util.Arrays; +import java.util.List; +import java.util.Objects; +import org.apache.iceberg.SortField.Direction; +import org.apache.iceberg.SortField.NullOrder; +import org.apache.iceberg.SortTransforms.SortTransform; +import org.apache.iceberg.types.Types; + +// TODO: toString() +public class SortOrder implements Serializable { + + private static final SortOrder UNSORTED_ORDER = new SortOrder(new Schema(), 0, ImmutableList.of()); + + private final Schema schema; + private final int orderId; + private final SortField[] fields; + private transient volatile List<SortField> fieldList; + + public static SortOrder unsorted() { + return UNSORTED_ORDER; + } + + private SortOrder(Schema schema, int orderId, List<SortField> fields) { + this.schema = schema; + this.orderId = orderId; + this.fields = new SortField[fields.size()]; + for (int fieldIndex = 0; fieldIndex < this.fields.length; fieldIndex++) { + this.fields[fieldIndex] = fields.get(fieldIndex); + } + } + + public Schema schema() { + return schema; + } + + public int orderId() { + return orderId; + } + + public List<SortField> fields() { + return lazyFieldList(); + } + + public boolean satisfies(SortOrder anotherSortOrder) { + if (anotherSortOrder.fields.length == 0) { + return true; + } + + if (anotherSortOrder.fields.length > fields.length) { + return false; + } + + for (int fieldIndex = 0; fieldIndex < anotherSortOrder.fields.length; fieldIndex++) { + SortField anotherField = anotherSortOrder.fields[fieldIndex]; + SortField field = fields[fieldIndex]; + if (!field.semanticEquals(anotherField)) { + return false; + } + } + + return true; + } + + @Override + public boolean equals(Object obj) { + if (this == obj) { + return true; + } + if (obj == null || getClass() != obj.getClass()) { + return false; + } + + SortOrder that = (SortOrder) obj; + return orderId == that.orderId && Arrays.equals(fields, that.fields); + } + + @Override + public int hashCode() { + return Objects.hash(orderId, Arrays.hashCode(fields)); + } + + private List<SortField> lazyFieldList() { + if (fieldList == null) { + synchronized (this) { + if (fieldList == null) { + this.fieldList = ImmutableList.copyOf(fields); + } + } + } + return fieldList; + } + + public static Builder builderFor(Schema schema) { + return new Builder(schema); + } + + // TODO: add validation + public static class Builder { + private final Schema schema; + private final List<SortField> fields = Lists.newArrayList(); + private int orderId = 0; + + private Builder(Schema schema) { + this.schema = schema; + } + + public Builder natural(String columnName) { Review comment: Right now, we only support a single ordering for each type, so everything would use the "natural" ordering. Also, if the intent was to add named orderings later, I think it is unlikely that custom collations would make good method names. It wouldn't make sense to use some sanitized form of [`ISO-8859-1/en_US/primary`](https://issues.apache.org/jira/browse/PARQUET-686?focusedCommentId=15854888&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-15854888), for example. Your intent might have been to express the transform instead of the order, with "natural" meaning the identity transform? In that case, it would look like this? ```java SortOrder.builderFor(schema) .day("ts", DESC) .bucket("id", 16) .natural("id", DESC, NULLS_FIRST) .build() ``` Instead of using the order name for the method, what about using the direction? That would look like this: ```java SortOrder.builderFor(schema) .desc(day("ts")) .asc(bucket("ts", 16)) .desc("id", NULLS_FIRST) .build(); ``` Another alternative is to use `by`: ```java SortOrder.builderFor(schema) .by(day("ts"), DESC) .by(bucket("ts", 16)) .by("id", DESC, NULLS_FIRST) .build(); ``` I think it makes sense to express the sorted field as an expression so that we can use helper methods to create the sort order like this. What do you think? ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
