keksmd commented on code in PR #972: URL: https://github.com/apache/incubator-graphar/pull/972#discussion_r4023576090
########## 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: Done: `Filter.Comparison.value()` returns `Optional<Literal>`, absent for null checks. The constructor is private and each factory builds exactly the shape it names, so there is no null contract left on the public surface. ########## 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: Done: `Filter.equal / notEqual / lessThan / lessThanOrEqual / greaterThan / greaterThanOrEqual / isNull / isNotNull`. `comparison(...)` is private now. ########## 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: Done: `ColumnRef` replaces `String column` in both `Filter` and `Projection`, and `Filter` is now a predicate tree (see the thread on composition). I went with `ColumnRef` rather than `Column` because `RecordBatch.column(int)` already returns a vector and the two would read as the same thing. ########## 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: Done. `Filter` is a closed tree: `Comparison`, `And`, `Or`, `Not`. Static `Filter.and(a, b, ...)`, `Filter.or(...)`, `Filter.not(x)` plus instance `a.and(b)`, `a.or(b)`, `a.negate()`, so your example is `Filter.isNotNull(a).and(Filter.greaterThan(a, Literal.of(0.0)))`. Same-kind junctions flatten and a double negation collapses, so structurally equal predicates compare equal. A reader walks the tree with `Filter.Visitor<R>` - one callback per node kind - so a new node kind is a compile error in every backend rather than a silently skipped branch. `ReadRequest.filters()` stays a list; it is still the top-level conjunction so a backend can push some terms down and evaluate the rest. ########## 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: Agreed, and this round moves the boundary to a typed `ColumnRef`. On the specific risks: a request is built before its file is opened, so a reference is unresolved by construction; the only place it binds is `Schema.resolve(ColumnRef) -> int` in the backend. That resolution is exact - verbatim name match, unknown column is an error, and a name that matches more than one field is an error (`Schema` allows duplicates, as you noted on #961, so first-match would be silent data corruption). There is no qualifier on purpose: the physical boundary reads one file with one schema, and mapping a qualified logical property onto a physical column is the job of the layer above. Nested paths are not in `ColumnType` yet, so `ColumnRef` is one segment today and grows to a path without breaking callers when a struct kind lands. ########## 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: Done: `ReadRequest.equals/hashCode` over uri, projection, row range, filters and limit, with a test that flips each one. -- 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]
