cloud-fan commented on code in PR #56965: URL: https://github.com/apache/spark/pull/56965#discussion_r3524979525
########## sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/parquet/types/ops/ParquetFilterOps.scala: ########## @@ -0,0 +1,66 @@ +/* + * 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.spark.sql.execution.datasources.parquet.types.ops + +import org.apache.parquet.filter2.predicate.FilterPredicate +import org.apache.parquet.schema.LogicalTypeAnnotation +import org.apache.parquet.schema.PrimitiveType.PrimitiveTypeName + +/** + * Optional Parquet filter-pushdown support for a Types Framework type. + * + * A framework type that wants its predicates pushed down to Parquet provides a + * [[ParquetFilterOps]] and registers it in [[ParquetTypeOps.filterOpsList]]; types that don't + * support pushdown add nothing and are read without filtering. + * + * Dispatch is keyed off the Parquet file's column encoding, not the requested Spark type, + * because filter pushdown matches the on-disk schema. The ops therefore declares the + * Parquet primitive + logical annotation it owns ([[primitiveTypeName]] / + * [[logicalTypeAnnotation]]); `ParquetFilters` reverse-looks-up the ops for a field via + * [[ParquetTypeOps.filterOpsFor]] and routes that field's predicates here. This keeps the + * per-type filter knowledge (value conversion, predicate construction) with the type + * instead of scattered across `ParquetFilters`. + * + * Implementations build the parquet-mr [[FilterPredicate]] for each comparison directly + * (e.g. via `FilterApi.eq(longColumn(path), ...)`), so they own the choice of physical + * column and the external-value -> physical-value conversion. The eq/notEq/in builders + * must tolerate a null `value` (used for IsNull / IsNotNull); the ordered builders + * (lt/ltEq/gt/gtEq) are only invoked with non-null values. + * + * @see TimeTypeParquetOps.filterOps for a reference implementation (INT64-backed TimeType) + * @since 4.3.0 + */ +private[parquet] trait ParquetFilterOps { + + /** The Parquet logical type annotation of the column this ops handles (may be null). */ + def logicalTypeAnnotation: LogicalTypeAnnotation + + /** The Parquet primitive type of the column this ops handles. */ + def primitiveTypeName: PrimitiveTypeName + + /** Whether `value` (a non-null external filter value) is pushable for this type. */ + def acceptsValue(value: Any): Boolean + + def makeEq(columnPath: Array[String], value: Any): FilterPredicate Review Comment: Since this trait is the extension point for adding pushdown to a new type ("add its ops to this Seq"), it's worth looking at what a second implementer actually has to write. Only three things vary per type — the column accessor (fixed by `primitiveTypeName`), the value converter, and `acceptsValue` — but all 7 `make*` builders are abstract, so an implementer also re-writes the invariant parts: the `FilterApi` call, the `HashSet` loop in `makeIn`, and the null-handling split where `eq`/`notEq`/`in` must tolerate a null value (IsNull/IsNotNull) but `lt`/`ltEq`/`gt`/`gtEq` must not. Getting that split wrong compiles and passes happy-path tests, then NPEs at runtime. All three current framework types (and the NANOS timestamps next in line) are INT64/Long-backed, so a Long-backed base — or default builder impls on the trait — taking a single `toPhysical(value: Any): JLong` plus the column accessor and `acceptsValue`, with the null split implemented once, would drop a new INT64 type to ~2 methods and remove the trap. I'd scope it to Long-backed rather than fully generic (parquet-mr's per-primitive `Column` types make full genericity more trouble than it's worth, and there are no non-Long framework types on the horizon). Not blocking — the current `TimeType` impl is correct; this is about the payoff of the abstraction as an extension point. -- 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]
