Fixed a bug in Quoter->split_unquote, added several tests for find_renamed_cols

bin/pt-online-schema-change

@@ -2335,12 +2335,18 @@ sub quote_val {
 
 sub split_unquote {
    my ( $self, $db_tbl, $default_db ) = @_;
-   $db_tbl =~ s/`//g;
    my ( $db, $tbl ) = split(/[.]/, $db_tbl);
    if ( !$tbl ) {
       $tbl = $db;
       $db  = $default_db;
    }
+   for ($db, $tbl) {
+      next unless $_;
+      s/\A`//;
+      s/`\z//;
+      s/``/`/;
+   }
+   
    return ($db, $tbl);
 }
 
@@ -3437,7 +3443,9 @@ sub is_cluster_node {
    PTDEBUG && _d($sql);
    my $row = $self->{dbh}->selectrow_arrayref($sql);
    PTDEBUG && _d(defined $row ? @$row : 'undef');
-   $self->{is_cluster_node} = $row && $row->[0] ? 1 : 0;
+   $self->{is_cluster_node} = $row && $row->[1]
+                            ? ($row->[1] eq 'ON' || $row->[1] eq '1')
+                            : 0;
   
    return $self->{is_cluster_node};
 }

lib/Quoter.pm

@@ -93,12 +93,18 @@ sub quote_val {
 #   <join_quote>
 sub split_unquote {
    my ( $self, $db_tbl, $default_db ) = @_;
-   $db_tbl =~ s/`//g;
    my ( $db, $tbl ) = split(/[.]/, $db_tbl);
    if ( !$tbl ) {
       $tbl = $db;
       $db  = $default_db;
    }
+   for ($db, $tbl) {
+      next unless $_;
+      s/\A`//;
+      s/`\z//;
+      s/``/`/;
+   }
+   
    return ($db, $tbl);
 }
 

t/pt-online-schema-change/find_renamed_cols.t

@@ -23,7 +23,8 @@ sub test_func {
    my ($alter, $renamed_cols) = @_;
    die "No alter arg" unless $alter;
    die "No renamed_cols arg" unless $renamed_cols;
-
+   (my $show_alter = $alter) =~ s/\n/\\n/g;
+   
    my %got_renamed_cols = eval {
       pt_online_schema_change::find_renamed_cols($alter, $tp);
    };
@@ -31,14 +32,14 @@ sub test_func {
       is_deeply(
          undef,
          $renamed_cols,
-         $alter,
+         $show_alter,
       ) or diag($EVAL_ERROR);
    }
    else {
       is_deeply(
          \%got_renamed_cols,
          $renamed_cols,
-         $alter,
+         $show_alter,
       ) or diag(Dumper(\%got_renamed_cols));
    }
 }
@@ -62,6 +63,14 @@ test_func(
    },
 );
 
+# Optional COLUMN?
+test_func(
+   "CHANGE column old_column_name new_column_name VARCHAR(255) NULL",
+   {
+      old_column_name => 'new_column_name',
+   },
+);
+
 # Space-sensitive?
 test_func(
    "CHANGE   a       z     VARCHAR(255)  NULL",
@@ -78,6 +87,15 @@ test_func(
    },
 );
 
+# Column named column?
+test_func(
+   "CHANGE `column` new_column_name VARCHAR(255) NULL",
+   {
+      column => 'new_column_name',
+   },
+);
+
+
 # Extended ascii?
 test_func(
    "CHANGE `café` `tête-à-tête` INT",
@@ -86,6 +104,71 @@ test_func(
    },
 );
 
+# UTF-8?
+if( Test::Builder->VERSION < 2 ) {
+   foreach my $method ( qw(output failure_output) ) {
+      binmode Test::More->builder->$method(), ':encoding(UTF-8)';
+   }
+}
+test_func(
+   "CHANGE `\x{30cb}` `\x{30cd}` INT",
+   {
+      "\x{30cb}" => "\x{30cd}",
+   },
+);
+# Mixed backtick-sensitive?
+test_func(
+   "CHANGE `a` z VARCHAR(255)  NULL",
+   {
+      a => 'z',
+   },
+);
+
+test_func(
+   "CHANGE a `z` VARCHAR(255)  NULL",
+   {
+      a => 'z',
+   },
+);
+
+# Ansi quotes-sensitive? (should matter)
+test_func(
+   qq{CHANGE "a" "z" VARCHAR(255)  NULL},
+   {
+      a => 'z',
+   },
+);
+
+# Embedded backticks?
+test_func(
+   "CHANGE `a``a` z VARCHAR(255)  NULL",
+   {
+      'a`a' => 'z',
+   },
+);
+
+# Emebedded spaces?
+test_func(
+   "CHANGE `a yes  ` z VARCHAR(255)  NULL",
+   {
+      'a yes  ' => 'z',
+   },
+);
+
+test_func(
+   "CHANGE `  yes  ` `\nyes!\na` VARCHAR(255)  NULL",
+   {
+      '  yes  ' => "\nyes!\na",
+   },
+);
+
+test_func(
+   "CHANGE `  yes  ` `\nyes!\na` VARCHAR(255)  NULL",
+   {
+      '  yes  ' => "\nyes!\na",
+   },
+);
+
 # #############################################################################
 # Two column alters
 # #############################################################################
@@ -98,6 +181,16 @@ test_func(
    },
 );
 
+# Pathological
+test_func(
+   "CHANGE a `CHANGE a z VARCHAR(255) NOT NULL` VARCHAR(255)  NULL, CHANGE foo bar INT",
+   {
+      a   => 'CHANGE a z VARCHAR(255) NOT NULL',
+      foo => 'bar',
+   },
+);
+
+
 # #############################################################################
 # Fake alters
 # #############################################################################
@@ -109,6 +202,23 @@ test_func(
    },
 );
 
+# Not really renamed, should ignore case
+test_func(
+   "CHANGE foo FOO FLOAT",
+   {
+   },
+);
+
+TODO: {
+   local $::TODO = "We don't parse the entire alter statement, what looks like a CHANGE COLUMNS";
+   # Not really an alter, pathological
+   test_func(
+      "MODIFY `CHANGE a z VARCHAR(255) NOT NULL` FLOAT",
+      {
+      },
+   );
+}
+
 # #############################################################################
 # Done.
 # #############################################################################