Github user JustinPihony commented on a diff in the pull request:
https://github.com/apache/spark/pull/15263#discussion_r80738565
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JdbcRelationProvider.scala
---
@@ -50,67 +51,52 @@ class
Github user JustinPihony commented on the issue:
https://github.com/apache/spark/pull/12601
@srowen The doc changes have been reviewed, so this should be good to go
---
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
Github user JustinPihony commented on a diff in the pull request:
https://github.com/apache/spark/pull/12601#discussion_r80404639
--- Diff:
sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCWriteSuite.scala ---
@@ -208,4 +210,84 @@ class JDBCWriteSuite extends SharedSQLContext
Github user JustinPihony commented on a diff in the pull request:
https://github.com/apache/spark/pull/12601#discussion_r80404577
--- Diff: docs/sql-programming-guide.md ---
@@ -1096,13 +1096,17 @@ the Data Sources API. The following options are
supported:
{% highlight
Github user JustinPihony commented on a diff in the pull request:
https://github.com/apache/spark/pull/12601#discussion_r80352586
--- Diff:
examples/src/main/java/org/apache/spark/examples/sql/JavaSQLDataSourceExample.java
---
@@ -21,6 +21,7 @@
import java.util.ArrayList
Github user JustinPihony commented on the issue:
https://github.com/apache/spark/pull/12601
@gatorsmile I added the R and SQL documentation. I took the SQL portion
from https://github.com/apache/spark/pull/6121/files
---
If your project is set up for it, you can reply to this email
Github user JustinPihony commented on a diff in the pull request:
https://github.com/apache/spark/pull/12601#discussion_r80350755
--- Diff:
examples/src/main/java/org/apache/spark/examples/sql/JavaSQLDataSourceExample.java
---
@@ -23,6 +23,8 @@
import java.util.List
Github user JustinPihony commented on the issue:
https://github.com/apache/spark/pull/12601
@HyukjinKwon @gatorsmile Could you please review the documentation that I
added so that we can put this to bed :)
---
If your project is set up for it, you can reply to this email and have
Github user JustinPihony commented on the issue:
https://github.com/apache/spark/pull/12601
@srowen Ping. I don't think there is anything on my plate. This should be
mergeable
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub
Github user JustinPihony commented on the issue:
https://github.com/apache/spark/pull/12601
@srowen I had to fix something on my local machine to get proper test
results, but this should be good to go now.
---
If your project is set up for it, you can reply to this email and have
Github user JustinPihony commented on the issue:
https://github.com/apache/spark/pull/12601
@srowen [This documentation is pulled directly from the code
examples](https://github.com/apache/spark/blame/master/docs/sql-programming-guide.md#L1080).
That said, in double checking this I
Github user JustinPihony commented on the issue:
https://github.com/apache/spark/pull/12601
@HyukjinKwon @gatorsmile @srowen Can one of you review my documentation
change so we can get this pushed out :)?
---
If your project is set up for it, you can reply to this email and have
Github user JustinPihony commented on the issue:
https://github.com/apache/spark/pull/12601
@srowen Documentation added.
---
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
Github user JustinPihony commented on the issue:
https://github.com/apache/spark/pull/12601
Can this be merged now?
---
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
Github user JustinPihony commented on the issue:
https://github.com/apache/spark/pull/12601
This seems good now. Could I please get this to at least run through the
automatic tests so we can push this through finally?
---
If your project is set up for it, you can reply
Github user JustinPihony commented on a diff in the pull request:
https://github.com/apache/spark/pull/12601#discussion_r77949242
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JdbcRelationProvider.scala
---
@@ -17,39 +17,105
Github user JustinPihony commented on a diff in the pull request:
https://github.com/apache/spark/pull/12601#discussion_r77949211
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JdbcRelationProvider.scala
---
@@ -17,39 +17,106
Github user JustinPihony commented on a diff in the pull request:
https://github.com/apache/spark/pull/12601#discussion_r77949176
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JdbcRelationProvider.scala
---
@@ -17,39 +17,106
Github user JustinPihony commented on a diff in the pull request:
https://github.com/apache/spark/pull/12601#discussion_r77949090
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JdbcRelationProvider.scala
---
@@ -17,39 +17,106
Github user JustinPihony commented on a diff in the pull request:
https://github.com/apache/spark/pull/12601#discussion_r77948243
--- Diff:
sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCWriteSuite.scala ---
@@ -208,4 +208,75 @@ class JDBCWriteSuite extends SharedSQLContext
Github user JustinPihony commented on a diff in the pull request:
https://github.com/apache/spark/pull/12601#discussion_r77948196
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JdbcRelationProvider.scala
---
@@ -17,39 +17,105
Github user JustinPihony commented on a diff in the pull request:
https://github.com/apache/spark/pull/12601#discussion_r77948104
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JdbcRelationProvider.scala
---
@@ -17,39 +17,105
Github user JustinPihony commented on a diff in the pull request:
https://github.com/apache/spark/pull/12601#discussion_r77947902
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JdbcRelationProvider.scala
---
@@ -17,39 +17,113
Github user JustinPihony commented on a diff in the pull request:
https://github.com/apache/spark/pull/12601#discussion_r77946835
--- Diff:
sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCWriteSuite.scala ---
@@ -208,4 +208,75 @@ class JDBCWriteSuite extends SharedSQLContext
Github user JustinPihony commented on a diff in the pull request:
https://github.com/apache/spark/pull/12601#discussion_r77946637
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JdbcRelationProvider.scala
---
@@ -17,39 +17,105
Github user JustinPihony commented on a diff in the pull request:
https://github.com/apache/spark/pull/12601#discussion_r77946434
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JdbcRelationProvider.scala
---
@@ -17,39 +17,105
Github user JustinPihony commented on a diff in the pull request:
https://github.com/apache/spark/pull/12601#discussion_r77946299
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JdbcRelationProvider.scala
---
@@ -17,39 +17,113
Github user JustinPihony commented on the issue:
https://github.com/apache/spark/pull/12601
@gatorsmile wrt tests, I had actually had these but stashed them to make a
merge easier and forgot to re-add them. I have since pushed them :)
---
If your project is set up for it, you can
Github user JustinPihony commented on the issue:
https://github.com/apache/spark/pull/12601
@HyukjinKwon You are correct that it is a different issue, so I went ahead
and removed the code, which actually showed that it was closer to ~5 lines of
code... So can we please move past
Github user JustinPihony commented on the issue:
https://github.com/apache/spark/pull/12601
@gatorsmile Again, because the work is already done and it allows a broader
flexibility for the user. It is only ~20 lines of simple code. What is the
reason to not have it?
---
If your
Github user JustinPihony commented on the issue:
https://github.com/apache/spark/pull/12601
@srowen I have addressed your concerns and also merged the branch to handle
the past few changes to jdbc. I think this should be good to go now.
---
If your project is set up for it, you can
Github user JustinPihony commented on the issue:
https://github.com/apache/spark/pull/12601
Yes I realize that `SchemaRelationProvider` is not necessary, but the
legwork has already been done, so why not take advantage of it. Even still,
this original PR is actually not that far from
Github user JustinPihony commented on the issue:
https://github.com/apache/spark/pull/12601
@gatorsmile I addressed the rollback in [this
comment](https://github.com/apache/spark/pull/12601#issuecomment-233076856) and
haven't heard back one way or the other. @srowen ?
---
If your
Github user JustinPihony commented on a diff in the pull request:
https://github.com/apache/spark/pull/12601#discussion_r77239532
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCRelation.scala
---
@@ -114,7 +115,9 @@ private[sql] case class
Github user JustinPihony commented on a diff in the pull request:
https://github.com/apache/spark/pull/12601#discussion_r77238783
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JdbcRelationProvider.scala
---
@@ -17,39 +17,105
Github user JustinPihony commented on the issue:
https://github.com/apache/spark/pull/12601
@srowen I have addressed all of your comments. The only changes made were
to remove `sys.error` and use `require` where appropriate.
---
If your project is set up for it, you can reply
Github user JustinPihony commented on a diff in the pull request:
https://github.com/apache/spark/pull/12601#discussion_r77067046
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCOptions.scala
---
@@ -36,4 +38,9 @@ private[jdbc] class
Github user JustinPihony commented on a diff in the pull request:
https://github.com/apache/spark/pull/12601#discussion_r77060202
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCOptions.scala
---
@@ -36,4 +36,9 @@ private[jdbc] class
Github user JustinPihony commented on a diff in the pull request:
https://github.com/apache/spark/pull/12601#discussion_r77061367
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JdbcRelationProvider.scala
---
@@ -19,37 +19,100 @@ package
Github user JustinPihony commented on a diff in the pull request:
https://github.com/apache/spark/pull/12601#discussion_r77060138
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCOptions.scala
---
@@ -36,4 +36,9 @@ private[jdbc] class
Github user JustinPihony commented on a diff in the pull request:
https://github.com/apache/spark/pull/12601#discussion_r77060790
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JdbcRelationProvider.scala
---
@@ -19,37 +19,100 @@ package
Github user JustinPihony commented on a diff in the pull request:
https://github.com/apache/spark/pull/12601#discussion_r77058751
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/DataFrameWriter.scala ---
@@ -396,49 +396,14 @@ final class DataFrameWriter[T] private[sql](ds
Github user JustinPihony commented on the issue:
https://github.com/apache/spark/pull/12601
@rxin I am sure you are busy, so if there is anybody else I can ping to
have this reviewed please let me know.
---
If your project is set up for it, you can reply to this email and have your
Github user JustinPihony commented on the issue:
https://github.com/apache/spark/pull/12601
@rxin Another ping for this to be reviewed please :)
---
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
Github user JustinPihony commented on the issue:
https://github.com/apache/spark/pull/12601
Bumping my JIRA comment to here for @rxin to respond please?
@rxin Given the bug found in SPARK-16401, the CreatableRelationProvider is
not necessary. However it might be nice to have
Github user JustinPihony commented on the issue:
https://github.com/apache/spark/pull/14077
@gatorsmile As I said above, I actually think it might be better to keep
the work that was already done and am waiting for Reynold's feedback.
---
If your project is set up for it, you can
Github user JustinPihony commented on the issue:
https://github.com/apache/spark/pull/14077
Thanks. I will have to wait until SPARK-16401 is resolved or else the code
will not pass tests, though.
---
If your project is set up for it, you can reply to this email and have your
reply
Github user JustinPihony commented on the issue:
https://github.com/apache/spark/pull/14077
Then the best course of action would be to use my current impl as it works
no matter the position of copy. I can add the additional tests if that would
make it more amenable? Otherwise I'll
Github user JustinPihony commented on the issue:
https://github.com/apache/spark/pull/14077
@gatorsmile If `copy` is a bug, then that is fine with me (I just commented
my findings on this and will be curious to hear back). That said, it would make
my implementation simpler. I'd
Github user JustinPihony commented on the issue:
https://github.com/apache/spark/pull/14075
@rxin It does look like this might have been a regression introduced via
[the initial creation of
`DataSource`](https://github.com/apache/spark/blob/1e28840594b9d972c96d3922ca0bf0f76e313e82
Github user JustinPihony commented on the issue:
https://github.com/apache/spark/pull/12601
@gatorsmile I did just review it and still prefer mine...a simpler PR does
not necessarily mean it is more correct.
---
If your project is set up for it, you can reply to this email and have
Github user JustinPihony commented on the issue:
https://github.com/apache/spark/pull/14077
This may seem simpler, but that's because it seems to be taking some
shortcuts to avoid having to refactor. This currently creates a cycle along the
lines of `df.save.df.jdbc`. Wouldn't
Github user JustinPihony commented on the issue:
https://github.com/apache/spark/pull/12601
Bump @HyukjinKwon I have some comments to your comments. Could you please
review them and I can push my changes.
---
If your project is set up for it, you can reply to this email and have
Github user JustinPihony commented on a diff in the pull request:
https://github.com/apache/spark/pull/12601#discussion_r67582844
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JdbcRelationProvider.scala
---
@@ -19,37 +19,105 @@ package
Github user JustinPihony commented on a diff in the pull request:
https://github.com/apache/spark/pull/12601#discussion_r67582037
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCRelation.scala
---
@@ -96,7 +97,16 @@ private[sql] case class
Github user JustinPihony commented on a diff in the pull request:
https://github.com/apache/spark/pull/12601#discussion_r67539731
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JdbcRelationProvider.scala
---
@@ -19,37 +19,105 @@ package
Github user JustinPihony commented on the issue:
https://github.com/apache/spark/pull/12601
Bump :) Anybody able to review this one for me please?
---
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
Github user JustinPihony commented on a diff in the pull request:
https://github.com/apache/spark/pull/12601#discussion_r66009900
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCRelation.scala
---
@@ -96,7 +97,16 @@ private[sql] case class
Github user JustinPihony commented on the pull request:
https://github.com/apache/spark/pull/12601#issuecomment-220431230
I just updated the branch to have no conflicts. Again, either the code
looks good to merge, or I can make JDBC a `CreatableRelationProvider` (but that
comes
Github user JustinPihony commented on the pull request:
https://github.com/apache/spark/pull/12601#issuecomment-217359217
I can finish this next Monday(fix the conflicts that now exist), and will
actually do that given the above comments. I'd still like to get an opinion on
whether I
Github user JustinPihony commented on the pull request:
https://github.com/apache/spark/pull/12601#issuecomment-213662908
@HyukjinKwon I just posted on the JIRA the background of `Properties` and
how reasonable it is to assume it can be converted to a `String`.
---
If your project
Github user JustinPihony commented on a diff in the pull request:
https://github.com/apache/spark/pull/12601#discussion_r60752881
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/DataFrameWriter.scala ---
@@ -244,7 +244,11 @@ final class DataFrameWriter private[sql](df
Github user JustinPihony commented on the pull request:
https://github.com/apache/spark/pull/12601#issuecomment-213463779
@HyukjinKwon You will notice that I opted to not deprecate jdbc as I don't
think that would be the correct path anyway (unless all format methods were
GitHub user JustinPihony opened a pull request:
https://github.com/apache/spark/pull/12601
[SPARK-14525][SQL] Make DataFrameWrite.save work for jdbc
## What changes were proposed in this pull request?
This change modifies the implementation of DataFrameWriter.save
64 matches
Mail list logo
