github-actions[bot] commented on code in PR #68363: URL: https://github.com/apache/doris/pull/68363#discussion_r4281135497
########## fe/fe-core/src/main/java/org/apache/doris/datasource/hive/HivePartitionFilterBuilder.java: ########## @@ -0,0 +1,212 @@ +// 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.doris.datasource.hive; + +import org.apache.doris.catalog.Column; +import org.apache.doris.nereids.trees.expressions.EqualTo; +import org.apache.doris.nereids.trees.expressions.Expression; +import org.apache.doris.nereids.trees.expressions.InPredicate; +import org.apache.doris.nereids.trees.expressions.SlotReference; +import org.apache.doris.nereids.trees.expressions.literal.Literal; +import org.apache.doris.nereids.types.DataType; +import org.apache.doris.nereids.util.ExpressionUtils; + +import com.google.common.collect.Lists; + +import java.util.HashMap; +import java.util.List; +import java.util.Locale; +import java.util.Map; +import java.util.function.Function; +import java.util.stream.Collectors; + +/** Converts the safe subset of Nereids partition predicates to the HMS filter grammar. */ +public final class HivePartitionFilterBuilder { + + private HivePartitionFilterBuilder() { + } + + /** + * Builds an HMS filter from direct equality and IN predicates. Returns null when any predicate + * or column type is outside the supported grammar, so callers can retain local pruning. + */ + public static String build(Expression predicate, List<Column> partitionColumns, + Map<String, String> partitionKeyHiveTypes) { + Map<String, Column> columnsByName = partitionColumns.stream() + .collect(Collectors.toMap(column -> column.getName().toLowerCase(Locale.ROOT), + Function.identity())); + Map<String, List<String>> valuesByName = new HashMap<>(); + for (Expression conjunct : ExpressionUtils.extractConjunction(predicate)) { + if (!collectValues(conjunct, columnsByName, partitionKeyHiveTypes, valuesByName)) { + return null; + } + } + return buildFilter(partitionColumns, partitionKeyHiveTypes, valuesByName); + } + + private static boolean collectValues(Expression expression, Map<String, Column> columnsByName, + Map<String, String> partitionKeyHiveTypes, Map<String, List<String>> valuesByName) { + if (expression instanceof EqualTo) { + EqualTo equalTo = (EqualTo) expression; + return collectEqualValue(equalTo.left(), equalTo.right(), columnsByName, + partitionKeyHiveTypes, valuesByName) + || collectEqualValue(equalTo.right(), equalTo.left(), columnsByName, + partitionKeyHiveTypes, valuesByName); + } + if (expression instanceof InPredicate) { + InPredicate inPredicate = (InPredicate) expression; + String columnName = slotName(inPredicate.getCompareExpr()); + if (columnName == null || !columnsByName.containsKey(columnName)) { + return false; + } + List<String> values = Lists.newArrayList(); + for (Expression option : inPredicate.getOptions()) { + String value = literalValue(option); + if (value == null + || !literalIsSupported((Literal) option, columnsByName.get(columnName), + partitionKeyHiveTypes.get(columnName))) { + return false; + } + values.add(value); + } + valuesByName.computeIfAbsent(columnName, ignored -> Lists.newArrayList()).addAll(values); + return true; + } + return false; + } + + private static boolean collectEqualValue(Expression slotExpression, Expression literalExpression, + Map<String, Column> columnsByName, Map<String, String> partitionKeyHiveTypes, + Map<String, List<String>> valuesByName) { + String columnName = slotName(slotExpression); + if (columnName == null || !columnsByName.containsKey(columnName)) { + return false; + } + String value = literalValue(literalExpression); + if (value == null + || !literalIsSupported((Literal) literalExpression, columnsByName.get(columnName), + partitionKeyHiveTypes.get(columnName))) { + return false; + } + valuesByName.computeIfAbsent(columnName, ignored -> Lists.newArrayList()).add(value); + return true; + } + + private static String slotName(Expression expression) { + if (expression instanceof SlotReference) { + return ((SlotReference) expression).getName().toLowerCase(Locale.ROOT); + } + return null; + } + + private static String literalValue(Expression expression) { + if (!(expression instanceof Literal)) { + return null; + } + Object value = ((Literal) expression).getValue(); + return value == null ? null : value.toString(); + } + + private static boolean literalIsSupported(Literal literal, Column column, String hiveType) { + DataType literalType = literal.getDataType(); + String value = literal.getValue().toString(); + if (isHmsIntegralType(hiveType)) { + if (!column.getType().isIntegerType()) { + return false; + } + return literalType.isIntegerType() && isIntegralLiteral(literal.getValue().toString()); Review Comment: [P1] Preserve noncanonical integer partitions on the JDO path For `Filter(p#1 = 1) -> FileScan(hive_tbl, p#1:INT)`, a valid HMS partition with raw value `01` is retained by the existing local pruner: `IntLiteral` parses it as 1 and `PartitionKey.originHiveKeys` preserves `01` for fetching. With `hive.metastore.try.direct.sql=false` and `hive.metastore.integral.jdo.pushdown=true`, Hive's JDO equality implementation instead matches the canonical partition-name fragment `p=1`, so the new `(p = 1)` filter successfully returns no partition. The empty response is accepted as complete, and local re-pruning cannot recover the missing rows. Hive explicitly documents this [leading-zero limitation](https://github.com/apache/hive/blob/rel/release-3.1.3/standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java#L538-L544). Retain full-list local pruning for integer filters unless the connector can guarantee numeric comparison semantics, and cover noncanonical raw partition values. ########## fe/fe-core/src/main/java/org/apache/doris/datasource/hive/ThriftHMSCachedClient.java: ########## @@ -332,6 +332,60 @@ public List<Partition> listPartitions(String dbName, String tblName) { } } + @Override + public List<Partition> listPartitionsByFilter(String dbName, String tblName, String filter) { + int threshold = filteredPartitionThreshold(partitionBatchSize); + FilteredPartitionPage page; + try (ThriftHMSClient client = getClient()) { + try { + page = ugiDoAs(() -> fetchFilteredPartitionPage( + client.client, dbName, tblName, filter, threshold)); + } catch (Exception e) { + client.setThrowable(e); + throw e; + } + } catch (Exception e) { + throw new HMSClientException("failed to filter partitions in table '%s.%s'.", e, dbName, tblName); + } + if (isFilteredPartitionResponseSaturated(page.rawCount, threshold)) { + throw new HMSClientException( + "HMS partition filter matched more than %d partitions in table '%s.%s'.", + threshold, dbName, tblName); + } + return page.partitions; + } + + private static FilteredPartitionPage fetchFilteredPartitionPage(IMetaStoreClient client, String dbName, + String tableName, String filter, int threshold) throws Exception { + int pageLimit = threshold + 1; + if (client instanceof HmsRawPartitionFilterPageSource) { + HmsRawPartitionFilterPage page = ((HmsRawPartitionFilterPageSource) client) + .listPartitionsByFilterRawPage(dbName, tableName, filter, pageLimit); + return new FilteredPartitionPage(page.getPartitions(), page.getRawCount()); + } + List<Partition> partitions = client.listPartitionsByFilter( + dbName, tableName, filter, (short) pageLimit); + return new FilteredPartitionPage(partitions, partitions.size()); Review Comment: [P1] Do not infer raw completeness from a DLF post-hook list The DLF provider uses `ProxyMetaStoreClient`, which does not implement `HmsRawPartitionFilterPageSource`, so it reaches this fallback. In the pinned DLF 0.2.14 client, `listPartitionsByFilter` applies `filterHook.filterPartitions` after its delegate has fetched at most `max` partitions. For `Filter(p = CAST('x' AS STRING)) -> FileScan(t[p,q])` with 5002 matches and the default threshold 5000, the request fetches only 5001 raw matches. If the configured hook hides one of those, this records 5000 as the raw count, accepts the page as complete, and silently omits the allowed 5002nd match. The normal full-name path enumerates without that cap and can retain it. Preserve the pre-hook count/completeness through this wrapper, or use full-list fallback for clients without that guarantee; the current raw-interface mock test does not exercise this adapter. ########## fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PruneFileScanPartition.java: ########## @@ -91,22 +92,43 @@ private SelectedPartitions pruneExternalPartitions(ExternalTable externalTable, .collect(Collectors.toList()); Map<String, PartitionItem> nameToPartitionItem = scan.getSelectedPartitions().selectedPartitions; + boolean connectorFilteredPartitions = false; + if (nameToPartitionItem.isEmpty() + && scan.getSelectedPartitions().isDeferredPartitionPruning() + && externalTable instanceof HMSExternalTable) { + Optional<Map<String, PartitionItem>> filteredPartitions = + ((HMSExternalTable) externalTable).getNameToPartitionItemsByFilter( Review Comment: [P2] Reuse the preloaded view once internal tables are locked With `enable_preload_external_metadata=true`, an `INSERT INTO internal_olap SELECT ... FROM hive_tbl WHERE p=1` or an internal-table/Hive join first materializes the full partition map before `StatementContext.lock()`. Binding nevertheless marks the Hive scan deferred, so this branch always calls `getNameToPartitionItemsByFilter`, which issues a fresh HMS RPC without consulting that warmed map. Rewriting occurs inside `NereidsPlanner.planWithLock`, and internal read locks remain held until its final resource release. A slow/unavailable HMS therefore extends internal table lock hold time again despite enabled preloading, while the query has already paid the full enumeration cost. Preserve and locally prune the preloaded selection for these statements, or complete selective HMS I/O before acquiring internal locks. ########## fe/fe-core/src/main/java/org/apache/doris/datasource/hive/HivePartitionFilterBuilder.java: ########## @@ -0,0 +1,212 @@ +// 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.doris.datasource.hive; + +import org.apache.doris.catalog.Column; +import org.apache.doris.nereids.trees.expressions.EqualTo; +import org.apache.doris.nereids.trees.expressions.Expression; +import org.apache.doris.nereids.trees.expressions.InPredicate; +import org.apache.doris.nereids.trees.expressions.SlotReference; +import org.apache.doris.nereids.trees.expressions.literal.Literal; +import org.apache.doris.nereids.types.DataType; +import org.apache.doris.nereids.util.ExpressionUtils; + +import com.google.common.collect.Lists; + +import java.util.HashMap; +import java.util.List; +import java.util.Locale; +import java.util.Map; +import java.util.function.Function; +import java.util.stream.Collectors; + +/** Converts the safe subset of Nereids partition predicates to the HMS filter grammar. */ +public final class HivePartitionFilterBuilder { + + private HivePartitionFilterBuilder() { + } + + /** + * Builds an HMS filter from direct equality and IN predicates. Returns null when any predicate + * or column type is outside the supported grammar, so callers can retain local pruning. + */ + public static String build(Expression predicate, List<Column> partitionColumns, + Map<String, String> partitionKeyHiveTypes) { + Map<String, Column> columnsByName = partitionColumns.stream() + .collect(Collectors.toMap(column -> column.getName().toLowerCase(Locale.ROOT), + Function.identity())); + Map<String, List<String>> valuesByName = new HashMap<>(); + for (Expression conjunct : ExpressionUtils.extractConjunction(predicate)) { + if (!collectValues(conjunct, columnsByName, partitionKeyHiveTypes, valuesByName)) { + return null; + } + } + return buildFilter(partitionColumns, partitionKeyHiveTypes, valuesByName); + } + + private static boolean collectValues(Expression expression, Map<String, Column> columnsByName, + Map<String, String> partitionKeyHiveTypes, Map<String, List<String>> valuesByName) { + if (expression instanceof EqualTo) { + EqualTo equalTo = (EqualTo) expression; + return collectEqualValue(equalTo.left(), equalTo.right(), columnsByName, + partitionKeyHiveTypes, valuesByName) + || collectEqualValue(equalTo.right(), equalTo.left(), columnsByName, + partitionKeyHiveTypes, valuesByName); + } + if (expression instanceof InPredicate) { + InPredicate inPredicate = (InPredicate) expression; + String columnName = slotName(inPredicate.getCompareExpr()); + if (columnName == null || !columnsByName.containsKey(columnName)) { + return false; + } + List<String> values = Lists.newArrayList(); + for (Expression option : inPredicate.getOptions()) { + String value = literalValue(option); + if (value == null + || !literalIsSupported((Literal) option, columnsByName.get(columnName), + partitionKeyHiveTypes.get(columnName))) { + return false; + } + values.add(value); + } + valuesByName.computeIfAbsent(columnName, ignored -> Lists.newArrayList()).addAll(values); + return true; + } + return false; + } + + private static boolean collectEqualValue(Expression slotExpression, Expression literalExpression, + Map<String, Column> columnsByName, Map<String, String> partitionKeyHiveTypes, + Map<String, List<String>> valuesByName) { + String columnName = slotName(slotExpression); + if (columnName == null || !columnsByName.containsKey(columnName)) { + return false; + } + String value = literalValue(literalExpression); + if (value == null + || !literalIsSupported((Literal) literalExpression, columnsByName.get(columnName), + partitionKeyHiveTypes.get(columnName))) { + return false; + } + valuesByName.computeIfAbsent(columnName, ignored -> Lists.newArrayList()).add(value); + return true; + } + + private static String slotName(Expression expression) { + if (expression instanceof SlotReference) { + return ((SlotReference) expression).getName().toLowerCase(Locale.ROOT); + } + return null; + } + + private static String literalValue(Expression expression) { + if (!(expression instanceof Literal)) { + return null; + } + Object value = ((Literal) expression).getValue(); + return value == null ? null : value.toString(); + } + + private static boolean literalIsSupported(Literal literal, Column column, String hiveType) { + DataType literalType = literal.getDataType(); + String value = literal.getValue().toString(); + if (isHmsIntegralType(hiveType)) { + if (!column.getType().isIntegerType()) { + return false; + } + return literalType.isIntegerType() && isIntegralLiteral(literal.getValue().toString()); + } + return isHmsStringType(hiveType) && column.getType().isStringType() && literalType.isStringType() Review Comment: [P2] Check Nereids literal families rather than exact types An ordinary predicate has the shape `Filter(city:VARCHAR = VarcharLiteral('shanghai')) -> FileScan(hive_tbl)` for a native Hive STRING partition key. `LogicalPlanBuilder.visitStringLiteral` creates `VarcharLiteral`, and `TypeCoercionUtils.castIfNotSameType` deliberately preserves string-like operands during comparison/IN coercion. Nereids `DataType.isStringType()` only recognizes `StringType`, so this returns false for normal `city='shanghai'` and `city IN (...)` queries and forces full partition enumeration. Likewise, the preceding `isIntegerType()` only recognizes `IntegerType`, rejecting coerced TINYINT/SMALLINT/BIGINT literals despite their supported native Hive types. Use the appropriate literal-family checks while retaining connector safety restrictions; the current tests construct exact `StringLiteral`/`IntegerLiteral` instances and miss these parsed SQL shapes. ########## fe/fe-core/src/main/java/org/apache/doris/datasource/hive/HMSExternalTable.java: ########## @@ -467,6 +471,57 @@ public Optional<SortedPartitionRanges<String>> getSortedPartitionRanges(CatalogR return hivePartitionValues.getSortedPartitionRanges(); } + @Override + public SelectedPartitions initSelectedPartitions(Optional<MvccSnapshot> snapshot) { + if (getDlaType() == DLAType.HIVE && !getPartitionColumns(snapshot).isEmpty()) { + return SelectedPartitions.DEFERRED_PARTITION_PRUNING; Review Comment: [P2] Resolve deferred selections before MV partition collection An unfiltered query such as `Aggregate(sum(v)) -> FileScan(hive_tbl[p])` now keeps this empty deferred selection through logical rewriting, because `PruneFileScanPartition` only matches `Filter(FileScan)`. Before physical translation, `InitMaterializationContextHook.afterRewrite` runs `QueryPartitionCollector`, which copies the empty map keys as a concrete set of zero queried partitions. `MTMVRewriteUtil.getMtmvPartitionsByRelatedPartitions` consequently selects no MV partitions for freshness checking, and the union-rewrite path also rejects all-empty query partitions. Existing partitioned async-MV rewrites of full-table Hive queries therefore stop working even when the MV is fresh. `materializeDeferredHivePartitions` runs too late to repair this. Materialize or correctly represent the all-partitions state before these MV consumers, with an unfiltered Hive/MV regression case. ########## fe/fe-core/src/main/java/org/apache/doris/datasource/hive/HivePartitionFilterBuilder.java: ########## @@ -0,0 +1,212 @@ +// 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.doris.datasource.hive; + +import org.apache.doris.catalog.Column; +import org.apache.doris.nereids.trees.expressions.EqualTo; +import org.apache.doris.nereids.trees.expressions.Expression; +import org.apache.doris.nereids.trees.expressions.InPredicate; +import org.apache.doris.nereids.trees.expressions.SlotReference; +import org.apache.doris.nereids.trees.expressions.literal.Literal; +import org.apache.doris.nereids.types.DataType; +import org.apache.doris.nereids.util.ExpressionUtils; + +import com.google.common.collect.Lists; + +import java.util.HashMap; +import java.util.List; +import java.util.Locale; +import java.util.Map; +import java.util.function.Function; +import java.util.stream.Collectors; + +/** Converts the safe subset of Nereids partition predicates to the HMS filter grammar. */ +public final class HivePartitionFilterBuilder { + + private HivePartitionFilterBuilder() { + } + + /** + * Builds an HMS filter from direct equality and IN predicates. Returns null when any predicate + * or column type is outside the supported grammar, so callers can retain local pruning. + */ + public static String build(Expression predicate, List<Column> partitionColumns, + Map<String, String> partitionKeyHiveTypes) { + Map<String, Column> columnsByName = partitionColumns.stream() + .collect(Collectors.toMap(column -> column.getName().toLowerCase(Locale.ROOT), + Function.identity())); + Map<String, List<String>> valuesByName = new HashMap<>(); + for (Expression conjunct : ExpressionUtils.extractConjunction(predicate)) { + if (!collectValues(conjunct, columnsByName, partitionKeyHiveTypes, valuesByName)) { + return null; + } + } + return buildFilter(partitionColumns, partitionKeyHiveTypes, valuesByName); + } + + private static boolean collectValues(Expression expression, Map<String, Column> columnsByName, + Map<String, String> partitionKeyHiveTypes, Map<String, List<String>> valuesByName) { + if (expression instanceof EqualTo) { + EqualTo equalTo = (EqualTo) expression; + return collectEqualValue(equalTo.left(), equalTo.right(), columnsByName, + partitionKeyHiveTypes, valuesByName) + || collectEqualValue(equalTo.right(), equalTo.left(), columnsByName, + partitionKeyHiveTypes, valuesByName); + } + if (expression instanceof InPredicate) { + InPredicate inPredicate = (InPredicate) expression; + String columnName = slotName(inPredicate.getCompareExpr()); + if (columnName == null || !columnsByName.containsKey(columnName)) { + return false; + } + List<String> values = Lists.newArrayList(); + for (Expression option : inPredicate.getOptions()) { + String value = literalValue(option); + if (value == null + || !literalIsSupported((Literal) option, columnsByName.get(columnName), + partitionKeyHiveTypes.get(columnName))) { + return false; + } + values.add(value); + } + valuesByName.computeIfAbsent(columnName, ignored -> Lists.newArrayList()).addAll(values); + return true; + } + return false; + } + + private static boolean collectEqualValue(Expression slotExpression, Expression literalExpression, + Map<String, Column> columnsByName, Map<String, String> partitionKeyHiveTypes, + Map<String, List<String>> valuesByName) { + String columnName = slotName(slotExpression); + if (columnName == null || !columnsByName.containsKey(columnName)) { + return false; + } + String value = literalValue(literalExpression); + if (value == null + || !literalIsSupported((Literal) literalExpression, columnsByName.get(columnName), + partitionKeyHiveTypes.get(columnName))) { + return false; + } + valuesByName.computeIfAbsent(columnName, ignored -> Lists.newArrayList()).add(value); + return true; + } + + private static String slotName(Expression expression) { + if (expression instanceof SlotReference) { + return ((SlotReference) expression).getName().toLowerCase(Locale.ROOT); + } + return null; + } + + private static String literalValue(Expression expression) { + if (!(expression instanceof Literal)) { + return null; + } + Object value = ((Literal) expression).getValue(); + return value == null ? null : value.toString(); + } + + private static boolean literalIsSupported(Literal literal, Column column, String hiveType) { + DataType literalType = literal.getDataType(); + String value = literal.getValue().toString(); + if (isHmsIntegralType(hiveType)) { + if (!column.getType().isIntegerType()) { + return false; + } + return literalType.isIntegerType() && isIntegralLiteral(literal.getValue().toString()); + } + return isHmsStringType(hiveType) && column.getType().isStringType() && literalType.isStringType() + && value.indexOf('\\') < 0 && value.indexOf('\'') < 0; Review Comment: [P1] Account for Glue's double-quote rewriting On a Glue catalog, `Filter(city = CAST('a" OR city = "b' AS STRING)) -> FileScan(t)` reaches this branch because constant folding produces a `StringLiteral`. The value contains neither an apostrophe nor a backslash, so the builder emits `(city = 'a" OR city = "b')`. However, the supported `AWSCatalogMetastoreClient.listPartitionsByFilter` calls `ExpressionHelper.replaceDoubleQuoteWithSingleQuotes`, which rewrites it to the valid but different filter `(city = 'a' OR city = 'b')`. The actual partition whose value is `a" OR city = "b` is omitted; a successful small/empty response is treated as complete, so the query silently loses its rows. Reject these values for this adapter and fall back, or preserve the literal contents through the Glue conversion. -- 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]
