You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2022/12/07 07:45:56 UTC
[GitHub] [ignite] alex-plekhanov opened a new pull request, #10424: IGNITE-18341 SQL Calcite: Introduce correlated distribution
alex-plekhanov opened a new pull request, #10424:
URL: https://github.com/apache/ignite/pull/10424
Thank you for submitting the pull request to the Apache Ignite.
In order to streamline the review of the contribution
we ask you to ensure the following steps have been taken:
### The Contribution Checklist
- [ ] There is a single JIRA ticket related to the pull request.
- [ ] The web-link to the pull request is attached to the JIRA ticket.
- [ ] The JIRA ticket has the _Patch Available_ state.
- [ ] The pull request body describes changes that have been made.
The description explains _WHAT_ and _WHY_ was made instead of _HOW_.
- [ ] The pull request title is treated as the final commit message.
The following pattern must be used: `IGNITE-XXXX Change summary` where `XXXX` - number of JIRA issue.
- [ ] A reviewer has been mentioned through the JIRA comments
(see [the Maintainers list](https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute#HowtoContribute-ReviewProcessandMaintainers))
- [ ] The pull request has been checked by the Teamcity Bot and
the `green visa` attached to the JIRA ticket (see [TC.Bot: Check PR](https://mtcga.gridgain.com/prs.html))
### Notes
- [How to Contribute](https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute)
- [Coding abbreviation rules](https://cwiki.apache.org/confluence/display/IGNITE/Abbreviation+Rules)
- [Coding Guidelines](https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines)
- [Apache Ignite Teamcity Bot](https://cwiki.apache.org/confluence/display/IGNITE/Apache+Ignite+Teamcity+Bot)
If you need any help, please email dev@ignite.apache.org or ask anу advice on http://asf.slack.com _#ignite_ channel.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] alex-plekhanov commented on a diff in pull request #10424: IGNITE-18341 SQL Calcite: Introduce correlated distribution
Posted by GitBox <gi...@apache.org>.
alex-plekhanov commented on code in PR #10424:
URL: https://github.com/apache/ignite/pull/10424#discussion_r1044269267
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/trait/IgniteDistributions.java:
##########
@@ -113,6 +114,15 @@ public static IgniteDistribution hash(List<Integer> keys, DistributionFunction f
return canonize(new DistributionTrait(ImmutableIntList.copyOf(keys), function));
}
+ /**
+ * @param corrId Target distribution correlation id.
+ * @param target Target distribution.
+ * @return Distribution by correlate.
+ */
+ public static IgniteDistribution correlated(CorrelationId corrId, IgniteDistribution target) {
Review Comment:
Fixed
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] alex-plekhanov commented on a diff in pull request #10424: IGNITE-18341 SQL Calcite: Introduce correlated distribution
Posted by GitBox <gi...@apache.org>.
alex-plekhanov commented on code in PR #10424:
URL: https://github.com/apache/ignite/pull/10424#discussion_r1044267653
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/trait/DistributionFunction.java:
##########
@@ -316,4 +327,59 @@ public AffinityDistribution(int cacheId, Object identity) {
return "affinity[identity=" + identity + ", cacheId=" + cacheId + ']';
}
}
+
+ /**
+ * Correlated distribution, used to bypass set of nodes on the right hand of CNLJ and to be restored to
+ * original hash distribution (with remapped keys) by the filter node.
+ */
+ public static final class CorrelatedDistribution extends DistributionFunction {
+ /** */
+ private final CorrelationId corrId;
+
+ /** */
+ private final IgniteDistribution target;
+
+ /** */
+ private CorrelatedDistribution(CorrelationId corrId, IgniteDistribution target) {
+ this.corrId = corrId;
+ this.target = target;
+
+ assert target.getType() == RelDistribution.Type.HASH_DISTRIBUTED : target.getType();
+ }
+
+ /** {@inheritDoc} */
+ @Override public RelDistribution.Type type() {
+ return RelDistribution.Type.RANDOM_DISTRIBUTED;
+ }
+
+ /** {@inheritDoc} */
+ @Override public <Row> Destination<Row> destination(
+ ExecutionContext<Row> ctx,
+ AffinityService affSrvc,
+ ColocationGroup target,
+ ImmutableIntList keys
+ ) {
+ throw new AssertionError("Correlated distribution should be converted to delegate before using");
+ }
+
+ /** */
+ public CorrelationId correlationId() {
+ return corrId;
+ }
+
+ /** */
+ public IgniteDistribution target() {
+ return target;
+ }
+
+ /** {@inheritDoc} */
+ @Override public boolean correlated() {
+ return true;
+ }
+
+ /** {@inheritDoc} */
+ @Override protected String name0() {
+ return "correlated[corrId=" + corrId + ", target=" + target + ']';
Review Comment:
But it placed on different positions, for example: `IGNITE.[].correlated[corrId=$cor0, target=affinity[identity=hash, cacheId=-1368047377][0]].rewindable.correlated[$cor0]`, here we know that the first is distribution and the second is correlation. Moreover, for distribution there will be always field `target`, for correlation where is only a set of correlation ids. If you insist on renaming, what name will be better? How about just shortening to `corr`?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] alex-plekhanov commented on a diff in pull request #10424: IGNITE-18341 SQL Calcite: Introduce correlated distribution
Posted by GitBox <gi...@apache.org>.
alex-plekhanov commented on code in PR #10424:
URL: https://github.com/apache/ignite/pull/10424#discussion_r1044267653
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/trait/DistributionFunction.java:
##########
@@ -316,4 +327,59 @@ public AffinityDistribution(int cacheId, Object identity) {
return "affinity[identity=" + identity + ", cacheId=" + cacheId + ']';
}
}
+
+ /**
+ * Correlated distribution, used to bypass set of nodes on the right hand of CNLJ and to be restored to
+ * original hash distribution (with remapped keys) by the filter node.
+ */
+ public static final class CorrelatedDistribution extends DistributionFunction {
+ /** */
+ private final CorrelationId corrId;
+
+ /** */
+ private final IgniteDistribution target;
+
+ /** */
+ private CorrelatedDistribution(CorrelationId corrId, IgniteDistribution target) {
+ this.corrId = corrId;
+ this.target = target;
+
+ assert target.getType() == RelDistribution.Type.HASH_DISTRIBUTED : target.getType();
+ }
+
+ /** {@inheritDoc} */
+ @Override public RelDistribution.Type type() {
+ return RelDistribution.Type.RANDOM_DISTRIBUTED;
+ }
+
+ /** {@inheritDoc} */
+ @Override public <Row> Destination<Row> destination(
+ ExecutionContext<Row> ctx,
+ AffinityService affSrvc,
+ ColocationGroup target,
+ ImmutableIntList keys
+ ) {
+ throw new AssertionError("Correlated distribution should be converted to delegate before using");
+ }
+
+ /** */
+ public CorrelationId correlationId() {
+ return corrId;
+ }
+
+ /** */
+ public IgniteDistribution target() {
+ return target;
+ }
+
+ /** {@inheritDoc} */
+ @Override public boolean correlated() {
+ return true;
+ }
+
+ /** {@inheritDoc} */
+ @Override protected String name0() {
+ return "correlated[corrId=" + corrId + ", target=" + target + ']';
Review Comment:
But it placed on different positions, for example: `IGNITE.[].correlated[corrId=$cor0, target=affinity[identity=hash, cacheId=-1368047377][0]].rewindable.correlated[$cor0]`, here we know that the first is distribution and the second is correlation. Moreover, for distribution there will be always field `target`, for correlation there is only a set of correlation ids. If you insist on renaming, what name will be better? How about just shortening to `corr`?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] korlov42 commented on a diff in pull request #10424: IGNITE-18341 SQL Calcite: Introduce correlated distribution
Posted by GitBox <gi...@apache.org>.
korlov42 commented on code in PR #10424:
URL: https://github.com/apache/ignite/pull/10424#discussion_r1045478387
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/trait/DistributionFunction.java:
##########
@@ -316,4 +327,59 @@ public AffinityDistribution(int cacheId, Object identity) {
return "affinity[identity=" + identity + ", cacheId=" + cacheId + ']';
}
}
+
+ /**
+ * Correlated distribution, used to bypass set of nodes on the right hand of CNLJ and to be restored to
+ * original hash distribution (with remapped keys) by the filter node.
+ */
+ public static final class CorrelatedDistribution extends DistributionFunction {
+ /** */
+ private final CorrelationId corrId;
+
+ /** */
+ private final IgniteDistribution target;
+
+ /** */
+ private CorrelatedDistribution(CorrelationId corrId, IgniteDistribution target) {
+ this.corrId = corrId;
+ this.target = target;
+
+ assert target.getType() == RelDistribution.Type.HASH_DISTRIBUTED : target.getType();
+ }
+
+ /** {@inheritDoc} */
+ @Override public RelDistribution.Type type() {
+ return RelDistribution.Type.RANDOM_DISTRIBUTED;
+ }
+
+ /** {@inheritDoc} */
+ @Override public <Row> Destination<Row> destination(
+ ExecutionContext<Row> ctx,
+ AffinityService affSrvc,
+ ColocationGroup target,
+ ImmutableIntList keys
+ ) {
+ throw new AssertionError("Correlated distribution should be converted to delegate before using");
+ }
+
+ /** */
+ public CorrelationId correlationId() {
+ return corrId;
+ }
+
+ /** */
+ public IgniteDistribution target() {
+ return target;
+ }
+
+ /** {@inheritDoc} */
+ @Override public boolean correlated() {
+ return true;
+ }
+
+ /** {@inheritDoc} */
+ @Override protected String name0() {
+ return "correlated[corrId=" + corrId + ", target=" + target + ']';
Review Comment:
To be honest, I didn’t think of a better name, so let’s proceed with the current one
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] asfgit closed pull request #10424: IGNITE-18341 SQL Calcite: Introduce correlated distribution
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #10424: IGNITE-18341 SQL Calcite: Introduce correlated distribution
URL: https://github.com/apache/ignite/pull/10424
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] korlov42 commented on a diff in pull request #10424: IGNITE-18341 SQL Calcite: Introduce correlated distribution
Posted by GitBox <gi...@apache.org>.
korlov42 commented on code in PR #10424:
URL: https://github.com/apache/ignite/pull/10424#discussion_r1043571089
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/trait/IgniteDistributions.java:
##########
@@ -113,6 +114,15 @@ public static IgniteDistribution hash(List<Integer> keys, DistributionFunction f
return canonize(new DistributionTrait(ImmutableIntList.copyOf(keys), function));
}
+ /**
+ * @param corrId Target distribution correlation id.
+ * @param target Target distribution.
+ * @return Distribution by correlate.
+ */
+ public static IgniteDistribution correlated(CorrelationId corrId, IgniteDistribution target) {
Review Comment:
Could you please add more details to the javadoc? What does correlated distribution mean? When should and should not it be used?
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/trait/DistributionFunction.java:
##########
@@ -316,4 +327,59 @@ public AffinityDistribution(int cacheId, Object identity) {
return "affinity[identity=" + identity + ", cacheId=" + cacheId + ']';
}
}
+
+ /**
+ * Correlated distribution, used to bypass set of nodes on the right hand of CNLJ and to be restored to
+ * original hash distribution (with remapped keys) by the filter node.
+ */
+ public static final class CorrelatedDistribution extends DistributionFunction {
+ /** */
+ private final CorrelationId corrId;
+
+ /** */
+ private final IgniteDistribution target;
+
+ /** */
+ private CorrelatedDistribution(CorrelationId corrId, IgniteDistribution target) {
+ this.corrId = corrId;
+ this.target = target;
+
+ assert target.getType() == RelDistribution.Type.HASH_DISTRIBUTED : target.getType();
+ }
+
+ /** {@inheritDoc} */
+ @Override public RelDistribution.Type type() {
+ return RelDistribution.Type.RANDOM_DISTRIBUTED;
+ }
+
+ /** {@inheritDoc} */
+ @Override public <Row> Destination<Row> destination(
+ ExecutionContext<Row> ctx,
+ AffinityService affSrvc,
+ ColocationGroup target,
+ ImmutableIntList keys
+ ) {
+ throw new AssertionError("Correlated distribution should be converted to delegate before using");
+ }
+
+ /** */
+ public CorrelationId correlationId() {
+ return corrId;
+ }
+
+ /** */
+ public IgniteDistribution target() {
+ return target;
+ }
+
+ /** {@inheritDoc} */
+ @Override public boolean correlated() {
+ return true;
+ }
+
+ /** {@inheritDoc} */
+ @Override protected String name0() {
+ return "correlated[corrId=" + corrId + ", target=" + target + ']';
Review Comment:
does it make sense to choose another name? Right now it clashes with CorrelationTrait
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org