fix JoinStringFactorVisitor. closes #688

tidyverse · Oct 13, 2014 · 53a6953 · 53a6953
1 parent 7b8bb6a
commit 53a6953
Show file tree

Hide file tree

Showing 3 changed files with 22 additions and 5 deletions.
diff --git a/NEWS.md b/NEWS.md
@@ -19,6 +19,8 @@
   Then the second argument must be either missing, with no name or be called `n`. 
   All other forms of the call are handled by R evaluation. (#683).
 
+* Fixed segfault in `JoinStringFactorVisitor` class. (#688)  
+
 # dplyr 0.3.0.1
 
 * Fixed problem with test script on Windows.

diff --git a/inst/include/dplyr/JoinVisitorImpl.h b/inst/include/dplyr/JoinVisitorImpl.h
@@ -233,13 +233,13 @@ namespace dplyr{
     class JoinStringFactorVisitor : public JoinVisitor {
     public:
         JoinStringFactorVisitor( const CharacterVector& left_, const IntegerVector& right_ ) : 
+            left(left_), 
+            right(right_),
             right_ptr(right_.begin()), 
             right_factor_ptr(Rcpp::internal::r_vector_start<STRSXP>(right_.attr("levels")) ), 
             left_ptr(Rcpp::internal::r_vector_start<STRSXP>(left_))
         {
             check_all_same_encoding(left_,right_.attr("levels")) ;
-            // check_all_utf8(left_) ;
-            // check_all_utf8(right_.attr("levels")) ;
         }
 
         inline size_t hash(int i){ 
@@ -275,22 +275,27 @@ namespace dplyr{
         }
 
     private:
+        CharacterVector left ;
+        IntegerVector right ;
         int*  right_ptr ;
         SEXP* right_factor_ptr ;
         SEXP* left_ptr ;
         boost::hash<SEXP> string_hash ;
 
         inline SEXP get(int i){
             SEXP res ;
+
             if( i>=0 ){
                 res = left_ptr[i] ;
             } else {
                 int index = -i-1 ;
-                if( right_ptr[index] == NA_INTEGER ) res = NA_STRING ;
-                res = right_factor_ptr[ right_ptr[index] - 1 ] ;
+                if( right_ptr[index] == NA_INTEGER ) {
+                    res = NA_STRING ;
+                } else {
+                    res = right_factor_ptr[ right_ptr[index] - 1 ] ;
+                }
             }
 
-
             return res ;
         }
 

diff --git a/tests/testthat/test-joins.r b/tests/testthat/test-joins.r
@@ -280,3 +280,13 @@ test_that("outer_join #96",{
   expect_equal( res$z[3:5], 3:5 )
 
 })
+
+test_that("JoinStringFactorVisitor handles NA #688", {
+  x <- data.frame(Greek = c("Alpha", "Beta", NA))
+  y <- data.frame(Greek = c("Alpha", "Beta", "Gamma"),
+                        Letters = c("C", "B", "C"), stringsAsFactors = F)
+
+  res <- left_join(x, y, by = "Greek")
+  expect_true( is.na(res$Greek[3]) )
+  expect_true( is.na(res$Letters[3]) )
+})