Jibing-Li commented on code in PR #38990:
URL: https://github.com/apache/doris/pull/38990#discussion_r1706485695


##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonExternalTable.java:
##########
@@ -187,22 +187,17 @@ public BaseAnalysisTask createAnalysisTask(AnalysisInfo 
info) {
     @Override
     public long fetchRowCount() {
         makeSureInitialized();
-        try {
-            long rowCount = 0;
-            Optional<SchemaCacheValue> schemaCacheValue = 
getSchemaCacheValue();
-            Table paimonTable = schemaCacheValue.map(value -> 
((PaimonSchemaCacheValue) value).getPaimonTable())
-                    .orElse(null);
-            if (paimonTable == null) {
-                return -1;
-            }
-            List<Split> splits = 
paimonTable.newReadBuilder().newScan().plan().splits();
-            for (Split split : splits) {
-                rowCount += split.rowCount();
-            }
-            return rowCount;
-        } catch (Exception e) {
-            LOG.warn("Fail to collect row count for db {} table {}", dbName, 
name, e);
+        long rowCount = 0;
+        Optional<SchemaCacheValue> schemaCacheValue = getSchemaCacheValue();
+        Table paimonTable = schemaCacheValue.map(value -> 
((PaimonSchemaCacheValue) value).getPaimonTable())
+                .orElse(null);
+        if (paimonTable == null) {
+            return -1;
+        }
+        List<Split> splits = 
paimonTable.newReadBuilder().newScan().plan().splits();

Review Comment:
   This could be expensive when the table is large. But it's not related to 
this pr, we can try to improve this in a separate pr if needed.



##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergUtils.java:
##########
@@ -592,22 +592,17 @@ public static List<Column> getSchema(ExternalCatalog 
catalog, String dbName, Str
      * @return estimated row count
      */
     public static long getIcebergRowCount(ExternalCatalog catalog, String 
dbName, String tbName) {
-        try {
-            Table icebergTable = Env.getCurrentEnv()
-                    .getExtMetaCacheMgr()
-                    .getIcebergMetadataCache()
-                    .getIcebergTable(catalog, dbName, tbName);
-            Snapshot snapshot = icebergTable.currentSnapshot();
-            if (snapshot == null) {
-                // empty table
-                return 0;
-            }
-            Map<String, String> summary = snapshot.summary();
-            return Long.parseLong(summary.get(TOTAL_RECORDS)) - 
Long.parseLong(summary.get(TOTAL_POSITION_DELETES));
-        } catch (Exception e) {
-            LOG.warn("Fail to collect row count for db {} table {}", dbName, 
tbName, e);
+        Table icebergTable = Env.getCurrentEnv()
+                .getExtMetaCacheMgr()
+                .getIcebergMetadataCache()
+                .getIcebergTable(catalog, dbName, tbName);
+        Snapshot snapshot = icebergTable.currentSnapshot();

Review Comment:
   I checked the code, the table may be null when the iceberg metadata cache is 
not loaded. But I think it's not a problem, because the NPE would be caught in 
the caller and return the default value -1. Meanwhile it will trigger iceberg 
metadata cache to load the table so we can get it next time. I think we can fix 
this in a separate pr if needed. But I feel we don't need to do anything about 
it right now.



-- 
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: commits-unsubscr...@doris.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@doris.apache.org
For additional commands, e-mail: commits-h...@doris.apache.org

Reply via email to