You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by "saikiranboga (via GitHub)" <gi...@apache.org> on 2024/09/03 17:01:33 UTC
[PR] Saikirab fix bytecode [tinkerpop]
saikiranboga opened a new pull request, #2757:
URL: https://github.com/apache/tinkerpop/pull/2757
JIRA: https://issues.apache.org/jira/browse/TINKERPOP-3110
<!--
Thanks for contributing! Reminders:
+ TARGET the earliest branch where you want the change
3.6-dev -> 3.6.8 (bugs only)
3.7-dev -> 3.7.3 (non-breaking)
master -> 4.0.0
+ Committers will MERGE the PR forward to newer versions
+ ADD entry to the CHANGELOG.asciidoc for the targeted version
Do not reference a JIRA number there
+ ADD JIRA number to title and link in description
+ PRs requires 3 +1s from committers OR
1 +1 and 7 day wait to merge.
+ MORE details: https://s.apache.org/rtnal
-->
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TINKERPOP-3110] Fix Bytecode build logic for strategies [tinkerpop]
Posted by "saikiranboga (via GitHub)" <gi...@apache.org>.
saikiranboga commented on code in PR #2757:
URL: https://github.com/apache/tinkerpop/pull/2757#discussion_r1743706799
##########
CHANGELOG.asciidoc:
##########
@@ -38,6 +38,7 @@ This release also includes changes from <<release-3-6-8, 3.6.8>>.
* Added getter method to `ConcatStep`, `ConjoinStep`, `SplitGlobalStep` and `SplitLocalStep` for their private fields.
* Gremlin Server docker containers shutdown gracefully when receiving a SIGTERM.
* TINKERPOP-3080 Support to specify Operator as a reducer in withSideEffect when parsing with the grammar
+* TINKERPOP-3110 Fixed bug in Bytecode build logic for strategies
Review Comment:
Removed JIRA ref. and expanded on the bug.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TINKERPOP-3110] Fix Bytecode build logic for strategies [tinkerpop]
Posted by "saikiranboga (via GitHub)" <gi...@apache.org>.
saikiranboga commented on PR #2757:
URL: https://github.com/apache/tinkerpop/pull/2757#issuecomment-2328895655
> i can't help wondering if you need additional tests. is there an existing test that ensures that we don't have duplicate strategies in bytecode anymore?
@spmallette, Added few more tests. We do have some tests around strategy validation, but not for duplicate checking.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TINKERPOP-3110] Fix Bytecode build logic for strategies [tinkerpop]
Posted by "xiazcy (via GitHub)" <gi...@apache.org>.
xiazcy commented on PR #2757:
URL: https://github.com/apache/tinkerpop/pull/2757#issuecomment-2330605059
Thanks @saikiranboga for the changes! Note you may need to resolve some CHANGELOG conflicts.
VOTE +1
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TINKERPOP-3110] Fix Bytecode build logic for strategies [tinkerpop]
Posted by "kenhuuu (via GitHub)" <gi...@apache.org>.
kenhuuu commented on PR #2757:
URL: https://github.com/apache/tinkerpop/pull/2757#issuecomment-2329713229
Is there a test for the problem stated in TINKERPOP-3110? A test that asserts that if multiple with() are used that it is combined into a single OptionsStrategy that contains all the options from all the with()s?
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TINKERPOP-3110] Fix Bytecode build logic for strategies [tinkerpop]
Posted by "kenhuuu (via GitHub)" <gi...@apache.org>.
kenhuuu commented on code in PR #2757:
URL: https://github.com/apache/tinkerpop/pull/2757#discussion_r1744235369
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/BytecodeHelper.java:
##########
@@ -397,6 +418,8 @@ else if (arguments[i] instanceof Bytecode)
}
}
+
Review Comment:
very small nit: remove added spaces
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TINKERPOP-3110] Fix Bytecode build logic for strategies [tinkerpop]
Posted by "kenhuuu (via GitHub)" <gi...@apache.org>.
kenhuuu commented on code in PR #2757:
URL: https://github.com/apache/tinkerpop/pull/2757#discussion_r1744237332
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/BytecodeHelper.java:
##########
@@ -350,6 +351,26 @@ public static <A extends TraversalStrategy> Iterator<A> findStrategies(final Byt
os -> (A) os.getArguments()[0]);
}
+ public static <A extends TraversalStrategy> boolean removeStrategies(final Bytecode bytecode, final String operator, final Class<TraversalStrategy>[] clazzes) {
Review Comment:
Is `<A extends TraversalStrategy>` needed? `A` doesn't seem to be used.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TINKERPOP-3110] Fix Bytecode build logic for strategies [tinkerpop]
Posted by "saikiranboga (via GitHub)" <gi...@apache.org>.
saikiranboga commented on code in PR #2757:
URL: https://github.com/apache/tinkerpop/pull/2757#discussion_r1744843089
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/BytecodeHelper.java:
##########
@@ -350,6 +351,26 @@ public static <A extends TraversalStrategy> Iterator<A> findStrategies(final Byt
os -> (A) os.getArguments()[0]);
}
+ public static <A extends TraversalStrategy> boolean removeStrategies(final Bytecode bytecode, final String operator, final Class<TraversalStrategy>[] clazzes) {
Review Comment:
Thanks for noticing. Removed.
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/BytecodeHelper.java:
##########
@@ -397,6 +418,8 @@ else if (arguments[i] instanceof Bytecode)
}
}
+
Review Comment:
Removed.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TINKERPOP-3110] Fix Bytecode build logic for strategies [tinkerpop]
Posted by "xiazcy (via GitHub)" <gi...@apache.org>.
xiazcy merged PR #2757:
URL: https://github.com/apache/tinkerpop/pull/2757
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TINKERPOP-3110] Fix Bytecode build logic for strategies [tinkerpop]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #2757:
URL: https://github.com/apache/tinkerpop/pull/2757#issuecomment-2327062224
## [Codecov](https://app.codecov.io/gh/apache/tinkerpop/pull/2757?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `86.36364%` with `3 lines` in your changes missing coverage. Please review.
> Project coverage is 76.41%. Comparing base [(`9b46b67`)](https://app.codecov.io/gh/apache/tinkerpop/commit/9b46b6777d2fa250e41daacf2fa4554605aff53a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`fc6e3ee`)](https://app.codecov.io/gh/apache/tinkerpop/commit/fc6e3ee30cad3549b49ad9d020db49942da22f95?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 201 commits behind head on 3.7-dev.
| [Files with missing lines](https://app.codecov.io/gh/apache/tinkerpop/pull/2757?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [.../tinkerpop/gremlin/process/traversal/Bytecode.java](https://app.codecov.io/gh/apache/tinkerpop/pull/2757?src=pr&el=tree&filepath=gremlin-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Ftinkerpop%2Fgremlin%2Fprocess%2Ftraversal%2FBytecode.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Z3JlbWxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS90aW5rZXJwb3AvZ3JlbWxpbi9wcm9jZXNzL3RyYXZlcnNhbC9CeXRlY29kZS5qYXZh) | 83.33% | [1 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/tinkerpop/pull/2757?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...gremlin/process/traversal/util/BytecodeHelper.java](https://app.codecov.io/gh/apache/tinkerpop/pull/2757?src=pr&el=tree&filepath=gremlin-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Ftinkerpop%2Fgremlin%2Fprocess%2Ftraversal%2Futil%2FBytecodeHelper.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Z3JlbWxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS90aW5rZXJwb3AvZ3JlbWxpbi9wcm9jZXNzL3RyYXZlcnNhbC91dGlsL0J5dGVjb2RlSGVscGVyLmphdmE=) | 90.00% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/tinkerpop/pull/2757?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## 3.7-dev #2757 +/- ##
=============================================
+ Coverage 76.14% 76.41% +0.27%
- Complexity 13152 13203 +51
=============================================
Files 1084 1060 -24
Lines 65160 61439 -3721
Branches 7285 7315 +30
=============================================
- Hits 49616 46950 -2666
+ Misses 12839 11973 -866
+ Partials 2705 2516 -189
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/tinkerpop/pull/2757?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TINKERPOP-3110] Fix Bytecode build logic for strategies [tinkerpop]
Posted by "kenhuuu (via GitHub)" <gi...@apache.org>.
kenhuuu commented on PR #2757:
URL: https://github.com/apache/tinkerpop/pull/2757#issuecomment-2332194615
VOTE +1
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TINKERPOP-3110] Fix Bytecode build logic for strategies [tinkerpop]
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on PR #2757:
URL: https://github.com/apache/tinkerpop/pull/2757#issuecomment-2329086494
thanks for the fixes - VOTE +1
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TINKERPOP-3110] Fix Bytecode build logic for strategies [tinkerpop]
Posted by "saikiranboga (via GitHub)" <gi...@apache.org>.
saikiranboga commented on PR #2757:
URL: https://github.com/apache/tinkerpop/pull/2757#issuecomment-2330654145
> Is there a test for the problem stated in TINKERPOP-3110? A test that asserts that if multiple with() are used that it is combined into a single OptionsStrategy that contains all the options from all the with()s?
Yes. See `BytecodeHelperTest#shouldFindStrategy`.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TINKERPOP-3110] Fix Bytecode build logic for strategies [tinkerpop]
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #2757:
URL: https://github.com/apache/tinkerpop/pull/2757#discussion_r1743553054
##########
CHANGELOG.asciidoc:
##########
@@ -38,6 +38,7 @@ This release also includes changes from <<release-3-6-8, 3.6.8>>.
* Added getter method to `ConcatStep`, `ConjoinStep`, `SplitGlobalStep` and `SplitLocalStep` for their private fields.
* Gremlin Server docker containers shutdown gracefully when receiving a SIGTERM.
* TINKERPOP-3080 Support to specify Operator as a reducer in withSideEffect when parsing with the grammar
+* TINKERPOP-3110 Fixed bug in Bytecode build logic for strategies
Review Comment:
the JIRA reference should not be in the CHANGELOG (i know its in the entry above, but that's not correct either). JIRA references are added on release in a separate listing. also, could this be a bit more specific by including a few words about what the "bug" was?
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TINKERPOP-3110] Fix Bytecode build logic for strategies [tinkerpop]
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #2757:
URL: https://github.com/apache/tinkerpop/pull/2757#discussion_r1743553054
##########
CHANGELOG.asciidoc:
##########
@@ -38,6 +38,7 @@ This release also includes changes from <<release-3-6-8, 3.6.8>>.
* Added getter method to `ConcatStep`, `ConjoinStep`, `SplitGlobalStep` and `SplitLocalStep` for their private fields.
* Gremlin Server docker containers shutdown gracefully when receiving a SIGTERM.
* TINKERPOP-3080 Support to specify Operator as a reducer in withSideEffect when parsing with the grammar
+* TINKERPOP-3110 Fixed bug in Bytecode build logic for strategies
Review Comment:
the JIRA reference should not be in the CHANGELOG (i know its in the entry above, but that's not correct either). JIRA references are added on release in a separate listing.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TINKERPOP-3110] Fix Bytecode build logic for strategies [tinkerpop]
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on PR #2757:
URL: https://github.com/apache/tinkerpop/pull/2757#issuecomment-2328610840
i can't help wondering if you need additional tests. is there an existing test that ensures that we don't have duplicate strategies in bytecode anymore?
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org