You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by "lvca (via GitHub)" <gi...@apache.org> on 2023/03/18 14:52:14 UTC

[GitHub] [tinkerpop] lvca opened a new pull request, #1993: perf: improved performance of comparison between not compatible types and nulls

lvca opened a new pull request, #1993:
URL: https://github.com/apache/tinkerpop/pull/1993

   This fix dramatically improves the performance of traversing when different types are encountered or simply null values. This piece of code should be designed to be fast because invoked millions of times with queries. It is a bad practice to throw Java exceptions instead of simply returning false.


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

[GitHub] [tinkerpop] xiazcy commented on pull request #1993: perf: improved performance of comparison (equals) between not compatible types and nulls

Posted by "xiazcy (via GitHub)" <gi...@apache.org>.
xiazcy commented on PR #1993:
URL: https://github.com/apache/tinkerpop/pull/1993#issuecomment-1495085032

   Cherry picked to https://github.com/apache/tinkerpop/commit/44c888dec68d66e56fd2acc8dbf09d7d1eed2aa5 and added changelog entry. 


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

[GitHub] [tinkerpop] lvca commented on a diff in pull request #1993: perf: improved performance of comparison (equals) between not compatible types and nulls

Posted by "lvca (via GitHub)" <gi...@apache.org>.
lvca commented on code in PR #1993:
URL: https://github.com/apache/tinkerpop/pull/1993#discussion_r1145551828


##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/util/GremlinValueComparator.java:
##########
@@ -129,8 +129,18 @@ public boolean equals(final Object f, final Object s) {
             if (containersOfDifferentSize(f, s))
                 return false;
 
+            // For Compare, NaN always produces ERROR
+            if (eitherAreNaN(f, s))
+                return false;
+
+            // For Compare we do not cross type boundaries, including null
+            if (!comparable(f, s))
+                return false;
+
             try {
-                return this.compare(f, s) == 0;
+              // comparable(f, s) assures that type(f) == type(s)

Review Comment:
   If you remove the try/catch block some tests won't pass



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

[GitHub] [tinkerpop] xiazcy commented on pull request #1993: perf: improved performance of comparison (equals) between not compatible types and nulls

Posted by "xiazcy (via GitHub)" <gi...@apache.org>.
xiazcy commented on PR #1993:
URL: https://github.com/apache/tinkerpop/pull/1993#issuecomment-1481587529

   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

[GitHub] [tinkerpop] xiazcy closed pull request #1993: perf: improved performance of comparison (equals) between not compatible types and nulls

Posted by "xiazcy (via GitHub)" <gi...@apache.org>.
xiazcy closed pull request #1993: perf: improved performance of comparison (equals) between not compatible types and nulls
URL: https://github.com/apache/tinkerpop/pull/1993


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

[GitHub] [tinkerpop] mikepersonick commented on pull request #1993: perf: improved performance of comparison (equals) between not compatible types and nulls

Posted by "mikepersonick (via GitHub)" <gi...@apache.org>.
mikepersonick commented on PR #1993:
URL: https://github.com/apache/tinkerpop/pull/1993#issuecomment-1476643796

   This change looks perfectly reasonable to me. +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

[GitHub] [tinkerpop] codecov-commenter commented on pull request #1993: perf: improved performance of comparison between not compatible types and nulls

Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #1993:
URL: https://github.com/apache/tinkerpop/pull/1993#issuecomment-1474874419

   ## [Codecov](https://codecov.io/gh/apache/tinkerpop/pull/1993?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#1993](https://codecov.io/gh/apache/tinkerpop/pull/1993?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (76e92f8) into [master](https://codecov.io/gh/apache/tinkerpop/commit/17b53c2c48a7a7932c41fb8d7fa47cb217e1c659?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (17b53c2) will **decrease** coverage by `4.33%`.
   > The diff coverage is `n/a`.
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #1993      +/-   ##
   ============================================
   - Coverage     68.75%   64.43%   -4.33%     
   ============================================
     Files           855       25     -830     
     Lines         41306     3821   -37485     
     Branches       5604        0    -5604     
   ============================================
   - Hits          28402     2462   -25940     
   + Misses        10914     1187    -9727     
   + Partials       1990      172    -1818     
   ```
   
   
   [see 830 files with indirect coverage changes](https://codecov.io/gh/apache/tinkerpop/pull/1993/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   :mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   


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

[GitHub] [tinkerpop] Cole-Greer commented on pull request #1993: perf: improved performance of comparison (equals) between not compatible types and nulls

Posted by "Cole-Greer (via GitHub)" <gi...@apache.org>.
Cole-Greer commented on PR #1993:
URL: https://github.com/apache/tinkerpop/pull/1993#issuecomment-1480077175

   This change looks good to me. I would agree that there are too many places in Tinkerpop where exceptions are used unnecessarily.
   
   VOTE +1 (Non-Binding)


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

[GitHub] [tinkerpop] xiazcy commented on a diff in pull request #1993: perf: improved performance of comparison (equals) between not compatible types and nulls

Posted by "xiazcy (via GitHub)" <gi...@apache.org>.
xiazcy commented on code in PR #1993:
URL: https://github.com/apache/tinkerpop/pull/1993#discussion_r1145394117


##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/util/GremlinValueComparator.java:
##########
@@ -129,8 +129,18 @@ public boolean equals(final Object f, final Object s) {
             if (containersOfDifferentSize(f, s))
                 return false;
 
+            // For Compare, NaN always produces ERROR
+            if (eitherAreNaN(f, s))
+                return false;
+
+            // For Compare we do not cross type boundaries, including null
+            if (!comparable(f, s))
+                return false;
+
             try {
-                return this.compare(f, s) == 0;
+              // comparable(f, s) assures that type(f) == type(s)

Review Comment:
   LGTM, just one quick question, do we still need this try/catch if we are no longer using the function that throws `GremlinTypeErrorException`? 



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

[GitHub] [tinkerpop] xiazcy commented on a diff in pull request #1993: perf: improved performance of comparison (equals) between not compatible types and nulls

Posted by "xiazcy (via GitHub)" <gi...@apache.org>.
xiazcy commented on code in PR #1993:
URL: https://github.com/apache/tinkerpop/pull/1993#discussion_r1146530591


##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/util/GremlinValueComparator.java:
##########
@@ -129,8 +129,18 @@ public boolean equals(final Object f, final Object s) {
             if (containersOfDifferentSize(f, s))
                 return false;
 
+            // For Compare, NaN always produces ERROR
+            if (eitherAreNaN(f, s))
+                return false;
+
+            // For Compare we do not cross type boundaries, including null
+            if (!comparable(f, s))
+                return false;
+
             try {
-                return this.compare(f, s) == 0;
+              // comparable(f, s) assures that type(f) == type(s)

Review Comment:
   Ah I see, it makes sense if the comparator type resolves to `GremlinValueComparator` and uses the compare function above. 



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

[GitHub] [tinkerpop] kenhuuu commented on pull request #1993: perf: improved performance of comparison (equals) between not compatible types and nulls

Posted by "kenhuuu (via GitHub)" <gi...@apache.org>.
kenhuuu commented on PR #1993:
URL: https://github.com/apache/tinkerpop/pull/1993#issuecomment-1481799089

   VOTE +1 pending CHANGELOG entry and targeting 3.6-dev (instead of master).
   
   I can cherry-pick this commit and add in those minor changes for you if you are too busy to do them.


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

[GitHub] [tinkerpop] Cole-Greer commented on pull request #1993: perf: improved performance of comparison (equals) between not compatible types and nulls

Posted by "Cole-Greer (via GitHub)" <gi...@apache.org>.
Cole-Greer commented on PR #1993:
URL: https://github.com/apache/tinkerpop/pull/1993#issuecomment-1481609167

   Just wanted to chime in really quick before it gets merged and ask if it's worth pushing this into 3.6-dev? Looks like a non breaking change which would help 3.6 users


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