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-2890] Avoid exceptions on local scope based steps where possible #2012

Merged
merged 6 commits into from
Apr 21, 2023

Conversation

vkagamlyk
Copy link
Contributor

Several local steps extended to work with input other than Iterable.
g.inject(1).dedup(local) is now valid query.

Affected steps: count, dedup, max, mean, min, order, range, sample, sum, tail.

@codecov-commenter
Copy link

codecov-commenter commented Apr 5, 2023

Codecov Report

Merging #2012 (68dbbd7) into 3.5-dev (8267422) will decrease coverage by 5.22%.
The diff coverage is n/a.

@@              Coverage Diff              @@
##             3.5-dev    #2012      +/-   ##
=============================================
- Coverage      69.37%   64.16%   -5.22%     
=============================================
  Files            866       25     -841     
  Lines          41183     3750   -37433     
  Branches        5423        0    -5423     
=============================================
- Hits           28570     2406   -26164     
+ Misses         10704     1166    -9538     
+ Partials        1909      178    -1731     

see 841 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@spmallette
Copy link
Contributor

spmallette commented Apr 13, 2023

Could you please add some feature tests for each of the steps that were changed so that this functionality is validated? Probably check to see if there are tests in general for this for all local scope steps even if you didn't make changes for them. Also, needs a changelog.

@@ -54,7 +54,7 @@ protected S map(final Traverser.Admin<S> traverser) {
start instanceof Collection ? ((Collection) start).size() :
start instanceof Path ? ((Path) start).size() :
start instanceof Iterable ? IteratorUtils.count((Iterable) start) :
this.limit;
IteratorUtils.count(IteratorUtils.asIterator(start));
Copy link
Contributor

Choose a reason for hiding this comment

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

is this meant as some sort of safety? RangeLocalStep.applyRange() seems to satisfy the issue, no? no exception thrown here:

gremlin> g.inject([1,2,3],[1],10).tail(local)
==>3
==>1
==>10

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also to handle arrays as input

gremlin> g.inject([1,2,3] as int[], [1]).tail(local,1)
==>[1, 2, 3]
==>1

| josh |

Scenario: g_injectX1X_dedupXlocalX_unfold
Copy link
Contributor

Choose a reason for hiding this comment

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

this was the contrived example from the JIRA, but i think it comes with the downside that you had to add OptOut for them in computer tests. could you come up with tests that don't require that OptOut please?

@vkagamlyk
Copy link
Contributor Author

VOTE+1

@kenhuuu
Copy link
Contributor

kenhuuu commented Apr 21, 2023

VOTE +1

1 similar comment
@xiazcy
Copy link
Contributor

xiazcy commented Apr 21, 2023

VOTE +1

@vkagamlyk vkagamlyk merged commit a02617d into apache:3.5-dev Apr 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants