Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TINKERPOP-3080] AggregateGlobalStep accepts all pre-defined Operators #2616

Open
wants to merge 1 commit into
base: 3.7-dev
Choose a base branch
from

Conversation

rdtr
Copy link
Contributor

@rdtr rdtr commented May 22, 2024

Support all Operators in AggregateGlobalStep and AggregateLocalStep.
https://issues.apache.org/jira/browse/TINKERPOP-3080

@codecov-commenter
Copy link

codecov-commenter commented May 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.50%. Comparing base (9b46b67) to head (b4a3580).
Report is 95 commits behind head on 3.7-dev.

Additional details and impacted files
@@              Coverage Diff              @@
##             3.7-dev    #2616      +/-   ##
=============================================
+ Coverage      76.14%   80.50%   +4.35%     
=============================================
  Files           1084       27    -1057     
  Lines          65160     5000   -60160     
  Branches        7285        0    -7285     
=============================================
- Hits           49616     4025   -45591     
+ Misses         12839      773   -12066     
+ Partials        2705      202    -2503     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rdtr rdtr force-pushed the aggregate_v2 branch 3 times, most recently from 1fab0ae to 050f77b Compare May 24, 2024 20:20
@rdtr rdtr changed the title AggregateGlobalStep acceps all pre-defined Operators [TINKERPOP-3080] AggregateGlobalStep acceps all pre-defined Operators May 24, 2024
@rdtr rdtr changed the title [TINKERPOP-3080] AggregateGlobalStep acceps all pre-defined Operators [TINKERPOP-3080] AggregateGlobalStep accepts all pre-defined Operators May 24, 2024

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: suggest renaming this to bulkReducingOperator or something similar to callout observation towards the nature of reducing operators instead of limiting to BulkSet. addAll works with either maps or any collection and assign works with any type of input.

Copy link

@saikiranboga saikiranboga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Norio for the change. Looks good overall. Few nits.

Comment on lines +126 to +127
// Pre-defined Operator such as addAll and assign will reduce over the whole input set, rather than
// applying a single input one by one.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +56 to +61
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;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same suggestions here as changes from AggregateGlobalStep.

Comment on lines +81 to +87

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);
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious, what was the issue here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants