[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #9875: Update cardinality when converting raw column to dict based

[GitBox <gi...@apache.org>]

Jackie-Jiang commented on code in PR #9875:
URL: https://github.com/apache/pinot/pull/9875#discussion_r1041377162


##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/BaseClusterIntegrationTestSet.java:
##########
@@ -606,6 +608,37 @@ public void testReset(TableType tableType)
     }, 30_000L, "Failed to wait for all segments come back online");
   }
 
+  public String reloadTableAndValidateResponse(String tableName, TableType tableType, boolean forceDownload)
+      throws IOException {
+    String jobId = null;
+    String response =
+        sendPostRequest(_controllerRequestURLBuilder.forTableReload(tableName, tableType, forceDownload), null);
+    String tableNameWithType = TableNameBuilder.forType(tableType).tableNameWithType(tableName);
+    JsonNode tableLevelDetails =
+        JsonUtils.stringToJsonNode(StringEscapeUtils.unescapeJava(response.split(": ")[1])).get(tableNameWithType);
+    String isZKWriteSuccess = tableLevelDetails.get("reloadJobMetaZKStorageStatus").asText();
+    assertEquals("SUCCESS", isZKWriteSuccess);

Review Comment:
   (minor, not introduced in this PR)
   ```suggestion
       assertEquals(isZKWriteSuccess, "SUCCESS");
   ```



##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/BaseClusterIntegrationTestSet.java:
##########
@@ -606,6 +608,37 @@ public void testReset(TableType tableType)
     }, 30_000L, "Failed to wait for all segments come back online");
   }
 
+  public String reloadTableAndValidateResponse(String tableName, TableType tableType, boolean forceDownload)
+      throws IOException {
+    String jobId = null;
+    String response =
+        sendPostRequest(_controllerRequestURLBuilder.forTableReload(tableName, tableType, forceDownload), null);
+    String tableNameWithType = TableNameBuilder.forType(tableType).tableNameWithType(tableName);
+    JsonNode tableLevelDetails =
+        JsonUtils.stringToJsonNode(StringEscapeUtils.unescapeJava(response.split(": ")[1])).get(tableNameWithType);
+    String isZKWriteSuccess = tableLevelDetails.get("reloadJobMetaZKStorageStatus").asText();
+    assertEquals("SUCCESS", isZKWriteSuccess);
+    jobId = tableLevelDetails.get("reloadJobId").asText();

Review Comment:
   (minor) Remove the declaration on line 613
   ```suggestion
       String jobId = tableLevelDetails.get("reloadJobId").asText();
   ```



##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/LLCRealtimeClusterIntegrationTest.java:
##########
@@ -206,7 +206,7 @@ public void setUp()
   @Override
   public void tearDown()
       throws Exception {
-    FileUtils.deleteDirectory(new File(CONSUMER_DIRECTORY));
+     FileUtils.deleteDirectory(new File(CONSUMER_DIRECTORY));

Review Comment:
   (nit) revert



##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/BaseClusterIntegrationTestSet.java:
##########
@@ -606,6 +608,37 @@ public void testReset(TableType tableType)
     }, 30_000L, "Failed to wait for all segments come back online");
   }
 
+  public String reloadTableAndValidateResponse(String tableName, TableType tableType, boolean forceDownload)
+      throws IOException {
+    String jobId = null;
+    String response =
+        sendPostRequest(_controllerRequestURLBuilder.forTableReload(tableName, tableType, forceDownload), null);
+    String tableNameWithType = TableNameBuilder.forType(tableType).tableNameWithType(tableName);
+    JsonNode tableLevelDetails =
+        JsonUtils.stringToJsonNode(StringEscapeUtils.unescapeJava(response.split(": ")[1])).get(tableNameWithType);
+    String isZKWriteSuccess = tableLevelDetails.get("reloadJobMetaZKStorageStatus").asText();
+    assertEquals("SUCCESS", isZKWriteSuccess);
+    jobId = tableLevelDetails.get("reloadJobId").asText();
+    String jobStatusResponse = sendGetRequest(_controllerRequestURLBuilder.forControllerJobStatus(jobId));
+    JsonNode jobStatus = JsonUtils.stringToJsonNode(jobStatusResponse);
+
+    // Validate all fields are present
+    assertEquals(jobStatus.get("metadata").get("jobId").asText(), jobId);
+    assertEquals(jobStatus.get("metadata").get("jobType").asText(), "RELOAD_ALL_SEGMENTS");
+    assertEquals(jobStatus.get("metadata").get("tableName").asText(), tableNameWithType);
+    return jobId;
+  }
+
+  public boolean isReloadJobCompleted(String reloadJobId)

Review Comment:
   We can remove the `OfflineClusterIntegrationTest.validateReloadJobSuccess()` and replace all usage with `assertTrue(isReloadJobCompleted(jobId)`. Currently we didn't really validate if the reload is done



##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/LLCRealtimeClusterIntegrationTest.java:
##########
@@ -277,6 +277,48 @@ public void testReload()
     testReload(false);
   }
 
+  @Test
+  public void testEnableDisableDictionaryWithReload()
+      throws Exception {
+    String query = "SELECT * FROM myTable WHERE ActualElapsedTime > 0";
+    JsonNode queryResponse = postQuery(query);
+    long numTotalDocs = queryResponse.get("totalDocs").asLong();
+
+    // Enable dictionary.
+    TableConfig tableConfig = getRealtimeTableConfig();
+    tableConfig.getIndexingConfig().getNoDictionaryColumns().remove("ActualElapsedTime");
+    updateTableConfig(tableConfig);
+    String enableDictReloadId = reloadTableAndValidateResponse(getTableName(), TableType.REALTIME, false);
+    TestUtils.waitForCondition(aVoid -> {
+      try {
+        return isReloadJobCompleted(enableDictReloadId);
+      } catch (Exception e) {
+        throw new RuntimeException(e);
+      }
+    }, 60_000L, "Failed to reload.");
+
+    queryResponse = postQuery(query);
+    long numTotalDocsAfterReload = queryResponse.get("totalDocs").asLong();
+    assertEquals(numTotalDocs, numTotalDocsAfterReload);

Review Comment:
   We want to validate if total doc and query response is always correct during the reload. We can also pick a query that can return different metadata based on whether the column is dictionary encoded to ensure the changing index actually happens. E.g. if we pick a value that doesn't exist, `SELECT COUNT(*) FROM myTable WHERE ActualElapsedTime = <val>` will have docs scanned in filter for raw index, but no scan for dictionary



-- 
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-unsubscribe@pinot.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org