wombatu-kun commented on code in PR #19456: URL: https://github.com/apache/hudi/pull/19456#discussion_r3698923204
########## hudi-trino/src/test/java/io/trino/plugin/hudi/TestHudiPredicatePushdownColumnOrdinals.java: ########## @@ -0,0 +1,354 @@ +/* + * Licensed 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 io.trino.plugin.hudi; + +import io.trino.filesystem.local.LocalInputFile; +import io.trino.parquet.ParquetReaderOptions; +import io.trino.plugin.base.metrics.FileFormatDataSourceStats; +import io.trino.plugin.hive.HiveColumnHandle; +import io.trino.plugin.hive.parquet.ParquetReaderConfig; +import io.trino.plugin.hudi.file.HudiBaseFile; +import io.trino.spi.SplitWeight; +import io.trino.spi.connector.ColumnHandle; +import io.trino.spi.connector.ConnectorPageSource; +import io.trino.spi.connector.ConnectorSession; +import io.trino.spi.connector.DynamicFilter; +import io.trino.spi.predicate.Domain; +import io.trino.spi.predicate.Range; +import io.trino.spi.predicate.TupleDomain; +import io.trino.spi.predicate.ValueSet; +import io.trino.spi.type.Type; +import io.trino.testing.MaterializedResult; +import io.trino.testing.TestingConnectorSession; +import org.apache.parquet.conf.PlainParquetConfiguration; +import org.apache.parquet.example.data.Group; +import org.apache.parquet.example.data.simple.SimpleGroupFactory; +import org.apache.parquet.hadoop.ParquetFileReader; +import org.apache.parquet.hadoop.ParquetWriter; +import org.apache.parquet.hadoop.example.ExampleParquetWriter; +import org.apache.parquet.io.LocalOutputFile; +import org.apache.parquet.schema.LogicalTypeAnnotation; +import org.apache.parquet.schema.MessageType; +import org.apache.parquet.schema.PrimitiveType; +import org.apache.parquet.schema.Types; +import org.joda.time.DateTimeZone; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.OptionalLong; +import java.util.Set; +import java.util.concurrent.CompletableFuture; + +import static io.trino.metastore.HiveType.HIVE_INT; +import static io.trino.plugin.hive.HiveColumnHandle.ColumnType.REGULAR; +import static io.trino.plugin.hive.HiveColumnHandle.createBaseColumn; +import static io.trino.plugin.hudi.HudiPageSourceProvider.createPageSource; +import static io.trino.spi.type.IntegerType.INTEGER; +import static io.trino.testing.MaterializedResult.materializeSourceDataStream; +import static java.lang.Integer.parseInt; +import static org.apache.parquet.schema.Type.Repetition.OPTIONAL; +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Reads a base file whose physical column order does not match the metastore's, the layout hive sync produces + * with {@code hoodie.datasource.hive_sync.omit_metadata_fields=true}: the five {@code _hoodie_*} meta columns are + * absent from the metastore, so every data column's metastore ordinal is five below its physical position. + * <p> + * With {@code hudi.parquet.use-column-names=false} the parquet page source resolves columns positionally, so a + * predicate whose handle still carries the metastore ordinal lands on whichever column physically sits there and + * row groups get pruned on that column's statistics. The fixture makes that observable: {@code c7} grows with the + * row index while every other data column stays in 0..9, so a domain meant for {@code c7} but applied to any other + * column excludes every row group and the read returns nothing. + * <p> + * Note that the shadowed column has to be part of the PROJECTION for the damage to appear: {@code + * descriptorsByPath} is derived from the projection, so a domain resolving to a column the query does not read + * finds no descriptor and is discarded instead. Do not "simplify" the projections below to the predicate column + * alone - that turns these tests green against the unfixed code. + */ +class TestHudiPredicatePushdownColumnOrdinals Review Comment: Done 40261aca65ce - OmittedMetaColumnsHudiTablesInitializer registers a metastore without the five meta columns, wired into the use-column-names=false suite. The predicate has to sit on the sixth data column: below that the stale ordinal resolves to a meta column, which is never projectable, so the domain is discarded rather than misapplied and the bug stays invisible. ########## hudi-trino/src/main/java/io/trino/plugin/hudi/HudiPageSourceProvider.java: ########## @@ -397,7 +401,7 @@ static ConnectorPageSource createPageSource( TupleDomain<ColumnDescriptor> parquetTupleDomain = options.isIgnoreStatistics() || !enablePredicatePushDown ? TupleDomain.all() - : getParquetTupleDomain(descriptorsByPath, getCombinedPredicate(hudiSplit, dynamicFilter), fileSchema, useColumnNames); + : getParquetTupleDomain(descriptorsByPath, getPushdownPredicate(hudiSplit, dynamicFilter, fileSchema, useColumnNames), fileSchema, useColumnNames); Review Comment: Done 40261aca65ce - took the end-to-end route in TestHudiMorMergeModeSemantics: value > 65 sits above every base value and below the obsolete k6 update, so enabling pushdown at the merge call site prunes the base row group whole and the obsolete row surfaces. The naive shape does not discriminate, since a log record that wins and carries the full record still gives the right answer with the base row dropped. -- 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]
