korlov42 commented on code in PR #3335:
URL: https://github.com/apache/ignite-3/pull/3335#discussion_r1524745868


##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/framework/TestBuilders.java:
##########
@@ -335,6 +339,16 @@ public interface ClusterBuilder {
          */
         ClusterBuilder dataProvider(String nodeName, String tableName, 
ScannableTable table);
 
+        /**
+         * Provides implementation of table with given name local per given 
node.
+         *
+         * @param nodeNames Names of the nodes given instance of table will be 
assigned to.
+         * @param tableName Name of the table given instance represents.
+         * @param table Actual table that will be used for read operations 
during execution.
+         * @return {@code this} for chaining.
+         */
+        ClusterBuilder dataProviders(List<String> nodeNames, String tableName, 
ScannableTable table);

Review Comment:
   If you want to specify the same table for every node, just call  
`dataProvider` several times for every node



##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/pruning/PartitionPruningMetadataExtractor.java:
##########
@@ -138,6 +152,177 @@ public IgniteRel visit(IgniteTableScan rel) {
         return rel;
     }
 
+    private static class ModifyNodeShuttle extends IgniteRelShuttle {
+        List<RexNode> projections = null;
+        List<List<RexNode>> expressions = null;
+        boolean unionRaised = false;
+
+        /** {@inheritDoc} */
+        @Override
+        public IgniteRel visit(IgniteProject rel) {
+            if (!unionRaised) {
+                // projection before union
+                assert projections == null;
+                // unexpected branch
+                if (projections != null) {
+                    throw Util.FoundOne.NULL;
+                }
+                projections = rel.getProjects();
+            } else {
+                // projections after union
+                if (expressions == null) {
+                    expressions = new ArrayList<>();
+                }
+                expressions.add(rel.getProjects());

Review Comment:
   what if plan will look like this 
   ```
   UnionAll
     Project
       Project
         Values
       
   ```
   ?



##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/pruning/PartitionPruningMetadataExtractor.java:
##########
@@ -138,6 +152,177 @@ public IgniteRel visit(IgniteTableScan rel) {
         return rel;
     }
 
+    private static class ModifyNodeShuttle extends IgniteRelShuttle {
+        List<RexNode> projections = null;
+        List<List<RexNode>> expressions = null;
+        boolean unionRaised = false;
+
+        /** {@inheritDoc} */
+        @Override
+        public IgniteRel visit(IgniteProject rel) {
+            if (!unionRaised) {
+                // projection before union
+                assert projections == null;

Review Comment:
   we never should put assertion like this. We may disable ProjectMerge rule, 
and the queries should not fail with random assertions



##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/framework/TestBuilders.java:
##########
@@ -127,7 +131,7 @@
 /**
  * A collection of builders to create test objects.
  */
-public class TestBuilders {
+public class TestBuilders extends BaseIgniteAbstractTest {

Review Comment:
   TestBuilders should never extends any test class



##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/framework/TestBuilders.java:
##########
@@ -1298,7 +1321,12 @@ public ScannableTable scannableTable() {
 
                 @Override
                 public UpdatableTable updatableTable() {
-                    throw new UnsupportedOperationException();
+                    UpdatableTable updatableTable = mock(UpdatableTable.class);
+                    
when(updatableTable.descriptor()).thenReturn(tableDescriptor());

Review Comment:
   if you need particular implementation of UpdatableTable for certain test, 
then it's better to provide it via cluster builder



##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/framework/TestNode.java:
##########
@@ -191,6 +193,10 @@ HybridClock clock() {
      * @param plan A plan to execute.
      * @return A cursor representing the result.
      */
+    public AsyncCursor<InternalSqlRow> executePlan(QueryPlan plan, @Nullable 
InternalTransaction transaction) {
+        return executionService.executePlan(transaction == null ? new 
NoOpTransaction(nodeName) : transaction, plan, createContext());
+    }
+
     public AsyncCursor<InternalSqlRow> executePlan(QueryPlan plan) {

Review Comment:
   javadoc is missed



-- 
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]

Reply via email to