SemyonSinchenko commented on code in PR #972: URL: https://github.com/apache/incubator-graphar/pull/972#discussion_r4017727310
########## maven-projects/io-api/src/main/java/org/apache/graphar/io/Filter.java: ########## @@ -0,0 +1,109 @@ +/* + * 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 single inspectable table-filter hint; request filters are combined with logical AND. A physical + * reader must validate a comparison literal against the column's {@link ColumnType}: BOOLEAN uses + * Boolean; integer and floating kinds use their matching boxed Java types; STRING uses String; DATE + * uses LocalDate; and TIMESTAMP_MILLIS uses millisecond-precise Instant. Other types cannot be + * compared by this contract. + * + * <p>Comparison operands must have the same declared type; a type mismatch is invalid rather than a + * coercion. Null values never match a comparison, including NOT_EQUAL; use IS_NULL or IS_NOT_NULL + * for null tests. Ordered STRING comparisons use {@link String#compareTo(String)}, and DATE and + * TIMESTAMP_MILLIS use their natural ordering. Readers must reject an invalid comparison before + * returning a result, whether the filter is pushed down or evaluated as fallback. + */ +public final class Filter { + private final String column; + private final ComparisonOperator operator; + private final Literal value; + + private Filter(String column, ComparisonOperator operator, Literal value) { + if (column == null || column.isBlank()) { + throw new IllegalArgumentException("A filter column cannot be blank."); + } + this.column = column; + this.operator = Objects.requireNonNull(operator, "Filter operator cannot be null."); + if ((operator == ComparisonOperator.IS_NULL || operator == ComparisonOperator.IS_NOT_NULL) + && value != null) { + throw new IllegalArgumentException(operator + " does not accept a comparison value."); + } + if (operator != ComparisonOperator.IS_NULL + && operator != ComparisonOperator.IS_NOT_NULL + && value == null) { + throw new IllegalArgumentException(operator + " requires a non-null comparison value."); + } + this.value = value; + } + + /** Creates a filter with an immutable scalar comparison value. */ + public static Filter comparison(String column, ComparisonOperator operator, Literal value) { + if (operator == ComparisonOperator.IS_NULL || operator == ComparisonOperator.IS_NOT_NULL) { + throw new IllegalArgumentException("Use isNull or isNotNull for null checks."); + } + return new Filter(column, operator, value); + } + + /** Creates a null check for {@code column}. */ + public static Filter isNull(String column) { + return new Filter(column, ComparisonOperator.IS_NULL, null); + } + + /** Creates a non-null check for {@code column}. */ + public static Filter isNotNull(String column) { + return new Filter(column, ComparisonOperator.IS_NOT_NULL, null); + } + Review Comment: Why not add `Filter.gt`, `Filter.get`, `Filter.neq`, `Filter.lt`, `Filter.leq`, etc. and make the `public static Filter comparison(String column, ComparisonOperator operator, Literal value)` private internal contract? ########## maven-projects/io-api/src/main/java/org/apache/graphar/io/Filter.java: ########## @@ -0,0 +1,109 @@ +/* + * 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 single inspectable table-filter hint; request filters are combined with logical AND. A physical + * reader must validate a comparison literal against the column's {@link ColumnType}: BOOLEAN uses + * Boolean; integer and floating kinds use their matching boxed Java types; STRING uses String; DATE + * uses LocalDate; and TIMESTAMP_MILLIS uses millisecond-precise Instant. Other types cannot be + * compared by this contract. + * + * <p>Comparison operands must have the same declared type; a type mismatch is invalid rather than a + * coercion. Null values never match a comparison, including NOT_EQUAL; use IS_NULL or IS_NOT_NULL + * for null tests. Ordered STRING comparisons use {@link String#compareTo(String)}, and DATE and + * TIMESTAMP_MILLIS use their natural ordering. Readers must reject an invalid comparison before + * returning a result, whether the filter is pushed down or evaluated as fallback. + */ +public final class Filter { + private final String column; + private final ComparisonOperator operator; + private final Literal value; + + private Filter(String column, ComparisonOperator operator, Literal value) { + if (column == null || column.isBlank()) { + throw new IllegalArgumentException("A filter column cannot be blank."); + } + this.column = column; + this.operator = Objects.requireNonNull(operator, "Filter operator cannot be null."); + if ((operator == ComparisonOperator.IS_NULL || operator == ComparisonOperator.IS_NOT_NULL) + && value != null) { Review Comment: Should we have `value` as `Option` instead of an explicit `null` contract? Passing `null` for optional value sounds like something from 2010th for me :D ########## maven-projects/io-api/src/main/java/org/apache/graphar/io/Filter.java: ########## @@ -0,0 +1,109 @@ +/* + * 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 single inspectable table-filter hint; request filters are combined with logical AND. A physical + * reader must validate a comparison literal against the column's {@link ColumnType}: BOOLEAN uses + * Boolean; integer and floating kinds use their matching boxed Java types; STRING uses String; DATE + * uses LocalDate; and TIMESTAMP_MILLIS uses millisecond-precise Instant. Other types cannot be + * compared by this contract. + * + * <p>Comparison operands must have the same declared type; a type mismatch is invalid rather than a + * coercion. Null values never match a comparison, including NOT_EQUAL; use IS_NULL or IS_NOT_NULL + * for null tests. Ordered STRING comparisons use {@link String#compareTo(String)}, and DATE and + * TIMESTAMP_MILLIS use their natural ordering. Readers must reject an invalid comparison before + * returning a result, whether the filter is pushed down or evaluated as fallback. + */ +public final class Filter { Review Comment: Should we make compositions possible, like ```java var filter1 = Filter.isNotNull("a); var filter2 = Filter.comparison("a", ComparisonOperator.GREATER_THAN, Literal.of(0.0)); var filter = filter1 && filter2; ``` ?? ########## maven-projects/io-api/src/main/java/org/apache/graphar/io/Filter.java: ########## @@ -0,0 +1,109 @@ +/* + * 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 single inspectable table-filter hint; request filters are combined with logical AND. A physical + * reader must validate a comparison literal against the column's {@link ColumnType}: BOOLEAN uses + * Boolean; integer and floating kinds use their matching boxed Java types; STRING uses String; DATE + * uses LocalDate; and TIMESTAMP_MILLIS uses millisecond-precise Instant. Other types cannot be + * compared by this contract. + * + * <p>Comparison operands must have the same declared type; a type mismatch is invalid rather than a + * coercion. Null values never match a comparison, including NOT_EQUAL; use IS_NULL or IS_NOT_NULL + * for null tests. Ordered STRING comparisons use {@link String#compareTo(String)}, and DATE and + * TIMESTAMP_MILLIS use their natural ordering. Readers must reject an invalid comparison before + * returning a result, whether the filter is pushed down or evaluated as fallback. + */ +public final class Filter { + private final String column; + private final ComparisonOperator operator; + private final Literal value; + + private Filter(String column, ComparisonOperator operator, Literal value) { + if (column == null || column.isBlank()) { + throw new IllegalArgumentException("A filter column cannot be blank."); + } + this.column = column; + this.operator = Objects.requireNonNull(operator, "Filter operator cannot be null."); + if ((operator == ComparisonOperator.IS_NULL || operator == ComparisonOperator.IS_NOT_NULL) + && value != null) { + throw new IllegalArgumentException(operator + " does not accept a comparison value."); + } + if (operator != ComparisonOperator.IS_NULL + && operator != ComparisonOperator.IS_NOT_NULL + && value == null) { + throw new IllegalArgumentException(operator + " requires a non-null comparison value."); + } + this.value = value; + } + + /** Creates a filter with an immutable scalar comparison value. */ + public static Filter comparison(String column, ComparisonOperator operator, Literal value) { + if (operator == ComparisonOperator.IS_NULL || operator == ComparisonOperator.IS_NOT_NULL) { + throw new IllegalArgumentException("Use isNull or isNotNull for null checks."); + } + return new Filter(column, operator, value); + } + + /** Creates a null check for {@code column}. */ + public static Filter isNull(String column) { + return new Filter(column, ComparisonOperator.IS_NULL, null); + } + + /** Creates a non-null check for {@code column}. */ + public static Filter isNotNull(String column) { + return new Filter(column, ComparisonOperator.IS_NOT_NULL, null); + } + Review Comment: In that case we can keep `null` contract and whatever we want, while users will get a clear and intuitive API... ########## maven-projects/io-api/src/main/java/org/apache/graphar/io/Filter.java: ########## @@ -0,0 +1,109 @@ +/* + * 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 single inspectable table-filter hint; request filters are combined with logical AND. A physical + * reader must validate a comparison literal against the column's {@link ColumnType}: BOOLEAN uses + * Boolean; integer and floating kinds use their matching boxed Java types; STRING uses String; DATE + * uses LocalDate; and TIMESTAMP_MILLIS uses millisecond-precise Instant. Other types cannot be + * compared by this contract. + * + * <p>Comparison operands must have the same declared type; a type mismatch is invalid rather than a + * coercion. Null values never match a comparison, including NOT_EQUAL; use IS_NULL or IS_NOT_NULL + * for null tests. Ordered STRING comparisons use {@link String#compareTo(String)}, and DATE and + * TIMESTAMP_MILLIS use their natural ordering. Readers must reject an invalid comparison before + * returning a result, whether the filter is pushed down or evaluated as fallback. + */ +public final class Filter { + private final String column; Review Comment: I like the idea of having `Literal` instead of `Object`, but the very logical next step will be to have `Column` instead of `String column`: by the end, filter can be a complex one, can't it? ########## maven-projects/io-api/src/main/java/org/apache/graphar/io/ReadRequest.java: ########## @@ -0,0 +1,148 @@ +/* + * 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.net.URI; +import java.util.ArrayList; +import java.util.Collections; +import java.util.EnumSet; +import java.util.List; +import java.util.Objects; +import java.util.Optional; +import java.util.OptionalLong; +import java.util.Set; + +/** + * An immutable physical read operation. Filters are combined with logical AND and may require a + * backend to read columns that are not in the requested output projection. + */ +public final class ReadRequest { Review Comment: Let's add `equals` / `hashCode`, so the physical execution layer can, for example, deduplicate requests / cache results per request / etc. ########## maven-projects/io-api/src/main/java/org/apache/graphar/io/Projection.java: ########## @@ -0,0 +1,91 @@ +/* + * 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 set of requested output columns. */ +public final class Projection { + private static final Projection ALL_COLUMNS = new Projection(true, List.of()); + + private final boolean allColumns; + private final List<String> columns; Review Comment: I still do not like the idea of working with `String column`. I see a lot of potential risks, like unresolved vs resolved, unqualified vs qualified, etc. etc. As well possible collisions by unqualified names as an example. ########## maven-projects/io-api/src/main/java/org/apache/graphar/io/ReadReport.java: ########## @@ -0,0 +1,61 @@ +/* + * 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.Collections; +import java.util.EnumSet; +import java.util.Objects; +import java.util.Set; + +/** Per-read accounting of physical hints applied or declined by a backend. */ +public final class ReadReport { Review Comment: I would add `equals` / `hashCode`, so the system can, for example, separate processed / unprocessed and compare them elementwise or use `Set` -- 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]
