Address code review comments

finos · Feb 27, 2024 · 79c4eee · 79c4eee
1 parent 07577da
commit 79c4eee
Show file tree

Hide file tree

Showing 9 changed files with 50 additions and 20 deletions.
diff --git a/...rsioning/DeriveDataErrorsLogicalPlan.java → ...oning/DeriveDataErrorRowsLogicalPlan.java b/...rsioning/DeriveDataErrorsLogicalPlan.java → ...oning/DeriveDataErrorRowsLogicalPlan.java
@@ -1,4 +1,4 @@
-// Copyright 2023 Goldman Sachs
+// Copyright 2024 Goldman Sachs
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -23,14 +23,14 @@
 import java.util.ArrayList;
 import java.util.List;
 
-public class DeriveDataErrorsLogicalPlan implements VersioningStrategyVisitor<LogicalPlan>
+public class DeriveDataErrorRowsLogicalPlan implements VersioningStrategyVisitor<LogicalPlan>
 {
     private List<String> primaryKeys;
     private List<String> remainingColumns;
     private Dataset tempStagingDataset;
     private int sampleRowCount;
 
-    public DeriveDataErrorsLogicalPlan(List<String> primaryKeys, List<String> remainingColumns, Dataset tempStagingDataset, int sampleRowCount)
+    public DeriveDataErrorRowsLogicalPlan(List<String> primaryKeys, List<String> remainingColumns, Dataset tempStagingDataset, int sampleRowCount)
     {
         this.primaryKeys = primaryKeys;
         this.remainingColumns = remainingColumns;

diff --git a/...ning/DeriveDataErrorCheckLogicalPlan.java → ...ioning/DeriveMaxDataErrorLogicalPlan.java b/...ning/DeriveDataErrorCheckLogicalPlan.java → ...ioning/DeriveMaxDataErrorLogicalPlan.java
@@ -25,14 +25,14 @@
 import java.util.ArrayList;
 import java.util.List;
 
-public class DeriveDataErrorCheckLogicalPlan implements VersioningStrategyVisitor<LogicalPlan>
+public class DeriveMaxDataErrorLogicalPlan implements VersioningStrategyVisitor<LogicalPlan>
 {
 
     List<String> primaryKeys;
     List<String> remainingColumns;
     Dataset tempStagingDataset;
 
-    public DeriveDataErrorCheckLogicalPlan(List<String> primaryKeys, List<String> remainingColumns, Dataset tempStagingDataset)
+    public DeriveMaxDataErrorLogicalPlan(List<String> primaryKeys, List<String> remainingColumns, Dataset tempStagingDataset)
     {
         this.primaryKeys = primaryKeys;
         this.remainingColumns = remainingColumns;

diff --git a/...al-plan/src/main/java/org/finos/legend/engine/persistence/components/planner/Planner.java b/...al-plan/src/main/java/org/finos/legend/engine/persistence/components/planner/Planner.java
@@ -446,13 +446,13 @@ protected void addDataErrorCheck(Map<DedupAndVersionErrorSqlType, LogicalPlan> d
         List<String> remainingColumns = getDigestOrRemainingColumns();
         if (ingestMode.versioningStrategy().accept(VersioningVisitors.IS_TEMP_TABLE_NEEDED))
         {
-            LogicalPlan logicalPlanForDataErrorCheck = ingestMode.versioningStrategy().accept(new DeriveDataErrorCheckLogicalPlan(primaryKeys, remainingColumns, tempStagingDataset()));
+            LogicalPlan logicalPlanForDataErrorCheck = ingestMode.versioningStrategy().accept(new DeriveMaxDataErrorLogicalPlan(primaryKeys, remainingColumns, tempStagingDataset()));
             if (logicalPlanForDataErrorCheck != null)
             {
                 dedupAndVersioningErrorChecks.put(DedupAndVersionErrorSqlType.MAX_DATA_ERRORS, logicalPlanForDataErrorCheck);
             }
 
-            LogicalPlan logicalPlanForDataErrors = ingestMode.versioningStrategy().accept(new DeriveDataErrorsLogicalPlan(primaryKeys, remainingColumns, tempStagingDataset(), options().sampleRowCount()));
+            LogicalPlan logicalPlanForDataErrors = ingestMode.versioningStrategy().accept(new DeriveDataErrorRowsLogicalPlan(primaryKeys, remainingColumns, tempStagingDataset(), options().sampleRowCount()));
             if (logicalPlanForDataErrors != null)
             {
                 dedupAndVersioningErrorChecks.put(DedupAndVersionErrorSqlType.DATA_ERROR_ROWS, logicalPlanForDataErrors);

diff --git a/...n/src/main/java/org/finos/legend/engine/persistence/components/util/LogicalPlanUtils.java b/...n/src/main/java/org/finos/legend/engine/persistence/components/util/LogicalPlanUtils.java
@@ -93,12 +93,6 @@ private LogicalPlanUtils()
     {
     }
 
-    public static String generateTableNameWithSuffix(String tableName, String suffix)
-    {
-        UUID uuid = UUID.randomUUID();
-        return tableName + UNDERSCORE + suffix + UNDERSCORE + uuid;
-    }
-
     public static Value INFINITE_BATCH_ID()
     {
         return InfiniteBatchIdValue.builder().build();

diff --git a/.../src/main/java/org/finos/legend/engine/persistence/components/util/TableNameGenUtils.java b/.../src/main/java/org/finos/legend/engine/persistence/components/util/TableNameGenUtils.java
@@ -0,0 +1,34 @@
+// Copyright 2024 Goldman Sachs
+//
+// Licensed 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.finos.legend.engine.persistence.components.util;
+
+import java.util.UUID;
+
+import static org.finos.legend.engine.persistence.components.util.LogicalPlanUtils.UNDERSCORE;
+
+public class TableNameGenUtils
+{
+    private static String generateTableSuffix()
+    {
+        UUID uuid = UUID.randomUUID();
+        int uuidHashCode = Math.abs(uuid.hashCode());
+        return UNDERSCORE + "LP" + UNDERSCORE + Integer.toString(uuidHashCode, 36);
+    }
+
+    public static String generateTableName(String baseTableName, String suffix)
+    {
+        return baseTableName + UNDERSCORE + suffix + UNDERSCORE + generateTableSuffix();
+    }
+}
diff --git a/...s/legend/engine/persistence/components/relational/bigquery/executor/BigQueryExecutor.java b/...s/legend/engine/persistence/components/relational/bigquery/executor/BigQueryExecutor.java
@@ -85,8 +85,7 @@ public List<TabularData> executePhysicalPlanAndGetResults(SqlPlan physicalPlan)
     @Override
     public List<TabularData> executePhysicalPlanAndGetResults(SqlPlan physicalPlan, int rows)
     {
-        // TODO to be implemented
-        return null;
+        throw new RuntimeException("Not implemented for Big Query");
     }
 
     @Override

diff --git a/...finos/legend/engine/persistence/components/relational/api/RelationalIngestorAbstract.java b/...finos/legend/engine/persistence/components/relational/api/RelationalIngestorAbstract.java
@@ -42,6 +42,7 @@
 import org.finos.legend.engine.persistence.components.util.LogicalPlanUtils;
 import org.finos.legend.engine.persistence.components.util.MetadataDataset;
 import org.finos.legend.engine.persistence.components.util.MetadataUtils;
+import org.finos.legend.engine.persistence.components.util.TableNameGenUtils;
 import org.finos.legend.engine.persistence.components.util.SchemaEvolutionCapability;
 import org.immutables.value.Value.Default;
 import org.immutables.value.Value.Derived;
@@ -536,6 +537,7 @@ private List<DataError> performDryRun()
     {
         if (enrichedIngestMode instanceof BulkLoad)
         {
+            executor.executePhysicalPlan(generatorResult.dryRunPreActionsSqlPlan());
             return relationalSink().performDryRun(executor, generatorResult.dryRunSqlPlan(), sampleRowCount());
         }
         else
@@ -657,6 +659,7 @@ private Datasets enrichDatasetsAndGenerateOperations(Datasets datasets)
                 .putAllAdditionalMetadata(additionalMetadata())
                 .bulkLoadEventIdValue(bulkLoadEventIdValue())
                 .batchSuccessStatusValue(batchSuccessStatusValue())
+                .sampleRowCount(sampleRowCount())
                 .build();
 
         planner = Planners.get(enrichedDatasets, enrichedIngestMode, plannerOptions(), relationalSink().capabilities());
@@ -748,7 +751,7 @@ private Datasets importExternalDataset(Datasets datasets)
         DatasetReference mainDataSetReference = datasets.mainDataset().datasetReference();
 
         externalDatasetReference = externalDatasetReference
-            .withName(externalDatasetReference.name().isPresent() ? externalDatasetReference.name().get() : LogicalPlanUtils.generateTableNameWithSuffix(mainDataSetReference.name().orElseThrow(IllegalStateException::new), STAGING))
+            .withName(externalDatasetReference.name().isPresent() ? externalDatasetReference.name().get() : TableNameGenUtils.generateTableName(mainDataSetReference.name().orElseThrow(IllegalStateException::new), STAGING))
             .withDatabase(externalDatasetReference.database().isPresent() ? externalDatasetReference.database().get() : mainDataSetReference.database().orElse(null))
             .withGroup(externalDatasetReference.group().isPresent() ? externalDatasetReference.group().get() : mainDataSetReference.group().orElse(null))
             .withAlias(externalDatasetReference.alias().isPresent() ? externalDatasetReference.alias().get() : mainDataSetReference.alias().orElseThrow(RuntimeException::new) + UNDERSCORE + STAGING);

diff --git a/...ational/snowflake/logicalplan/datasets/SnowflakeStagedFilesDatasetPropertiesAbstract.java b/...ational/snowflake/logicalplan/datasets/SnowflakeStagedFilesDatasetPropertiesAbstract.java
@@ -43,10 +43,10 @@ default boolean dryRunSupported()
     {
         // Only supported for CSV
         boolean dryRunSupported = false;
-        if (fileFormat().isPresent() && fileFormat().get() instanceof StandardFileFormatAbstract)
+        if (fileFormat().isPresent() && fileFormat().get() instanceof StandardFileFormat)
         {
-            StandardFileFormatAbstract standardFileFormatAbstract = (StandardFileFormatAbstract) fileFormat().get();
-            dryRunSupported = standardFileFormatAbstract.formatType().equals(FileFormatType.CSV);
+            StandardFileFormat standardFileFormat = (StandardFileFormat) fileFormat().get();
+            dryRunSupported = standardFileFormat.formatType().equals(FileFormatType.CSV);
         }
 
         return dryRunSupported;

diff --git a/...ersistence/components/relational/snowflake/sqldom/schemaops/statements/CopyStatement.java b/...ersistence/components/relational/snowflake/sqldom/schemaops/statements/CopyStatement.java
@@ -223,7 +223,7 @@ void validate() throws SqlDomException
     {
         if (srcTable == null)
         {
-            throw new SqlDomException("selectStatement is mandatory for Copy Table Command");
+            throw new SqlDomException("srcTable is mandatory for Copy Table Command");
         }
 
         if (table == null)