Refactor usage of Column::get_element_isvalid() (#3457)

- rename `get_element_isvalid()` to `get_element_validity()`;
- do not use it to check validity of multiple elements in the same column to slightly improve performance;
- streamline `dt.fillna()` backend.

Author: oleksiyskononenko

src/core/column.cc

@@ -330,7 +330,7 @@ py::oobj Column::get_element_as_pyobject(size_t i) const {
   }
 }
 
-bool Column::get_element_isvalid(size_t i) const {
+bool Column::get_element_validity(size_t i) const {
   dt::SType st = data_stype();
 
   switch (st) {

src/core/column.h

@@ -171,7 +171,14 @@ class Column
     py::oobj get_element_as_pyobject(size_t i) const;
 
     // Return validity of the i-th element.
-    bool get_element_isvalid(size_t i) const;
+    // Note, it is not efficient to use this method to check validity
+    // of multiple elements within the same column. That's because everytime
+    // `get_element_validity()` is called, it has to determine
+    // the column's stype to call an appropriate `get_element()` implementation.
+    // If you do need validity of multiple elements, determine an appropriate
+    // `get_element()` implementation on your own, and then call it
+    // as many times as necessary.
+    bool get_element_validity(size_t i) const;
 
 
   //------------------------------------

src/core/column/qcut.h

@@ -88,7 +88,7 @@ class Qcut_ColumnImpl : public Virtual_ColumnImpl {
 
       // If there is one group only, fill it with constants or NA's.
       if (is_const_ || gb.size() == 1) {
-        if (col_.get_element_isvalid(0)) {
+        if (col_.get_element_validity(0)) {
           col_tmp = Column(new ConstInt_ColumnImpl(
                       col_.nrows(),
                       (nquantiles_ - 1) / 2,
@@ -113,7 +113,7 @@ class Qcut_ColumnImpl : public Virtual_ColumnImpl {
         size_t row;
         bool row_valid = ri.get_element(0, &row);
         xassert(row_valid); (void) row_valid;
-        has_na_group = !col_.get_element_isvalid(row);
+        has_na_group = !col_.get_element_validity(row);
       }
 
       // Set up number of valid groups and the quantile coefficients.

src/core/expr/fexpr_fillna.cc

@@ -64,6 +64,25 @@ class FExpr_FillNA : public FExpr_Func {
 
 
     template <bool REVERSE>
+    static RowIndex fill_rowindex(Column& col, const Groupby& gby) {
+      switch (col.stype()) {
+        case SType::BOOL:
+        case SType::INT8:    return fill_rowindex<int8_t, REVERSE>(col, gby);
+        case SType::INT16:   return fill_rowindex<int16_t, REVERSE>(col, gby);
+        case SType::DATE32:
+        case SType::INT32:   return fill_rowindex<int32_t, REVERSE>(col, gby);
+        case SType::TIME64:
+        case SType::INT64:   return fill_rowindex<int64_t, REVERSE>(col, gby);
+        case SType::FLOAT32: return fill_rowindex<float, REVERSE>(col, gby);
+        case SType::FLOAT64: return fill_rowindex<double, REVERSE>(col, gby);
+        case SType::STR32:
+        case SType::STR64:   return fill_rowindex<CString, REVERSE>(col, gby);
+        default: throw RuntimeError();
+      }
+    }
+
+
+    template <typename T, bool REVERSE>
     static RowIndex fill_rowindex(Column& col, const Groupby& gby) {
       Buffer buf = Buffer::mem(col.nrows() * sizeof(int32_t));
       auto indices = static_cast<int32_t*>(buf.xptr());
@@ -75,16 +94,18 @@ class FExpr_FillNA : public FExpr_Func {
           size_t i1, i2;
           gby.get_group(gi, &i1, &i2);
           size_t fill_id = REVERSE? i2 - 1 : i1;
+          T value;
+          bool is_valid;
 
           if (REVERSE) {
             for (size_t i = i2; i-- > i1;) {
-              size_t is_valid = col.get_element_isvalid(i);
+              is_valid = col.get_element(i, &value);
               fill_id = is_valid? i : fill_id;
               indices[i] = static_cast<int32_t>(fill_id);
             }
           } else {
             for (size_t i = i1; i < i2; ++i) {
-              size_t is_valid = col.get_element_isvalid(i);
+              is_valid = col.get_element(i, &value);
               fill_id = is_valid? i : fill_id;
               indices[i] = static_cast<int32_t>(fill_id);
             }
@@ -136,18 +157,14 @@ class FExpr_FillNA : public FExpr_Func {
       } else {
         // Fill with the previous/subsequent non-missing values
         Groupby gby = ctx.get_groupby();
-        if (!gby) {
-          gby = Groupby::single_group(wf.nrows());
-        } else {
-          wf.increase_grouping_mode(Grouping::GtoALL);
-        }
+        wf.increase_grouping_mode(Grouping::GtoALL);
 
         for (size_t i = 0; i < wf.ncols(); ++i) {
           bool is_grouped = ctx.has_group_column(
                               wf.get_frame_id(i),
                               wf.get_column_id(i)
                             );
-          if (is_grouped) continue;
+          if (is_grouped || wf.get_column(i).stype() == SType::VOID) continue;
 
           Column coli = wf.retrieve_column(i);
           auto stats = coli.get_stats_if_exist();
@@ -160,7 +177,6 @@ class FExpr_FillNA : public FExpr_Func {
                                     : fill_rowindex<false>(coli, gby);
               coli.apply_rowindex(ri);
           }
-
           wf.replace_column(i, std::move(coli));
         }
       }

tests/test-dt.py

@@ -1081,4 +1081,4 @@ def test_issue2873():
     assert t10000 < 1.0
     # The timer can have low resolution and produce `t1000 == 0`
     if t1000 > 0:
-        assert t10000 / t1000 < 50
+        assert t10000 / t1000 < 100