Github user felixcheung commented on a diff in the pull request:

    https://github.com/apache/spark/pull/18366#discussion_r123891161
  
    --- Diff: R/pkg/R/functions.R ---
    @@ -86,6 +86,22 @@ NULL
     #' df <- createDataFrame(data.frame(time = as.POSIXct(dts), y = y))}
     NULL
     
    +#' String functions for Column operations
    +#'
    +#' String functions defined for \code{Column}.
    +#'
    +#' @param x Column to compute on. In \code{instr}, it is the substring to 
check. In \code{format_number},
    --- End diff --
    
    hmm, I see. I think we need to be more clear, since there is meaning for 
columns and their order is significant, for instance,
    
http://spark.apache.org/docs/latest/api/scala/index.html#org.apache.spark.sql.functions$@instr(str:org.apache.spark.sql.Column,substring:String):org.apache.spark.sql.Column
    ```
    instr(str: Column, substring: String)
    ```
    the 2nd column is the substring to look for. maybe it's confusing to list 
column x first?
    
    I wish there is a way to rename parameter in a backward compatible way, but 
I'm not sure there is. perhaps
    ```
    setMethod("instr", signature(y = "Column", substring = "character", x = 
"character"),
    ```
    but that sort of break the generic. maybe
    ```
    setMethod("instr", signature(y = "Column", x = "character"),
    setMethod("instr", signature(y = "Column", substring = "character"),
    ```
    ?
    
    or maybe we should just have instr and format_number in its own rd?
    
    what do you think?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to