Merge lp:~mmcm/akiban-server/parse-boolean into lp:~akiban-technologies/akiban-server/trunk

[Mike McMahon]

Although the recent change to expose AkBool included extensive new tests, they missed the simple case of boolean string literal, such as 'true', which was ending up as false.

Additionally, ActiveRecord sends 't' and 'f' (see lib/active_record/connection_adapters/abstract/quoting.rb and note that even redefining quoted_true and quoted_false isn't enough because they're also in type_cast), so we need those, too.

[Thomas Jones-Low (tjoneslo)]

True and False for the server.

review: Approve

[Nathan Williams (nwilliams)]

Note that they weren't missed. It was left as-is for MySQL compat.

Perhaps that isn't important though.

[Akiban Build User (build-akiban)]

There were 2 failures during build/test:

* job server-build failed at build number 3928: http://172.16.20.104:8080/job/server-build/3928/

* view must-pass failed: server-build is yellow

review: Needs Fixing

[Mike McMahon (mmcm)]

A column declared BOOLEAN via the adapter will still be TINYINT, won't it? So conversions involving this type won't apply. Or did I misunderstand the earlier change?

[lp:~mmcm/akiban-server/parse-boolean]

2602. By Mike McMahon on 2013-03-23

Make the test more obvious.

[Mike McMahon (mmcm)]

Like Nathan said, there was an explicit test for one of the now incompatible cases.

So, clean up to make it much more explicit for the future what we are proposing to do.

review: Needs Resubmitting

[Nathan Williams (nwilliams)]

> A column declared BOOLEAN via the adapter will still be
> TINYINT, won't it? So conversions involving this type
> won't apply.

Conversion from string to BOOLEAN, such as WHERE <varchar>. Silly edge case, of course.

The rest of the compatible parse (e.g. 1.asdf = true) is funny, but no need to redo everything right now.

review: Approve

=== modified file 'src/main/java/com/akiban/server/types3/TParsers.java'
--- src/main/java/com/akiban/server/types3/TParsers.java	2013-03-22 20:05:57 +0000
+++ src/main/java/com/akiban/server/types3/TParsers.java	2013-03-23 13:38:21 +0000
@@ -33,31 +33,39 @@
         @Override
         public void parse(TExecutionContext context, PValueSource source, PValueTarget target)
         {
-            // parse source is a string representing a number-ish, where '0' is false, any other integer is true.
-            // We're looking for an optional negative, followed by an optional dot, followed by any number of digits,
-            // followed by anything. If any of those digits is not 0, the result is true; otherwise it's false.
             String s = source.getString();
-            boolean negativeAllowed = true;
-            boolean periodAllowed = true;
             boolean result = false;
-            for (int i = 0, len = s.length(); i < len; ++i) {
-                char c = s.charAt(i);
-                if (negativeAllowed && c == '-') {
-                    negativeAllowed = false;
-                }
-                else if (periodAllowed && c == '.') {
-                    periodAllowed = false;
-                    negativeAllowed = false;
-                }
-                else if (Character.isDigit(c)) {
-                    if (c != '0') {
-                        result = true;
+            if (s.equalsIgnoreCase("true") || s.equalsIgnoreCase("t")) {
+                result = true;
+            }
+            else if (s.equalsIgnoreCase("false") || s.equalsIgnoreCase("f")) {
+                result = false;
+            }
+            else {
+                // parse source is a string representing a number-ish, where '0' is false, any other integer is true.
+                // We're looking for an optional negative, followed by an optional dot, followed by any number of digits,
+                // followed by anything. If any of those digits is not 0, the result is true; otherwise it's false.
+                boolean negativeAllowed = true;
+                boolean periodAllowed = true;
+                for (int i = 0, len = s.length(); i < len; ++i) {
+                    char c = s.charAt(i);
+                    if (negativeAllowed && c == '-') {
+                        negativeAllowed = false;
+                    }
+                    else if (periodAllowed && c == '.') {
+                        periodAllowed = false;
+                        negativeAllowed = false;
+                    }
+                    else if (Character.isDigit(c)) {
+                        if (c != '0') {
+                            result = true;
+                            break;
+                        }
+                    }
+                    else {
                         break;
                     }
                 }
-                else {
-                    break;
-                }
             }
             target.putBool(result);
         }

=== modified file 'src/test/java/com/akiban/server/types3/BooleanParserTest.java'
--- src/test/java/com/akiban/server/types3/BooleanParserTest.java	2013-03-22 20:05:57 +0000
+++ src/test/java/com/akiban/server/types3/BooleanParserTest.java	2013-03-23 13:38:21 +0000
@@ -55,7 +55,12 @@
         param(builder, "a1", false); // MySQL doesn't believe in steak sauce
         param(builder, "0", false);
         param(builder, "0.0", false);
-        param(builder, "true", false);
+
+        param(builder, "false", false);
+        param(builder, "f", false);
+        // Following are not MySQL compatible, but required for ActiveRecord.
+        param(builder, "true", true);
+        param(builder, "t", true);
 
         return builder.asList();
     }