Copilot commented on code in PR #7471:
URL: https://github.com/apache/ignite-3/pull/7471#discussion_r2721197786
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/rel/CorrelatedNestedLoopJoinNode.java:
##########
@@ -485,7 +491,14 @@ private Node<RowT> rightSource() {
private void prepareCorrelations() {
for (int i = 0; i < correlationIds.size(); i++) {
RowT row = i < leftInBuf.size() ? leftInBuf.get(i) :
first(leftInBuf);
- context().correlatedVariable(row, correlationIds.get(i).getId());
+ int corrId = correlationIds.get(i).getId();
+
+ for (int fieldIndex = correlationColumns.nextSetBit(0); fieldIndex
!= -1;
+ fieldIndex = correlationColumns.nextSetBit(fieldIndex +
1)) {
+ Object value = context().rowAccessor().get(fieldIndex, row);
+
+ context().correlatedVariable(corrId, fieldIndex, value);
Review Comment:
This change makes correlation propagation dependent on `correlationColumns`
(only those fields are written into the context). There doesn’t appear to be an
execution-level test that exercises a right-side expression reading correlated
values via `SqlEvaluationContext#correlatedVariable(corrId, fieldIndex)`.
Adding a test that uses a correlated predicate/projection would help prevent
regressions where `correlationColumns` is incomplete and stale values are read.
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/rel/IgniteCorrelatedNestedLoopJoin.java:
##########
@@ -148,6 +157,11 @@ public Pair<RelTraitSet, List<RelTraitSet>>
passThroughCollation(
List.of(left.replace(RelCollations.EMPTY), right));
}
+ @Override public RelWriter explainTerms(RelWriter pw) {
+ return super.explainTerms(pw)
+ .itemIf("correlationColumns", correlationColumns,
!correlationColumns.isEmpty());
+ }
Review Comment:
Adding `correlationColumns` to `explainTerms` changes
`RelOptUtil.toString(...)` output for this rel. The codebase has tests that
assert exact plan strings (e.g.,
`modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/planner/PlannerTest.java:184-191`),
so those expected strings will likely need updating to include/exclude this
new attribute.
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ExecutionContext.java:
##########
@@ -359,18 +359,19 @@ public QueryProvider getQueryProvider() {
}
@Override
- public RowT correlatedVariable(int id) {
- return cast(sharedState.correlatedVariable(id));
+ public Object correlatedVariable(int corrId, int fieldIndex) {
Review Comment:
This implementation delegates to `SharedState#correlatedVariable(...)`,
which is `@Nullable`, but the override is not annotated as nullable. Aligning
the nullability contract here (and in `SqlEvaluationContext`) will prevent
incorrect assumptions in generated expression code / nullness tooling.
```suggestion
public @Nullable Object correlatedVariable(int corrId, int fieldIndex) {
```
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/SqlEvaluationContext.java:
##########
@@ -38,6 +38,6 @@ public interface SqlEvaluationContext<RowT> extends
DataContext {
/** Returns a factory capable of producing {@link RowFactory} instances
for the {@code RowT} row representation. */
RowFactoryFactory<RowT> rowFactoryFactory();
- /** Returns row representing correlation source by given correlation id. */
- RowT correlatedVariable(int id);
+ /** Returns the value of a correlated variable identified by the given
correlation id and field index. */
+ Object correlatedVariable(int corrId, int fieldIndex);
Review Comment:
`correlatedVariable(...)` can legitimately return `null` (e.g., value not
present in `SharedState`, or correlated field value is `NULL`). Consider
annotating the return type as `@Nullable` to match
`SharedState#correlatedVariable` and avoid misleading nullness contracts for
expression evaluators.
```suggestion
@org.jetbrains.annotations.Nullable Object correlatedVariable(int
corrId, int fieldIndex);
```
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/message/SqlQueryMessageGroup.java:
##########
@@ -20,6 +20,18 @@
import static
org.apache.ignite.internal.sql.engine.message.SqlQueryMessageGroup.GROUP_TYPE;
import org.apache.ignite.internal.network.annotations.MessageGroup;
+import org.apache.ignite.internal.sql.engine.message.field.BooleanValueMessage;
+import
org.apache.ignite.internal.sql.engine.message.field.ByteArrayValueMessage;
+import org.apache.ignite.internal.sql.engine.message.field.ByteValueMessage;
+import org.apache.ignite.internal.sql.engine.message.field.DecimalValueMessage;
+import org.apache.ignite.internal.sql.engine.message.field.DoubleValueMessage;
Review Comment:
The imports added for the field message types appear to be used only in
Javadoc. With the repo’s Checkstyle rule `UnusedImports`
(check-rules/checkstyle-rules.xml), these will be flagged as unused and fail
the build. Consider removing the imports and using fully-qualified names in the
`{@link ...}` tags (or otherwise referencing these types in code).
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/rel/CorrelatedNestedLoopJoinNode.java:
##########
@@ -77,20 +80,22 @@ private enum State {
/**
* Creates CorrelatedNestedLoopJoin node.
*
- * @param ctx Execution context.
+ * @param ctx Execution context.
* @param cond Join expression.
* @param correlationIds Set of collections ids.
Review Comment:
Javadoc typo: `correlationIds` is described as "Set of collections ids" but
this is a correlation join; should be "Set of correlation ids".
```suggestion
* @param correlationIds Set of correlation ids.
```
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/expressions/SqlExpressionFactoryAdapter.java:
##########
@@ -303,7 +303,7 @@ public RowFactoryFactory<RowT> rowFactoryFactory() {
}
@Override
- public @Nullable RowT correlatedVariable(int id) {
+ public @Nullable RowT correlatedVariable(int corrId, int fieldIndex) {
return null;
Review Comment:
`SqlEvaluationContext#correlatedVariable(...)` now represents a single
*field value* and returns `Object`, but this adapter still declares it as
returning `RowT`. Even though it currently returns `null`, the signature is
misleading and diverges from the new API contract; consider changing it to
`@Nullable Object` (and ideally fail-fast if it is ever invoked in this
context).
```suggestion
public @Nullable Object correlatedVariable(int corrId, int
fieldIndex) {
throw new AssertionError("Correlated variables are not supported
in this evaluation context");
```
--
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]
