Skip to content

Conversation

@felixcheung
Copy link
Member

@shivaram sorry it took longer to fix some conflicts, this is the change to add an alias for table

@SparkQA
Copy link

SparkQA commented Dec 21, 2015

Test build #48090 has finished for PR 10406 at commit 2e4b090.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@felixcheung felixcheung changed the title [SPARK-12232] New R API for read.table to avoid name conflict [SPARK-12232][SPARKR] New R API for read.table to avoid name conflict Dec 21, 2015
@sun-rui
Copy link
Contributor

sun-rui commented Dec 21, 2015

How about "tableToDF" ? there are some API methods having table in their names, like "createExternalTable", "saveAsTable", "tables". "tableToDF" is shorter and consistent.

@felixcheung
Copy link
Member Author

I'm not sure - table in R generally is associated with data.frame (think read.table) or the popular data.table, I actually think saveAsTable and tables should be updated for clarity as well.

@felixcheung
Copy link
Member Author

@shivaram @sun-rui any thought on this?

@sun-rui
Copy link
Contributor

sun-rui commented Dec 30, 2015

IMO, In the context of Spark, table means SQL table. Now that we have so many existing APIs using table in their names, as a new API, it is better to be consistent.

@shivaram
Copy link
Contributor

Sorry for the delay in getting back on this - I agree that using table to refer to SQL table is probably fine. Also since this will go into 2.0 we can probably remove some of the old functions like tables instead of marking them as deprecated ?

@felixcheung
Copy link
Member Author

Sure - I think we are saying this should be called tableToDF. What about tables, saveToTable, cacheTable, dropTempTable, or tableNames?

@shivaram
Copy link
Contributor

So tableToDF actually sounds good to me and its fine to assume that somebody using SparkR thinks of table as a SQL table, so saveToTable, dropTempTable etc. sound fine to me.

We should definitely remove table which conflicts with base R and is completely unrelated. As for tables I'm also inclined to remove it especially if tableNames returns the same thing. Anyway you can open a new JIRA for discussing the rename / removal as well.

@felixcheung
Copy link
Member Author

right.. tables is returning a DataFrame whereas tableNames is returning a vector? I'll open a JIRA to discuss these two.

For this, are we ok to remove table now, instead of .Deprecate I have here?

@shivaram
Copy link
Contributor

Yes - removing table is fine.

@shivaram
Copy link
Contributor

We should make a note in the release notes / deprecation from 1.x to 2.0 list somewhere as well
cc @rxin

@felixcheung
Copy link
Member Author

There's the migration guide: http://spark.apache.org/docs/latest/sparkr.html#migration-guide

@felixcheung
Copy link
Member Author

Done, updated code, tests, and doc.

@shivaram
Copy link
Contributor

Thanks. LGTM. I'll just keep this open for a few hours to see if @sun-rui has any comments

@SparkQA
Copy link

SparkQA commented Jan 20, 2016

Test build #49746 has finished for PR 10406 at commit 42b5af7.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@sun-rui
Copy link
Contributor

sun-rui commented Jan 20, 2016

LGTM

@asfgit asfgit closed this in 488bbb2 Jan 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants