SemyonSinchenko commented on code in PR #961: URL: https://github.com/apache/incubator-graphar/pull/961#discussion_r3836959353
########## maven-projects/io-api/src/main/java/org/apache/graphar/io/WriteMode.java: ########## @@ -0,0 +1,28 @@ +/* + * 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.graphar.io; + +/** The disposition required for one physical output location. */ +public enum WriteMode { Review Comment: Are we going to support `APPEND`? ########## maven-projects/io-api/src/main/java/org/apache/graphar/io/RecordBatch.java: ########## @@ -0,0 +1,32 @@ +/* + * 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.graphar.io; + +/** A format-neutral, finite group of rows sharing one schema. */ +public interface RecordBatch { Review Comment: Why row-oriented model, not columnar? I mean we can have rows and record batch of them, or we can have a column (or vector in Apache Arrow terms) and a batch of them. Tbh I would prefer to mimic the Apache Arrow layout (as follow as possible to what is described in the Apache Arrow Java). The GraphAr C++ already rely on Arrow. ########## maven-projects/io-api/src/main/java/org/apache/graphar/io/Field.java: ########## @@ -0,0 +1,53 @@ +/* + * 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.graphar.io; + +import java.util.Objects; + +/** A named, typed physical column in a {@link Schema}. */ +public final class Field { Review Comment: Checking a field in schema is a common op, let's add `equals` and `hash` as well. ########## maven-projects/io-api/src/main/java/org/apache/graphar/io/ColumnType.java: ########## @@ -0,0 +1,89 @@ +/* + * 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.graphar.io; + +import java.util.Objects; +import java.util.Optional; + +/** A recursive, format-neutral type used in a physical {@link Schema}. */ +public final class ColumnType { Review Comment: Do we want a more rich hierarchy? FixedSized, VariableSized, Complex (List, etc.), etc.? This one looks complete but the target is to support all that Apache Parquet supports. And one day the current proposal may become hard to maintain ########## maven-projects/io-api/src/main/java/org/apache/graphar/io/Schema.java: ########## @@ -0,0 +1,51 @@ +/* + * 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.graphar.io; + +import java.util.ArrayList; +import java.util.HashSet; +import java.util.List; +import java.util.Objects; +import java.util.Set; + +/** An ordered, immutable mapping of physical batch columns to neutral field definitions. */ +public final class Schema { + private final List<Field> fields; + + public Schema(List<Field> fields) { + Objects.requireNonNull(fields, "Schema fields cannot be null."); + List<Field> copy = new ArrayList<>(fields.size()); + Set<String> names = new HashSet<>(); + for (Field field : fields) { + Field nonNullField = Objects.requireNonNull(field, "A schema field cannot be null."); + if (!names.add(nonNullField.name())) { + throw new IllegalArgumentException( + "Schema contains duplicate field: " + nonNullField.name()); Review Comment: Are we sure that it is a responsibility of the `Schema` to check for duplicates? Because fields can be accessed by index (and it is the only way to access), I see no problems in duplicated names tbh -- 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]
