Skip to content

Conversation

@draliatp
Copy link

No description provided.

@salesforce-cla
Copy link

Thanks for the contribution! Before we can merge this, we need @draliatp to sign the Salesforce Inc. Contributor License Agreement.

@j10t j10t requested a review from jschneidereit April 2, 2025 01:49

With the Salesforce Data Cloud JDBC driver you can efficiently query millions of rows of data with low latency, and perform bulk data extractions.
This driver is read-only, forward-only, and requires Java 8 or greater. It uses the new [Data Cloud Query API SQL syntax](https://developer.salesforce.com/docs/data/data-cloud-query-guide/references/dc-sql-reference/data-cloud-sql-context.html).
This driver allows you to efficiently query large datasets in Salesforce Data Cloud with low latency and perform bulk data extractions. It's read-only, forward-only, and requires Java 8 or later. The driver uses the Data Cloud Query API SQL syntax (see [Data Cloud Query API SQL syntax](https://developer.salesforce.com/docs/data/data-cloud-query-guide/references/dc-sql-reference/data-cloud-sql-context.html) for details).
Copy link
Contributor

Choose a reason for hiding this comment

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

what does "forward-only" mean? I never heard that term before in the context of a JDBC driver?

Copy link

Choose a reason for hiding this comment

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

ResultSet implements getType(), ours returns TYPE_FORWARD_ONLY.

Clients use this to know whether or not the ResultSet is scrollable, ours is not.

https://docs.oracle.com/javase/8/docs/api/java/sql/ResultSet.html#getType--

Copy link
Contributor

Choose a reason for hiding this comment

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

makes sense. Is "forward-only" a noteworthy enough restriction to mention it immediately in the second sentence?

To me this sounds more like an implementation detail. Most users will not really care about whether our result-sets are forward-only or not. Or am I missing something?

Copy link

Choose a reason for hiding this comment

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

I’m fairly indifferent -- we can remove it, no problem. That said, I also see value in keeping it as-is since it’s a high-level functional limitation that could impact whether the driver suits an application, same as read-only. On the other hand, unlike read-only, forward-only is common among big data JDBC drivers and rarely a blocker, so most readers wouldn’t miss it if we removed it here.

Since it seemed surprisingly prominent to you here, I'd err on the side of removing it.


With the Salesforce Data Cloud JDBC driver you can efficiently query millions of rows of data with low latency, and perform bulk data extractions.
This driver is read-only, forward-only, and requires Java 8 or greater. It uses the new [Data Cloud Query API SQL syntax](https://developer.salesforce.com/docs/data/data-cloud-query-guide/references/dc-sql-reference/data-cloud-sql-context.html).
This driver allows you to efficiently query large datasets in Salesforce Data Cloud with low latency and perform bulk data extractions. It's read-only, forward-only, and requires Java 8 or later. The driver uses the Data Cloud Query API SQL syntax (see [Data Cloud Query API SQL syntax](https://developer.salesforce.com/docs/data/data-cloud-query-guide/references/dc-sql-reference/data-cloud-sql-context.html) for details).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This driver allows you to efficiently query large datasets in Salesforce Data Cloud with low latency and perform bulk data extractions. It's read-only, forward-only, and requires Java 8 or later. The driver uses the Data Cloud Query API SQL syntax (see [Data Cloud Query API SQL syntax](https://developer.salesforce.com/docs/data/data-cloud-query-guide/references/dc-sql-reference/data-cloud-sql-context.html) for details).
This driver allows you to efficiently query large datasets in Salesforce Data Cloud with low latency and perform bulk data extractions. It's read-only, forward-only, and requires Java 8 or later. The driver uses the [Data Cloud Query API SQL syntax](https://developer.salesforce.com/docs/data/data-cloud-query-guide/references/dc-sql-reference/data-cloud-sql-context.html).

I don't think we need a parenthesized sentence here, we can simply put the link directly into the sentence

### Maven Dependency

To add the driver to your project, add the following Maven dependency:
To use the driver in your Maven project, add the following to your `pom.xml`:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
To use the driver in your Maven project, add the following to your `pom.xml`:
To use the driver in your Maven project, add the following dependency to your `pom.xml`:

not sure, reads more natural to me that way... but I am not a native speaker, up to you, Dralia...

Comment on lines +25 to 32
## Building the Driver

```shell
To build and test the driver, run:

```bash
mvn clean install
```

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 should move this down all the way to the end of this document? Our customers should never need to build the driver from source. Building the driver is only a task which our own developers (and other open-source contributors) will have to do

Comment on lines +55 to +57
| `user` | Salesforce username. | Username/Password |
| `password` | Salesforce password. | Username/Password |
| `clientId` | Connected app consumer key. | All |
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 should put the "All" entry to the top, instead of somwhere in the middle of the table

Suggested change
| `user` | Salesforce username. | Username/Password |
| `password` | Salesforce password. | Username/Password |
| `clientId` | Connected app consumer key. | All |
| `clientId` | Connected app consumer key. | All |
| `user` | Salesforce username. | Username/Password |
| `password` | Salesforce password. | Username/Password |

Remember to replace placeholders like `${userName}` and `${query}` with your actual values. Consider using parameterized queries to prevent SQL injection vulnerabilities.

To find examples of these generated assertions, look for files with the path `**/test/**/*Assert.java`.
## Generated Assertions
Copy link
Contributor

Choose a reason for hiding this comment

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

this is only relevant for developers of the JDBC driver, not for our users.

We should probably group this together with the "Building the Driver" section

Contributing to the JDBC driver

Contributions to this repository from the open-source community are welcome. For straightforward changes, feel free to just open a GIthub Pull Request. For more complex changes, we recommend to first discuss your envisioned change with us via a GIthub Issue.

Building the Driver

To build and test the driver, run:

mvn clean install

Re-generating the Assertions

Some classes use generated assertions (from AssertJ Assertions Generator). If you modify these classes, regenerate the assertions:

mvn assertj:generate-assertions

```
com.salesforce.datacloud.jdbc.DataCloudJDBCDriver
```
### Driver Class
Copy link
Contributor

@vogelsgesang vogelsgesang Apr 2, 2025

Choose a reason for hiding this comment

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

The "Driver Class (Reiterated)" section below should be sufficient. I don't think we need this section, at least I am not sure which value it provides to re-iterate the same information in both places

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