-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Add Salesforce JDBC Connector #2548
Conversation
262c062
to
733cd8c
Compare
0299090
to
7819474
Compare
aff701e
to
14e552f
Compare
@Praveen2112, can you help review this? |
@martint Yes |
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.
Thanks for working on this.
presto-salesforce/src/main/java/io/prestosql/plugin/salesforce/SalesforceClientModule.java
Outdated
Show resolved
Hide resolved
presto-salesforce/src/main/java/io/prestosql/plugin/salesforce/SalesforceClientModule.java
Outdated
Show resolved
Hide resolved
presto-salesforce/src/main/java/io/prestosql/plugin/salesforce/SalesforceClientModule.java
Outdated
Show resolved
Hide resolved
presto-salesforce/src/main/java/io/prestosql/plugin/salesforce/SalesforceClient.java
Outdated
Show resolved
Hide resolved
presto-salesforce/src/main/java/io/prestosql/plugin/salesforce/SalesforceClient.java
Outdated
Show resolved
Hide resolved
private static BlockWriteFunction multiPicklistWriteFunction(Type type) | ||
{ | ||
return (statement, index, value) -> { | ||
// Not implemented |
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.
Any reasons for not implementing it ? It would be iterating over that value
and building a string ?
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.
Pure laziness at this point; the JDBC driver does not support writes yet.
...-salesforce/src/main/java/io/prestosql/plugin/salesforce/driver/soap/ForceSoapValidator.java
Outdated
Show resolved
Hide resolved
presto-salesforce/src/main/java/io/prestosql/plugin/salesforce/driver/statement/FieldDef.java
Show resolved
Hide resolved
presto-salesforce/src/main/java/io/prestosql/plugin/salesforce/driver/statement/FieldDef.java
Outdated
Show resolved
Hide resolved
presto-salesforce/src/main/java/io/prestosql/plugin/salesforce/driver/statement/FieldDef.java
Outdated
Show resolved
Hide resolved
...rce/src/main/java/io/prestosql/plugin/salesforce/driver/statement/ParameterMetadataImpl.java
Outdated
Show resolved
Hide resolved
...lesforce/src/main/java/io/prestosql/plugin/salesforce/driver/connection/ForceConnection.java
Show resolved
Hide resolved
metadataCache.put(this.partnerConnection.getConfig().getUsername(), this.metadata); | ||
} | ||
|
||
this.metadata.setConnection(this); |
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.
But it won't be thread safe right ?
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.
Right.
...lesforce/src/main/java/io/prestosql/plugin/salesforce/driver/connection/ForceConnection.java
Outdated
Show resolved
Hide resolved
...lesforce/src/main/java/io/prestosql/plugin/salesforce/driver/connection/ForceConnection.java
Outdated
Show resolved
Hide resolved
...lesforce/src/main/java/io/prestosql/plugin/salesforce/driver/connection/ForceConnection.java
Outdated
Show resolved
Hide resolved
1a1c169
to
e0ac5c5
Compare
<repository> | ||
<id>mulesoft-releases</id> | ||
<name>MuleSoft Releases Repository</name> | ||
<url>http://repository.mulesoft.org/releases/</url> | ||
<layout>default</layout> | ||
</repository> |
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.
Are the artifacts in maven central? It's not a good idea to depend on third-party repositories. It makes the build more brittle and makes it hard for people that are using a maven proxy in their organizations.
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.
Good point. Let me check on that.
@@ -941,7 +941,6 @@ private static String escapeNamePattern(String name, String escape) | |||
{ | |||
requireNonNull(name, "name is null"); | |||
requireNonNull(escape, "escape is null"); | |||
checkArgument(!escape.isEmpty(), "Escape string must not be empty"); |
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.
Can we implement it as a separate commit
public SalesforceClient(BaseJdbcConfig baseConfig, SalesforceConfig salesforceConfig, ConnectionFactory connectionFactory) | ||
{ | ||
super(baseConfig, "", connectionFactory); | ||
|
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.
This line can be removed
import java.util.List; | ||
import java.util.stream.Collectors; | ||
|
||
import static org.junit.Assert.assertEquals; |
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.
We can also use TestNG right ?
.to(SalesforceClient.class) | ||
.in(Scopes.SINGLETON); | ||
|
||
configBinder(binder).bindConfig(BaseJdbcConfig.class); |
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.
It would be binded in BaseJdbcModules so I guess we might not have to bind here
.in(Scopes.SINGLETON); | ||
|
||
configBinder(binder).bindConfig(BaseJdbcConfig.class); | ||
configBinder(binder).bindConfig(TypeHandlingJdbcConfig.class); |
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.
It would be binded in BaseJdbcModules so I guess we might not have to bind here
import java.util.regex.Matcher; | ||
import java.util.regex.Pattern; | ||
|
||
public class ForceDriver |
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.
We can also add services file for this
throw new SQLFeatureNotSupportedException(); | ||
} | ||
|
||
static { |
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.
We can move this static method up
|
||
import java.io.Serializable; | ||
|
||
public class Column |
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.
Can we have a builder for it ?
import static java.util.Objects.requireNonNull; | ||
import static java.util.stream.Collectors.joining; | ||
|
||
public class SoqlQueryBuilder |
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.
We can extend QueryBuilder
?
{ | ||
}).in(Scopes.SINGLETON); | ||
|
||
binder.requestStaticInjection(ForceConnection.class); |
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.
Where do we inject this ?
wonder if there is any progress with the salesforce connector? |
Could this PR be completed? |
Closing after discussion of @bitsondatadev with @pgagnon . If anyone wants to pick this up, feel free to take the code in this PR and adjust as necessary. |
Closes #1854