Copilot commented on code in PR #59281:
URL: https://github.com/apache/doris/pull/59281#discussion_r2647647201
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/source/PaimonScanNode.java:
##########
@@ -413,9 +417,19 @@ public List<org.apache.paimon.table.source.Split>
getPaimonSplitFromAPI() throws
.filter(i -> i >= 0)
.toArray();
ReadBuilder readBuilder = paimonTable.newReadBuilder();
- return readBuilder.withFilter(predicates)
+ TableScan scan = readBuilder.withFilter(predicates)
.withProjection(projected)
- .newScan().plan().splits();
+ .newScan();
+ PaimonMetricRegistry registry = new PaimonMetricRegistry();
+ if (scan instanceof InnerTableScan) {
+ scan = ((InnerTableScan) scan).withMetricsRegistry(registry);
+ }
+ List<org.apache.paimon.table.source.Split> splits =
scan.plan().splits();
+ PaimonScanMetricsReporter.report(source.getTargetTable(),
paimonTable.name(), registry);
+ if (!registry.getAllGroups().isEmpty()) {
+ registry.clear();
+ }
+ return splits;
Review Comment:
The PR description mentions a regression test file
'test_paimon_scan_metrics_profile.groovy' that should verify Paimon Scan
Metrics appear in the query profile, but this test file is not included in the
diff. The test coverage for this feature is incomplete without the automated
test.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/profile/PaimonScanMetricsReporter.java:
##########
@@ -0,0 +1,152 @@
+// 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.paimon.profile;
+
+import org.apache.doris.catalog.TableIf;
+import org.apache.doris.common.profile.RuntimeProfile;
+import org.apache.doris.common.profile.SummaryProfile;
+import org.apache.doris.common.util.DebugUtil;
+import org.apache.doris.qe.ConnectContext;
+
+import org.apache.paimon.metrics.Counter;
+import org.apache.paimon.metrics.Gauge;
+import org.apache.paimon.metrics.Histogram;
+import org.apache.paimon.metrics.HistogramStatistics;
+import org.apache.paimon.metrics.Metric;
+import org.apache.paimon.metrics.MetricGroup;
+import org.apache.paimon.operation.metrics.ScanMetrics;
+
+import java.util.Map;
+import java.util.concurrent.TimeUnit;
+
+public class PaimonScanMetricsReporter {
+ private static final double P95 = 0.95d;
+
+ public static void report(TableIf table, String paimonTableName,
PaimonMetricRegistry registry) {
+ if (registry == null || paimonTableName == null) {
+ return;
+ }
+ String resolvedTableName = paimonTableName;
+ MetricGroup group = registry.getGroup(ScanMetrics.GROUP_NAME,
paimonTableName);
+ if (group == null) {
+ String prefix = ScanMetrics.GROUP_NAME + ":";
+ for (Map.Entry<String, MetricGroup> entry :
registry.getAllGroupsAsMap().entrySet()) {
+ String key = entry.getKey();
+ if (!key.startsWith(prefix)) {
+ continue;
+ }
+ if (group != null) {
+ group = null;
+ break;
+ }
+ group = entry.getValue();
+ resolvedTableName = key.substring(prefix.length());
+ }
+ }
Review Comment:
The fallback logic that iterates through all groups when direct lookup fails
has a confusing flow. When multiple ScanMetrics groups are found (lines 53-56),
the code sets group to null and breaks, effectively skipping metrics reporting
silently. Consider adding a debug or warning log message to indicate why
metrics are being skipped in this scenario to aid troubleshooting.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/source/PaimonScanNode.java:
##########
@@ -413,9 +417,19 @@ public List<org.apache.paimon.table.source.Split>
getPaimonSplitFromAPI() throws
.filter(i -> i >= 0)
.toArray();
ReadBuilder readBuilder = paimonTable.newReadBuilder();
- return readBuilder.withFilter(predicates)
+ TableScan scan = readBuilder.withFilter(predicates)
.withProjection(projected)
- .newScan().plan().splits();
+ .newScan();
+ PaimonMetricRegistry registry = new PaimonMetricRegistry();
+ if (scan instanceof InnerTableScan) {
+ scan = ((InnerTableScan) scan).withMetricsRegistry(registry);
+ }
+ List<org.apache.paimon.table.source.Split> splits =
scan.plan().splits();
+ PaimonScanMetricsReporter.report(source.getTargetTable(),
paimonTable.name(), registry);
+ if (!registry.getAllGroups().isEmpty()) {
+ registry.clear();
+ }
+ return splits;
Review Comment:
If an exception occurs during scan.plan().splits() execution (line 427), the
registry will not be cleaned up, potentially causing a resource leak. Consider
wrapping the split planning and metrics reporting in a try-finally block to
ensure the registry is always cleared, even when exceptions occur.
```suggestion
List<org.apache.paimon.table.source.Split> splits;
try {
splits = scan.plan().splits();
PaimonScanMetricsReporter.report(source.getTargetTable(),
paimonTable.name(), registry);
return splits;
} finally {
if (!registry.getAllGroups().isEmpty()) {
registry.clear();
}
}
```
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/profile/PaimonMetricRegistry.java:
##########
@@ -0,0 +1,72 @@
+// 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.paimon.profile;
+
+import org.apache.paimon.metrics.MetricGroup;
+import org.apache.paimon.metrics.MetricGroupImpl;
+import org.apache.paimon.metrics.MetricRegistry;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Collection;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+
+public class PaimonMetricRegistry extends MetricRegistry {
+ private static final Logger LOG =
LoggerFactory.getLogger(PaimonMetricRegistry.class);
+ private static final String TABLE_TAG_KEY = "table";
+ private final ConcurrentHashMap<String, MetricGroup> groups = new
ConcurrentHashMap<>();
+
+ @Override
+ protected MetricGroup createMetricGroup(String name, Map<String, String>
tags) {
+ MetricGroup group = new MetricGroupImpl(name, tags);
+ String table = tags == null ? "" : tags.getOrDefault(TABLE_TAG_KEY,
"");
+ groups.put(buildKey(name, table), group);
+ LOG.debug("Created metric group: {}:{}", name, table);
+ return group;
+ }
+
+ public MetricGroup getGroup(String name, String table) {
+ String key = buildKey(name, table);
+ MetricGroup group = groups.get(key);
+ if (group == null) {
+ LOG.warn("MetricGroup not found: {}", key);
+ }
+ return group;
+ }
+
+ public void removeGroup(String name, String table) {
+ groups.remove(buildKey(name, table));
+ }
+
+ public Collection<MetricGroup> getAllGroups() {
+ return groups.values();
+ }
+
+ public Map<String, MetricGroup> getAllGroupsAsMap() {
+ return new ConcurrentHashMap<>(groups);
+ }
Review Comment:
The method creates a new ConcurrentHashMap copy of the groups map on every
call. This could be inefficient if called frequently. Consider whether
returning an unmodifiable view or the original map would be more appropriate,
depending on the caller's needs.
--
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]
