korlov42 commented on code in PR #2443:
URL: https://github.com/apache/ignite-3/pull/2443#discussion_r1298424586
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/rel/AbstractSetOpNode.java:
##########
@@ -305,40 +314,29 @@ protected List<RowT> getOnMapper(int cnt) {
return res;
}
- protected List<RowT> getOnSingleOrReducer(int cnt) {
+ private List<RowT> getResultRows(int cnt) {
Iterator<Map.Entry<GroupKey, int[]>> it =
groups.entrySet().iterator();
-
List<RowT> res = new ArrayList<>(cnt);
while (it.hasNext() && cnt > 0) {
Map.Entry<GroupKey, int[]> entry = it.next();
-
- GroupKey key = entry.getKey();
-
- Object[] fields = new Object[key.fieldsCount()];
-
- for (int i = 0; i < fields.length; i++) {
- fields[i] = key.field(i);
- }
-
- RowT row = rowFactory.create(fields);
-
- int[] cntrs = entry.getValue();
-
+ RowT row = createOutputRow(entry);
int availableRows = availableRows(entry.getValue());
+ updateAvailableRows(entry.getValue(), availableRows);
+
if (availableRows <= cnt) {
it.remove();
- if (availableRows == 0) {
- continue;
- }
-
cnt -= availableRows;
} else {
availableRows = cnt;
- decrementAvailableRows(cntrs, availableRows);
+ assert availableRows > 0;
+ assert all : "This branch should only be accessible for
non distinct variant of a set operator";
+
+ int[] cntrs = entry.getValue();
+ cntrs[0] -= availableRows;
Review Comment:
position to decrement is an internal thing of particular Grouping
implementation, thus I would prefer to hide it in `decrementAvailableRows` as
it was before
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/rel/set/IgniteColocatedIntersect.java:
##########
@@ -30,6 +30,14 @@
* Physical node for INTERSECT operator which inputs are colocated.
*/
public class IgniteColocatedIntersect extends IgniteIntersect implements
IgniteColocatedSetOp {
+ /**
+ * Constructor.
+ *
+ * @param cluster Cluster that this relational expression belongs to.
+ * @param traitSet The traits of this rel.
Review Comment:
you've got quite peculiar spacing between param name and its description.
Please fix it everywhere in the patch
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/rel/AbstractSetOpNode.java:
##########
@@ -195,31 +196,32 @@ private void flush() throws Exception {
}
/**
- * Grouping.
- * TODO Documentation https://issues.apache.org/jira/browse/IGNITE-15859
+ * Grouping provides base driver code to implement a set operator.
+ * The basic idea is to store the number of distinct rows per input set
and use these numbers to calculate
+ * the number of rows an operator should produce.
*/
protected abstract static class Grouping<RowT> {
protected final Map<GroupKey, int[]> groups = new HashMap<>();
- protected final RowHandler<RowT> hnd;
-
protected final AggregateType type;
protected final boolean all;
protected final RowFactory<RowT> rowFactory;
- /** Processed rows count in current set. */
- protected int rowsCnt = 0;
+ private final RowHandler<RowT> hnd;
- protected Grouping(ExecutionContext<RowT> ctx, RowFactory<RowT>
rowFactory, AggregateType type, boolean all) {
+ private final int columnNum;
+
+ protected Grouping(ExecutionContext<RowT> ctx, RowFactory<RowT>
rowFactory, int columnNum, AggregateType type, boolean all) {
Review Comment:
```suggestion
protected Grouping(ExecutionContext<RowT> ctx, RowFactory<RowT>
rowFactory, int columnNum, AggregateType type, boolean all) {
```
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/rel/set/IgniteReduceIntersect.java:
##########
@@ -34,55 +34,68 @@
* Physical node for REDUCE phase of INTERSECT operator.
*/
public class IgniteReduceIntersect extends IgniteIntersect implements
IgniteReduceSetOp {
+
+ private final int inputsNum;
Review Comment:
do we really need this as an explicit field? Is it possible compute this as
difference `rowSize(inputOfReduce) - rowSize(reduce)`?
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/rel/set/IgniteMapSetOp.java:
##########
@@ -66,6 +68,32 @@ default RelDataType buildRowType() {
return builder.build();
}
+ /**
+ * Creates a row type produced by MAP phase of INTERSECT/EXCEPT operator.
+ * For input row (a:type1, b:type2) and {@code inputsNum} = {@code 3} it
produces the following row type:
+ * <pre>
+ * f0: type1
+ * f1: type2
+ * _count_0: int
+ * _count_1: int
+ * _count_2: int
+ * </pre>
+ */
+ public static RelDataType buildRowType(IgniteTypeFactory typeFactory,
RelDataType rowType, int inputsNum) {
Review Comment:
I like the idea to keep utility functions close to the place of usage
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/rel/AbstractSetOpNode.java:
##########
@@ -195,31 +196,32 @@ private void flush() throws Exception {
}
/**
- * Grouping.
- * TODO Documentation https://issues.apache.org/jira/browse/IGNITE-15859
+ * Grouping provides base driver code to implement a set operator.
+ * The basic idea is to store the number of distinct rows per input set
and use these numbers to calculate
Review Comment:
according to our guidelines, every paragraph should start with `<p>` tag and
have one blank line above
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/rel/set/IgniteMapSetOp.java:
##########
@@ -52,16 +53,28 @@ default List<Pair<RelTraitSet, List<RelTraitSet>>>
deriveDistribution(
);
}
- /** Build RowType for MAP node. */
- default RelDataType buildRowType() {
- RelDataTypeFactory typeFactory = Commons.typeFactory(getCluster());
-
- assert typeFactory instanceof IgniteTypeFactory;
-
+ /**
+ * Creates a row type produced by MAP phase of INTERSECT/EXCEPT operator.
+ * For input row (a:type1, b:type2) and {@code inputsNum} = {@code 3} it
produces the following row type:
Review Comment:
```suggestion
* Creates a row type produced by MAP phase of INTERSECT/EXCEPT operator.
*
* <p>For input row (a:type1, b:type2) and {@code inputsNum} = {@code 3}
it produces the following row type:
```
--
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]
