github-actions[bot] commented on code in PR #67725:
URL: https://github.com/apache/doris/pull/67725#discussion_r4023083382
##########
fe/fe-connector/fe-connector-hive/src/main/java/org/apache/doris/connector/hive/HiveConnectorMetadata.java:
##########
@@ -2528,19 +2738,58 @@ static Map<String, String> parsePartitionName(String
partName,
return values;
}
- private boolean matchesPredicates(Map<String, String> partValues,
+ private boolean matchesPredicates(Map<String, String> partValues,
Map<String, String> partKeyTypes,
Map<String, List<String>> predicates) {
for (Map.Entry<String, List<String>> entry : predicates.entrySet()) {
String colName = entry.getKey();
List<String> allowedValues = entry.getValue();
String actualValue = partValues.get(colName);
Review Comment:
[P1] Bind fallback values to the declared partition keys
[Hive 3.1.3's
`FileUtils.makePartName`](https://github.com/apache/hive/blob/rel/release-3.1.3/common/src/java/org/apache/hadoop/hive/common/FileUtils.java#L139-L162)
lowercases partition keys, but this lookup uses the original declared
spelling. For example, an API-created key `P-X` cannot use the direct HMS
filter, so the local fallback parses Hive's `p-x=1` into key `p-x`; this line
then looks up `P-X`, rejects every real partition, and the carried selection
makes the query return no rows. Please bind decoded values positionally to
`partKeyNames` (and validate arity) instead of trusting the rendered key
spelling, and add an uppercase special-key fallback test.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/QueryPartitionCollector.java:
##########
@@ -75,13 +80,54 @@ public Void
visitLogicalCatalogRelation(LogicalCatalogRelation catalogRelation,
&& ((ExternalTable)
catalogRelation.getTable()).supportInternalPartitionPruned()) {
LogicalFileScan logicalFileScan = (LogicalFileScan)
catalogRelation;
SelectedPartitions selectedPartitions =
logicalFileScan.getSelectedPartitions();
-
tablePartitions.addAll(selectedPartitions.selectedPartitions.keySet());
- tableUsedPartitionNameMap.put(table.getFullQualifiers(),
- Pair.of(catalogRelation.getRelationId(), tablePartitions));
+ if (selectedPartitions.isDeferredPartitionPruning()) {
+ Set<String> deferredPartitions = materializeDeferredPartitions(
+ (PluginDrivenExternalTable) table, logicalFileScan,
context);
+ if (deferredPartitions == null) {
+ // The connector view is unavailable: keep the "query all
partitions" marker.
+ tableUsedPartitionNameMap.put(table.getFullQualifiers(),
PartitionCompensator.ALL_PARTITIONS);
+ } else {
+ tablePartitions.addAll(deferredPartitions);
+ tableUsedPartitionNameMap.put(table.getFullQualifiers(),
+ Pair.of(catalogRelation.getRelationId(),
tablePartitions));
+ }
+ } else {
+
tablePartitions.addAll(selectedPartitions.selectedPartitions.keySet());
Review Comment:
[P2] Treat unavailable NOT_PRUNED views as all partitions
This new non-DEFERRED branch assumes the selection map is authoritative, but
`PruneFileScanPartition` also returns `NOT_PRUNED` when an unrepresentable
connector partition makes the full view unavailable. Execution correctly
interprets that state as scan-all, while this code records its empty sentinel
map as a concrete zero-partition set; `PartitionCompensator` defines that as
"query no partitions," so the async-MV path rejects an otherwise eligible
rewrite. Please map the unavailable/scan-all state to `ALL_PARTITIONS` (or
carry a distinct state) and test a connector-declined predicate with one
unrepresentable typed partition.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/QueryPartitionCollector.java:
##########
@@ -75,13 +80,54 @@ public Void
visitLogicalCatalogRelation(LogicalCatalogRelation catalogRelation,
&& ((ExternalTable)
catalogRelation.getTable()).supportInternalPartitionPruned()) {
LogicalFileScan logicalFileScan = (LogicalFileScan)
catalogRelation;
SelectedPartitions selectedPartitions =
logicalFileScan.getSelectedPartitions();
-
tablePartitions.addAll(selectedPartitions.selectedPartitions.keySet());
- tableUsedPartitionNameMap.put(table.getFullQualifiers(),
- Pair.of(catalogRelation.getRelationId(), tablePartitions));
+ if (selectedPartitions.isDeferredPartitionPruning()) {
+ Set<String> deferredPartitions = materializeDeferredPartitions(
+ (PluginDrivenExternalTable) table, logicalFileScan,
context);
+ if (deferredPartitions == null) {
+ // The connector view is unavailable: keep the "query all
partitions" marker.
+ tableUsedPartitionNameMap.put(table.getFullQualifiers(),
PartitionCompensator.ALL_PARTITIONS);
+ } else {
+ tablePartitions.addAll(deferredPartitions);
+ tableUsedPartitionNameMap.put(table.getFullQualifiers(),
+ Pair.of(catalogRelation.getRelationId(),
tablePartitions));
+ }
+ } else {
+
tablePartitions.addAll(selectedPartitions.selectedPartitions.keySet());
+ tableUsedPartitionNameMap.put(table.getFullQualifiers(),
+ Pair.of(catalogRelation.getRelationId(),
tablePartitions));
+ }
} else {
// not support get partition scene, we consider query all
partitions from table
tableUsedPartitionNameMap.put(table.getFullQualifiers(),
PartitionCompensator.ALL_PARTITIONS);
}
return null;
}
+
+ /**
+ * Materializes the partition names a DEFERRED (not yet enumerated) file
scan reads, for the MV partition
+ * compensation decision only, or {@code null} when the connector view
cannot be materialized.
+ *
+ * <p>{@code DEFERRED} is produced by {@link
PluginDrivenExternalTable#initSelectedPartitions}, i.e. only a
+ * table whose connector can prune partitions from a predicate - so the
cast holds, and the connector is the
+ * only authority for the partition names this scan reads.</p>
+ *
+ * <p>WHY enumeration and not {@link PartitionCompensator#ALL_PARTITIONS}:
the marker means "this query reads
+ * EVERY partition of the base table", which the compensator turns into
"the materialized view already covers
+ * everything, so no union compensation is needed". A deferred view is
merely NOT ENUMERATED YET, so reporting
+ * the marker suppresses exactly the compensation that re-reads the base
partitions an MV does not cover - e.g.
+ * a partition added to the base table after the last MV refresh, whose
rows then silently disappear from a
+ * rewritten query ({@code mv.external_table.part_partition_invalid},
{@code test_hive_rewrite_mtmv}). The
+ * enumeration is the scan's unfiltered view - the same full view the scan
itself has to materialize before
+ * generating splits, served from the connector's partition view cache.
When no partition predicate pruned the
+ * scan (the only way {@code DEFERRED} survives {@code
PruneFileScanPartition}, whose connector-declined path
+ * materializes a local selection) it is exactly the query's own
selection; on a plan collected before that
+ * pruning it is a superset, which can only make the compensator union
MORE base partitions, never fewer.</p>
+ */
+ private static Set<String>
materializeDeferredPartitions(PluginDrivenExternalTable table, LogicalFileScan
scan,
+ CascadesContext context) {
+ Optional<MvccSnapshot> snapshot =
context.getStatementContext().getSnapshot(table,
+ scan.getTableSnapshot(), scan.getScanParams());
+ Optional<Map<String, PartitionItem>> partitions =
table.getNameToPartitionItemsForScan(snapshot);
Review Comment:
[P1] Materialize this view before taking planner table locks
`afterRewrite` runs after `StatementContext.lock()`, and an unfiltered Hive
scan reaches this call with a cold `DEFERRED` view. The call then performs the
full connector/HMS listing and builds every partition item while read locks for
any internal tables in the same query remain held, blocking DDL and metadata
writers for unbounded external latency and O(all partitions) work. This is a
separate no-filter MV-collection path from the existing predicate-pruning lock
thread. Please preload/reuse the pinned full view before locking, and cover a
mixed internal/Hive MV-rewrite plan with a slow listing.
--
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]
