Skip to content

Conversation

@beliefer
Copy link
Contributor

@beliefer beliefer commented Mar 6, 2023

What changes were proposed in this pull request?

Currently, the connect project have the new DataFrameWriter API which is corresponding to Spark DataFrameWriter API. But the connect's DataFrameWriter missing the jdbc API.

Why are the changes needed?

This PR try to add JDBC to DataFrameWriter.

Does this PR introduce any user-facing change?

'No'.
New feature.

How was this patch tested?

@hvanhovell It seems that add test cases no way.

@beliefer
Copy link
Contributor Author

beliefer commented Mar 6, 2023

@hvanhovell It seems that add test cases no way.

@hvanhovell
Copy link
Contributor

hmmm - let me think about it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a question @hvanhovell @beliefer . For the connect-client api, should we verify the parameters on the client side or on the server side?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think verify parameters on the server side is a robust way. Certainly, the work on client side will reduce the pressure on the server side.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Server side. There are a couple of reasons for this:

  • The server cannot trust the client to implement the verification properly. I am sure we will get it right for Scala and Python, but there are potentially a plethora of other frontends that need to do the same.
  • Keeping the client simple and reduce duplication. If we need to do this for every client we'll end up with a lot of duplication and increase client complexity.

@hvanhovell
Copy link
Contributor

@beliefer we should be able to create an in-memory table and append a couple of rows to that right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need a separate method? There is only one method using it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is calling save() right? Why not call it TABLE_SAVE_METHOD_SAVE?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can remove ProblemFilters.exclude[Problem]("org.apache.spark.sql.DataFrameWriter.jdbc") from CheckConnectJvmClientCompatibility in this pr
`

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the reminder.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the move?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems the server can't find the jar from class path.
image

Copy link
Contributor

@hvanhovell hvanhovell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Can you look into the test failure?

@beliefer beliefer closed this Mar 29, 2023
@hvanhovell
Copy link
Contributor

@beliefer are you abandoning this one?

@beliefer
Copy link
Contributor Author

@beliefer are you abandoning this one?

Because other PR implement this function.

@hvanhovell
Copy link
Contributor

Is that #40415?

@beliefer
Copy link
Contributor Author

Is that #40415?

It is #40358

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants