Address review feedback for system tables

Author: xiangfu0

pinot-broker/pom.xml

@@ -36,7 +36,6 @@
     <dependency>
       <groupId>org.apache.pinot</groupId>
       <artifactId>pinot-java-client</artifactId>
-      <version>${project.version}</version>
     </dependency>
     <dependency>
       <groupId>org.apache.pinot</groupId>

pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java

@@ -1155,19 +1155,17 @@ private BrokerResponseNative buildSystemTableBrokerResponse(String tableName, Sc
     DataSchema dataSchema = buildSystemTableDataSchema(schema, projectionColumns);
     List<GenericRow> rows = response != null ? response.getRows() : Collections.emptyList();
     List<Object[]> resultRows = new ArrayList<>();
-    if (rows != null) {
-      for (GenericRow row : rows) {
-        Object[] values = new Object[projectionColumns.size()];
-        for (int i = 0; i < projectionColumns.size(); i++) {
-          values[i] = row.getValue(projectionColumns.get(i));
-        }
-        resultRows.add(values);
+    for (GenericRow row : rows) {
+      Object[] values = new Object[projectionColumns.size()];
+      for (int i = 0; i < projectionColumns.size(); i++) {
+        values[i] = row.getValue(projectionColumns.get(i));
       }
+      resultRows.add(values);
     }
     BrokerResponseNative brokerResponse = new BrokerResponseNative();
     brokerResponse.setResultTable(new ResultTable(dataSchema, resultRows));
     brokerResponse.setNumDocsScanned(resultRows.size());
-    brokerResponse.setNumEntriesScannedPostFilter(resultRows.size());
+    brokerResponse.setNumEntriesScannedPostFilter(response != null ? response.getTotalRows() : 0);
     if (response != null) {
       brokerResponse.setTotalDocs(response.getTotalRows());
     }

pinot-broker/src/test/java/org/apache/pinot/broker/requesthandler/SystemTableBrokerRequestHandlerTest.java

@@ -152,7 +152,21 @@ public SystemTableResponse getRows(SystemTableRequest request)
       row2.putValue("status", "OFFLINE");
       rows.add(row2);
 
-      return new SystemTableResponse(rows, System.currentTimeMillis(), rows.size());
+      int totalRows = rows.size();
+      int offset = Math.max(0, request.getOffset());
+      int limit = request.getLimit();
+      if (offset > 0) {
+        if (offset >= rows.size()) {
+          return new SystemTableResponse(List.of(), System.currentTimeMillis(), totalRows);
+        }
+        rows = new ArrayList<>(rows.subList(offset, rows.size()));
+      }
+      if (limit == 0) {
+        rows = List.of();
+      } else if (limit > 0 && rows.size() > limit) {
+        rows = new ArrayList<>(rows.subList(0, limit));
+      }
+      return new SystemTableResponse(rows, System.currentTimeMillis(), totalRows);
     }
   }
 }

pinot-common/src/main/java/org/apache/pinot/common/systemtable/provider/TablesSystemTableProvider.java

@@ -72,6 +72,18 @@ public final class TablesSystemTableProvider implements SystemTableProvider {
       .addSingleValueDimension("tableConfig", FieldSpec.DataType.STRING)
       .build();
 
+  private static final boolean IS_ADMIN_CLIENT_AVAILABLE;
+  static {
+    boolean available;
+    try {
+      Class.forName("org.apache.pinot.client.admin.PinotAdminClient");
+      available = true;
+    } catch (ClassNotFoundException e) {
+      available = false;
+    }
+    IS_ADMIN_CLIENT_AVAILABLE = available;
+  }
+
   private final TableCache _tableCache;
   private final @Nullable HelixAdmin _helixAdmin;
   private final @Nullable String _clusterName;
@@ -130,12 +142,12 @@ public SystemTableResponse getRows(SystemTableRequest request) {
     List<String> sortedTableNames = new ArrayList<>(tableNamesWithType);
     sortedTableNames.sort(Comparator.naturalOrder());
 
-    List<GenericRow> rows = new ArrayList<>(sortedTableNames.size());
-    int offset = request.getOffset();
+    int offset = Math.max(0, request.getOffset());
     int limit = request.getLimit();
-    if (limit == 0) {
-      return new SystemTableResponse(List.of(), System.currentTimeMillis(), 0);
-    }
+    boolean hasLimit = limit > 0;
+    int totalRows = 0;
+    int initialCapacity = hasLimit ? Math.min(sortedTableNames.size(), limit) : 0;
+    List<GenericRow> rows = new ArrayList<>(initialCapacity);
     for (String tableNameWithType : sortedTableNames) {
       TableType tableType = TableNameBuilder.getTableTypeFromTableName(tableNameWithType);
       if (tableType == null) {
@@ -146,6 +158,17 @@ public SystemTableResponse getRows(SystemTableRequest request) {
       if (!matchesFilter(request.getFilter(), stats, rawTableName)) {
         continue;
       }
+      totalRows++;
+      if (offset > 0) {
+        offset--;
+        continue;
+      }
+      if (limit == 0) {
+        continue;
+      }
+      if (hasLimit && rows.size() >= limit) {
+        continue;
+      }
       GenericRow row = new GenericRow();
       row.putValue("tableName", rawTableName);
       row.putValue("type", stats._type);
@@ -159,16 +182,9 @@ public SystemTableResponse getRows(SystemTableRequest request) {
       row.putValue("serverTenant", stats._serverTenant);
       row.putValue("replicas", stats._replicas);
       row.putValue("tableConfig", stats._tableConfig);
-      if (offset > 0) {
-        offset--;
-        continue;
-      }
       rows.add(row);
-      if (limit > 0 && rows.size() >= limit) {
-        break;
-      }
     }
-    return new SystemTableResponse(rows, System.currentTimeMillis(), rows.size());
+    return new SystemTableResponse(rows, System.currentTimeMillis(), totalRows);
   }
 
   private TableStats buildStats(String tableNameWithType, TableType tableType) {
@@ -364,7 +380,7 @@ private boolean matchesNumber(List<String> candidates, long actual, SystemTableF
               baseUrl, response.statusCode(), response.body());
         }
       } catch (Exception e) {
-        LOGGER.warn("system.tables: error fetching table size for {} via {}: {}", tableName, baseUrl, e.toString(), e);
+        LOGGER.warn("system.tables: error fetching table size for {} via {}", tableName, baseUrl, e);
       }
     }
     return null;
@@ -453,13 +469,10 @@ private List<String> discoverControllersFromHelix() {
   }
 
   private boolean isAdminClientAvailable() {
-    try {
-      Class.forName("org.apache.pinot.client.admin.PinotAdminClient");
-      return true;
-    } catch (ClassNotFoundException e) {
+    if (!IS_ADMIN_CLIENT_AVAILABLE) {
       LOGGER.debug("PinotAdminClient not on classpath; falling back to HTTP for table size fetch");
-      return false;
     }
+    return IS_ADMIN_CLIENT_AVAILABLE;
   }
 
   private @Nullable TableSize fetchWithAdminClient(List<String> controllers, String tableNameWithType) {

pinot-common/src/main/java/org/apache/pinot/common/utils/DatabaseUtils.java

@@ -67,9 +67,12 @@ public static String translateTableName(String tableName, @Nullable String datab
       case 2:
         Preconditions.checkArgument(!tableSplit[1].isEmpty(), "Invalid table name '%s'", tableName);
         String databasePrefix = tableSplit[0];
-        // Allow system tables to bypass database prefix validation so they can be queried regardless of header.
-        // System tables are globally accessible and not subject to database-level access controls, so skip validation.
-        // Ensure system tables do not expose sensitive information because this bypass is intentional.
+        /*
+         * Allow system tables to bypass database prefix validation so they can be queried regardless of the database
+         * header. System tables are intended to be globally accessible and are not subject to database-level access
+         * controls. Ensure system tables do not expose sensitive information because they can be queried without a
+         * matching database context.
+         */
         if ("system".equalsIgnoreCase(databasePrefix)) {
           return tableName;
         }