Avatical Removal Part 3/3: Remove Avatica dependency completely by carsheng · Pull Request #166 · forcedotcom/datacloud-jdbc

carsheng · 2026-04-06T21:47:20Z

Completes the removal of Apache Calcite Avatica from jdbc-core. The driver no longer depends on Avatica for result set handling, cursor management, type accessors

Remove Avatica from StreamingResultSet: no longer extends AvaticaResultSet. Implements DataCloudResultSet directly with all JDBC ResultSet methods delegating to QueryJDBCAccessor instances. Adds proper checkClosed() guards on all methods.
Remove Avatica cursor layer: delete MetadataCursor (extended AbstractCursor) and MetadataResultSet (extended AvaticaResultSet). Replace with DataCloudMetadataResultSet, a standalone metadata result set implementation.
Extract DataCloudQueryAccessor interface: defines the accessor contract (all typed getters) that QueryJDBCAccessor implements, replacing the dependency on Avatica's AbstractCursor.Accessor.
Rename for clarity: SimpleResultSetMetaData → DataCloudResultSetMetaData, SimpleMetadataResultSet → DataCloudMetadataResultSet, DataCloudAccessor → DataCloudQueryAccessor.

codecov · 2026-04-06T21:53:44Z

Codecov Report

❌ Patch coverage is 83.79310% with 47 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.94%. Comparing base (f01c77a) to head (6cf0615).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...sforce/datacloud/jdbc/core/StreamingResultSet.java	73.71%	46 Missing ⚠️
...atacloud/jdbc/core/accessor/QueryJDBCAccessor.java	50.00%	1 Missing ⚠️

❌ Your patch check has failed because the patch coverage (83.79%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #166      +/-   ##
============================================
- Coverage     82.40%   81.94%   -0.47%     
- Complexity     1568     1585      +17     
============================================
  Files           119      116       -3     
  Lines          4422     4464      +42     
  Branches        457      450       -7     
============================================
+ Hits           3644     3658      +14     
- Misses          556      587      +31     
+ Partials        222      219       -3

Components	Coverage Δ
JDBC Core	`82.74% <83.79%> (-0.62%)`	⬇️
JDBC Main	`40.69% <ø> (ø)`
JDBC HTTP	`89.76% <ø> (ø)`
JDBC Utilities	`65.25% <ø> (-0.83%)`	⬇️
Spark Datasource	`∅ <ø> (∅)`

Files with missing lines	Coverage Δ
...e/datacloud/jdbc/core/ArrowStreamReaderCursor.java	`88.00% <100.00%> (+1.79%)`	⬆️
...sforce/datacloud/jdbc/core/ColumnNameResolver.java	`100.00% <100.00%> (ø)`
...force/datacloud/jdbc/core/DataCloudConnection.java	`58.27% <100.00%> (-0.60%)`	⬇️
...datacloud/jdbc/core/DataCloudDatabaseMetadata.java	`98.34% <100.00%> (ø)`
...atacloud/jdbc/core/DataCloudMetadataResultSet.java	`77.19% <100.00%> (ø)`
...esforce/datacloud/jdbc/core/QueryMetadataUtil.java	`91.45% <100.00%> (ø)`
...d/jdbc/core/accessor/QueryJDBCAccessorFactory.java	`98.46% <100.00%> (ø)`
...jdbc/core/accessor/impl/BaseIntVectorAccessor.java	`80.85% <100.00%> (-0.40%)`	⬇️
...dbc/core/accessor/impl/BaseListVectorAccessor.java	`93.33% <100.00%> (-0.79%)`	⬇️
.../jdbc/core/accessor/impl/BinaryVectorAccessor.java	`100.00% <100.00%> (ø)`
... and 19 more

... and 4 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

...e/src/test/java/com/salesforce/datacloud/jdbc/core/metadata/SimpleResultSetMetaDataTest.java

mkaufmann · 2026-04-07T16:03:27Z

jdbc-core/src/test/java/com/salesforce/datacloud/jdbc/core/QueryJDBCDataCursorTest.java

-    public void testCursorNextAndRowCount() {
-        List<Object> data = new ArrayList<>();
-        data.add("Account_home_dll");
-        cursor = new MetadataCursor(data);
-        cursor.next();
-        assertThat(cursor.next()).isFalse();
-    }


Do we have test coverage for cursor somewhere else?

jdbc-core/src/main/java/com/salesforce/datacloud/jdbc/core/accessor/DataCloudAccessor.java

mkaufmann · 2026-04-07T16:05:08Z

...re/src/main/java/com/salesforce/datacloud/jdbc/core/metadata/DataCloudResultSetMetaData.java

+        // Arrow timestamps with timezone map to JDBC TIMESTAMP, not TIMESTAMP_WITH_TIMEZONE
+        if (typeId == Types.TIMESTAMP_WITH_TIMEZONE) {
+            return Types.TIMESTAMP;
+        }


Why do we need to adjust that here? Feels wrong to me to override the column type

It does feel wrong, however the reason for this right now is that Arrow's TIMESTAMPSECTZ / TIMESTAMPMILLITZ / etc. store an epoch value + a timezone string, but the TimeStampVectorAccessor already normalizes these to UTC LocalDateTime / Timestamp values before returning them. By the time the JDBC caller gets the value via getTimestamp(), the timezone has already been folded into the instant — the returned java.sql.Timestamp doesn't carry timezone info.

If we remove the override, and TimeStampVectorAccessor remains as is, JDBCReferenceTest fails because it expects select timestamptz '1000-01-01 01:00:00+0100' to map to columnType=93 and className java.sql.TimeStamp

My view is that this is a symptom of the fact we don't currently correctly handle timestamp/timezone types in the JDBC driver, and that this should go away once #158 gets in

@KaviarasuSakthivadivel / @carsheng let's discuss on how we want to sequence the two PRs.

jdbc-core/src/main/java/com/salesforce/datacloud/jdbc/core/ArrowStreamReaderCursor.java

mkaufmann · 2026-04-07T16:07:12Z

jdbc-core/src/main/java/com/salesforce/datacloud/jdbc/core/ColumnNameResolver.java

+        for (int i = 0; i < columns.size(); i++) {
+            ColumnMetadata col = columns.get(i);
+            String label = col.getName();
+            if (label != null) {
                // Use putIfAbsent to ensure first occurrence (lowest ordinal) wins for duplicates
-                exactColumnLabelMap.putIfAbsent(columnMetaData.label, columnMetaData.ordinal);
-            }
-        }
-
-        // Second pass: index lowercase labels to ordinals, but only if the lowercase key doesn't exist yet
-        // This ensures the first column with a given lowercase name wins (lowest ordinal)
-        for (ColumnMetaData columnMetaData : columns) {
-            if (columnMetaData.label != null) {
-                String lowerLabel = columnMetaData.label.toLowerCase();
-                // Only add if this lowercase key hasn't been seen yet (preserves first occurrence)
-                lowercaseColumnLabelMap.putIfAbsent(lowerLabel, columnMetaData.ordinal);
+                exactColumnLabelMap.putIfAbsent(label, i);
+                lowercaseColumnLabelMap.putIfAbsent(label.toLowerCase(), i);


Doesn#t this change behavior? I think this should break a test case

Do we need to change this behaviour?

Actually taking a second look I think the new logic is correct. On the lookup side we always first check exact and then lower case. Having a single pass is more efficient than interating twice.

Please keep the documentation on the semantics of the two maps and how this is working.

jdbc-core/src/main/java/com/salesforce/datacloud/jdbc/core/accessor/DataCloudAccessor.java

jdbc-core/src/main/java/com/salesforce/datacloud/jdbc/core/accessor/QueryJDBCAccessor.java

jdbc-core/src/main/java/com/salesforce/datacloud/jdbc/core/StreamingResultSet.java

jdbc-core/src/main/java/com/salesforce/datacloud/jdbc/core/ArrowStreamReaderCursor.java

mkaufmann · 2026-04-08T15:55:21Z

...-core/src/main/java/com/salesforce/datacloud/jdbc/core/metadata/SimpleResultSetMetaData.java

+        // Match standard JDBC behavior: readOnly=false does not mean the column is writable,
+        // it means we don't know. isWritable() and isDefinitelyWritable() still return false.
+        return false;


I think the old behavior was correct because we know that this column can't be written through SQL as DC only supports SELECT clauses

At first i thought so too, but i've since realized the following the "old behavior" was actually incorrect.

According to QueryJDBCReferenceTest, which compares behavior against the Postgres driver, isReadOnly=false for a column is in line w/ what Postgres driver does.
So the correct behavior should be:

Generally: ResultSet.isReadonly=true // the resultset itself isReadonly, we don't support write operations For this line: ResultSet.getMetadata.isReadOnly(1)=false //a column is technically not readonly

Why was this not caught earlier?
SimpleResultSetMetadata in the earlier avatica PR (aka, "old behavior") were limited to only the metadata flow. Now that that SimpleResultSetMetadata is now incorporated into the normal ResultSet flow (i.e. u make a query, then look at the metadata), QueryJDBCReferenceTest fails b/c it expects column.isReadonly=false. See here in reference.json: https://github.com/forcedotcom/datacloud-jdbc/blob/main/jdbc-reference/src/main/resources/reference.json#L19

isReadOnly=false for a column is in line w/ what Postgres driver does.

Postgres driver is setting this to falser because one can do insert / update statements in Postgres. In Data Cloud that doesn't work so for us it would be correct to set isReadOnly true AFAIK

mkaufmann · 2026-04-08T15:59:27Z

jdbc-core/src/main/java/com/salesforce/datacloud/jdbc/core/ColumnNameResolver.java

+        for (int i = 0; i < columns.size(); i++) {
+            ColumnMetadata col = columns.get(i);
+            String label = col.getName();
+            if (label != null) {
                // Use putIfAbsent to ensure first occurrence (lowest ordinal) wins for duplicates
-                exactColumnLabelMap.putIfAbsent(columnMetaData.label, columnMetaData.ordinal);
-            }
-        }
-
-        // Second pass: index lowercase labels to ordinals, but only if the lowercase key doesn't exist yet
-        // This ensures the first column with a given lowercase name wins (lowest ordinal)
-        for (ColumnMetaData columnMetaData : columns) {
-            if (columnMetaData.label != null) {
-                String lowerLabel = columnMetaData.label.toLowerCase();
-                // Only add if this lowercase key hasn't been seen yet (preserves first occurrence)
-                lowercaseColumnLabelMap.putIfAbsent(lowerLabel, columnMetaData.ordinal);
+                exactColumnLabelMap.putIfAbsent(label, i);
+                lowercaseColumnLabelMap.putIfAbsent(label.toLowerCase(), i);


Actually taking a second look I think the new logic is correct. On the lookup side we always first check exact and then lower case. Having a single pass is more efficient than interating twice.

Please keep the documentation on the semantics of the two maps and how this is working.

jdbc-core/src/main/java/com/salesforce/datacloud/jdbc/core/StreamingResultSet.java

…& SimpleMetadataResultSet-->DataCloudMetadataResultSet

refactor: avatica phase 3

c0c7aa4

refactor: remove scratch md file

d5f95f3

mkaufmann approved these changes Apr 7, 2026

View reviewed changes

mkaufmann self-requested a review April 7, 2026 16:03

mkaufmann reviewed Apr 7, 2026

View reviewed changes

carsheng changed the title ~~Avatical Removal Part 2/3: Remove Avatica dependency completely~~ Avatical Removal Part 3/3: Remove Avatica dependency completely Apr 7, 2026

KaviarasuSakthivadivel reviewed Apr 7, 2026

View reviewed changes

jdbc-core/src/main/java/com/salesforce/datacloud/jdbc/core/accessor/DataCloudAccessor.java Outdated Show resolved Hide resolved

mkaufmann reviewed Apr 7, 2026

View reviewed changes

jdbc-core/src/main/java/com/salesforce/datacloud/jdbc/core/accessor/QueryJDBCAccessor.java Outdated Show resolved Hide resolved

mkaufmann reviewed Apr 7, 2026

View reviewed changes

jdbc-core/src/main/java/com/salesforce/datacloud/jdbc/core/StreamingResultSet.java Outdated Show resolved Hide resolved

mkaufmann reviewed Apr 7, 2026

View reviewed changes

jdbc-core/src/main/java/com/salesforce/datacloud/jdbc/core/ArrowStreamReaderCursor.java Outdated Show resolved Hide resolved

mkaufmann reviewed Apr 8, 2026

View reviewed changes

carsheng added 10 commits April 12, 2026 21:41

chore: remove and re-add comments for clarity

94adb25

fix: resultset is always readable=true

261f0fb

feat: remove nullConsumer and fix isReadOnly

adadb62

fix: don't return metadata when resultset is closed

78153f6

fix: add checkClosed to other methods

2f4f0c1

refactor: rename SimpleResultSetMetaData->DataCloudResultSetMetaData …

41c5292

…& SimpleMetadataResultSet-->DataCloudMetadataResultSet

refactor: rename DataCloudAccessor-->DataCloudQueryAccessor

57faca8

chore: re-add comments on how we resolve column names

64f223d

test: add missing tests for streamingresultSet

fc5a5bc

fix: update getObject() to no longer silently fail and return null

6cf0615

Conversation

carsheng commented Apr 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Apr 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carsheng Apr 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

carsheng commented Apr 6, 2026 •

edited

Loading

codecov bot commented Apr 6, 2026 •

edited

Loading

carsheng Apr 13, 2026 •

edited

Loading