-
Notifications
You must be signed in to change notification settings - Fork 28.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-12318][SPARKR] Save mode in SparkR should be error by default #10290
Conversation
Test build #47649 has finished for PR 10290 at commit
|
@@ -1903,7 +1903,7 @@ setMethod("except", | |||
#' } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you add the default to this doc line above:
#' @param mode One of 'append', 'overwrite', 'error', 'ignore' save mode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@felixcheung Is it necessary to do that ? Because I notice there's no doc for the default value in other R APIs. Actually user can see the default value in the function signature which is also in the R doc.
## S4 method for signature 'DataFrame,character'
write.df(df, path, source = NULL,
mode = "error", ...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I think that's just good practice, but it's fine both ways.
Document this change into migration guide? |
Make sense, will do. |
Test build #47713 has finished for PR 10290 at commit
|
I don't think we have a migration guide section in SparkR programming guide but we should, like sql: |
@felixcheung yes, there's no migration doc for R. So I just put the doc in the data frame example. I can create migration section if necessary. |
Yeah lets create a migration section -- This also came up in another PR and I think is useful to have. |
Test build #47719 has finished for PR 10290 at commit
|
@@ -148,7 +148,7 @@ printSchema(people) | |||
</div> | |||
|
|||
The data sources API can also be used to save out DataFrames into multiple file formats. For example we can save the DataFrame from the previous example | |||
to a Parquet file using `write.df` | |||
to a Parquet file using `write.df` (Before spark 1.7, mode's default value is 'append', we change it to 'error' to be consistent with scala api) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you reword this to
Until Spark 1.6, the default mode for writes was `append`. It was changed in Spark 1.7 to `error` to match the Scala API
One minor thing is that I'm not sure its very good to hard code these version numbers here. For example we could have a RC3 and then this might be in Spark 1.6 cc @JoshRosen Are there known good practices on how to handle documentation changes of this form ? |
Cc @marmbrus it'll be good to have this PR in rc3. There are some unaddressed docs comments but I can fix them up while merging ? |
Sorry, RC3 has already been cut. |
Test build #47768 has finished for PR 10290 at commit
|
LGTM. Merging into master (and not |
@shivaram actually, i'm going to have to cut RC3 again. You get a second chance if you merge now! |
Oh sure cherry picking now |
shivaram Please help review. Author: Jeff Zhang <zjffdu@apache.org> Closes #10290 from zjffdu/SPARK-12318. (cherry picked from commit 2eb5af5) Signed-off-by: Shivaram Venkataraman <shivaram@cs.berkeley.edu>
Done ! Thanks @marmbrus for the heads up |
@shivaram Please help review.