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

in ms excel, 100>"23" , "100">"23" will be converted to Number to compare, hope poi do it the same #5

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from

Conversation

helloworld2688
Copy link

No description provided.

@Gagravarr
Copy link
Contributor

Are you able to do a version of the patch without all the whitespace changes? Currently it's very hard with them to work out what logic was changed!

@helloworld2688
Copy link
Author

i can see at https://github.com/apache/poi/pull/5/files +11 -9, it is clear. or what can i do?

@Gagravarr
Copy link
Contributor

I've tried something very similar to your patch, mostly just documentation/comment tweaks:

Index: src/java/org/apache/poi/ss/formula/eval/RelationalOperationEval.java
===================================================================
--- src/java/org/apache/poi/ss/formula/eval/RelationalOperationEval.java    (revision 1517631)
+++ src/java/org/apache/poi/ss/formula/eval/RelationalOperationEval.java    (working copy)
@@ -45,7 +45,8 @@
     * Bool.TRUE > Bool.FALSE
     * Bool.FALSE == Blank
     *
-    * Strings are never converted to numbers or booleans
+    * Strings are never converted to booleans
+    * Strings are converted to numbers for simple cases
     * String > any number. ALWAYS
     * Non-empty String > Blank
     * Empty String == Blank
@@ -94,6 +95,16 @@
        if (vb instanceof BoolEval) {
            return -1;
        }
+       
+       // Try both as numbers
+       try {
+           Double nA = OperandResolver.coerceValueToDouble(va); 
+           Double nB = OperandResolver.coerceValueToDouble(vb); 
+           return NumberComparer.compare(nA, nB);
+       } catch (Exception e) {
+           // At least one isn't a number / string of a number
+       }
+       
        if (va instanceof StringEval) {
            if (vb instanceof StringEval) {
                StringEval sA = (StringEval) va;
@@ -105,13 +116,7 @@
        if (vb instanceof StringEval) {
            return -1;
        }
-       if (va instanceof NumberEval) {
-           if (vb instanceof NumberEval) {
-               NumberEval nA = (NumberEval) va;
-               NumberEval nB = (NumberEval) vb;
-               return NumberComparer.compare(nA.getNumberValue(), nB.getNumberValue());
-           }
-       }
+
        throw new IllegalArgumentException("Bad operand types (" + va.getClass().getName() + "), ("
                + vb.getClass().getName() + ")");
    }

The problem is that at least one unit test - org.apache.poi.ss.formula.eval.TestFormulasFromSpreadsheet - then fails. Are you able to check why your suggested fix is breaking this test?

@helloworld2688
Copy link
Author

ur, in my enviroment , it's ok, i can not find anythin wrong, maybe some log or stacktrace can help u end it

@asfgit
Copy link

asfgit commented Sep 28, 2017

Can one of the admins verify this patch?

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

Successfully merging this pull request may close these issues.

3 participants