[
https://issues.apache.org/jira/browse/TINKERPOP-3080?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17849632#comment-17849632
]
ASF GitHub Bot commented on TINKERPOP-3080:
-------------------------------------------
saikiranboga commented on code in PR #2616:
URL: https://github.com/apache/tinkerpop/pull/2616#discussion_r1615528311
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AggregateLocalStep.java:
##########
@@ -52,9 +53,20 @@ public AggregateLocalStep(final Traversal.Admin traversal,
final String sideEffe
@Override
protected void sideEffect(final Traverser.Admin<S> traverser) {
+ final TraversalSideEffects sideEffects =
this.getTraversal().getSideEffects();
+ // Pre-defined Operator such as addAll and assign will reduce over the
whole input set, rather than
+ // applying a single input one by one.
+ final boolean isOperatorForBulkSet =
sideEffects.getReducer(sideEffectKey) == Operator.addAll ||
+ sideEffects.getReducer(sideEffectKey) == Operator.assign;
+
Review Comment:
Same suggestions here as changes from AggregateGlobalStep.
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/verification/ComputerVerificationStrategy.java:
##########
@@ -80,6 +78,13 @@ public void apply(final Traversal.Admin<?, ?> traversal) {
if (UNSUPPORTED_STEPS.stream().filter(c ->
c.isAssignableFrom(step.getClass())).findFirst().isPresent())
throw new VerificationException("The following step is
currently not supported on GraphComputer: " + step, traversal);
+
+ if (step instanceof SideEffectCapable) {
+ final BinaryOperator<?> sideEffectOperator =
traversal.getSideEffects().getReducer(((SideEffectCapable<?, ?>)
step).getSideEffectKey());
+ if (sideEffectOperator instanceof Operator && (!((Operator)
sideEffectOperator).isCommutative())) {
+ throw new VerificationException("The following step has an
SideEffect operator " + sideEffectOperator + " which is currently not supported
on GraphComputer: " + step, traversal);
+ }
+ }
Review Comment:
Curious, what was the issue here?
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AggregateGlobalStep.java:
##########
@@ -122,8 +121,16 @@ protected Traverser.Admin<S> processNextStart() {
@Override
public void processAllStarts() {
+ final TraversalSideEffects sideEffects =
this.getTraversal().getSideEffects();
+
+ // Pre-defined Operator such as addAll and assign will reduce over the
whole input set, rather than
+ // applying a single input one by one.
Review Comment:
Suggest to also add to comment here on how both these operators work to be
clear on why we focus on reduction over the whole input set. Basically,
`addAll` can only work when either inputs to it are Maps or instances of
Collection interface. And `assign` works for any type of input.
> AggregateStep can support all Operators predefined in TinkerPop
> ---------------------------------------------------------------
>
> Key: TINKERPOP-3080
> URL: https://issues.apache.org/jira/browse/TINKERPOP-3080
> Project: TinkerPop
> Issue Type: Improvement
> Components: language, process
> Reporter: Norio Akagi
> Priority: Major
> Fix For: 3.7.3
>
>
> Currently, {{AggreteGlobalStep}} and {{AggreteLocalStep}} only support addAll
> and assign as Operator. This is because they use BulkSet to apply to
> Operator. Only addAll and assign can work with BulkSet, so for other
> operators it results in a type casting failure.
> They can be more flexible to work with any operators depending on what is set
> by {{{}withSideEffect(){}}}.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
