cloud-fan commented on code in PR #56965: URL: https://github.com/apache/spark/pull/56965#discussion_r3530804017
########## sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/parquet/types/ops/ParquetFilterOps.scala: ########## @@ -0,0 +1,107 @@ +/* + * 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 java.lang.{Long => JLong} +import java.util.HashSet + +import org.apache.parquet.filter2.predicate.{FilterApi, FilterPredicate} +import org.apache.parquet.filter2.predicate.SparkFilterApi.longColumn +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, 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. Most primitive-backed types + * should extend [[LongParquetFilterOps]] (or a future per-primitive base) rather than implement + * the builders directly, so that boilerplate and the null-handling split are written once. + * + * @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 + def makeNotEq(columnPath: Array[String], value: Any): FilterPredicate + def makeLt(columnPath: Array[String], value: Any): FilterPredicate + def makeLtEq(columnPath: Array[String], value: Any): FilterPredicate + def makeGt(columnPath: Array[String], value: Any): FilterPredicate + def makeGtEq(columnPath: Array[String], value: Any): FilterPredicate + def makeIn(columnPath: Array[String], values: Array[Any]): FilterPredicate +} + +/** + * Base [[ParquetFilterOps]] for a type stored in an INT64 Parquet column. Implements all seven + * predicate builders over `longColumn`, so a concrete type supplies only the encoding it owns + * ([[logicalTypeAnnotation]] - [[primitiveTypeName]] is fixed to INT64), [[acceptsValue]], and + * the [[toLong]] value conversion. The null-handling split (eq/notEq/in tolerate a null value + * for IsNull / IsNotNull; the ordered builders never receive null) and the `makeIn` set + * construction are written here once, so an implementer can't get them wrong. + */ +private[parquet] abstract class LongParquetFilterOps extends ParquetFilterOps { Review Comment: This resolves my earlier note — the null-split and the 7-builder boilerplate are written once here, so a new INT64 type can't get the split wrong. Worth deciding the end-state shape while this is the founding PR of the extension point, though: the base is scoped to one physical family (`longColumn` + `primitiveTypeName = INT64` hardcoded), so the first INT32/BINARY/DOUBLE framework type will have to copy-paste a near-identical sibling base — the same duplication this refactor removed, one level up. A single generic base covers every ordered physical type. `FilterApi.lt/eq` are generic over `T <: Comparable[T]` with the column-capability bounds, and `LongColumn`/`IntColumn`/`BinaryColumn`/`DoubleColumn`/`FloatColumn` all implement `SupportsLtGt` (only `BooleanColumn` is eq-only, and no framework type is boolean-backed). So a `TypedParquetFilterOps[T <: Comparable[T]]` parameterized on a column accessor + a `toPhysical(value: Any): T` converter writes all 7 builders once for every primitive, and `LongParquetFilterOps` becomes `TypedParquetFilterOps[JLong]` with the accessor and `primitiveTypeName` pre-filled. The type parameter doesn't leak if you keep the public `ParquetFilterOps` trait non-generic (it's what `filterOpsList`, `filterOpsFor`, and the `FrameworkFilterOps` extractor consume) and confine `T` to the base — the registry and extractor never see it. Non-blocking: every framework type today, and NANOS timestamps next, is INT64, so the Long-only base is narrower but not wrong and this is mergeable as-is. And to be fair this reverses my own prior advice to scope it to Long — reading the parquet-mr API since, the generic base is no harder to write, so I'd take it now rather than migrate later. -- 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]
