Upgrade to Cassandra Java Driver 4.x - #302
Merged
Merged
Conversation
signing { sign publishing.publications } registered the .asc signature artifacts
unconditionally, so publishToMavenLocal failed for SNAPSHOT builds (and JitPack,
which builds the project without a GPG key) looking for a non-existent
tempto-core-<version>.jar.asc. Only attach signatures when actually signing
(release builds with signing enabled); release publishing is unchanged.
Contributor
Reviewer's GuideUpgrades Tempto to use the Apache Cassandra Java Driver 4.x by replacing the legacy Cluster/Session APIs with CqlSession, updating type mappings and metadata access patterns, adapting batch write logic to the new driver, and adjusting build/signing and example configs (driver coordinates, Cassandra Docker image, and configuration) for compatibility with newer Cassandra and JitPack builds. Flow diagram for Cassandra configuration into CqlSession builder and usageflowchart LR
Configuration -->|getStringMandatory host, getIntMandatory port, getString datacenter| CassandraQueryExecutor
CassandraQueryExecutor -->|CqlSession.builder addContactPoint withLocalDatacenter build| CqlSession
CqlSession -->|execute sql| QueryResult
CqlSession -->|prepare insertQuery| CassandraBatchLoader
CassandraBatchLoader -->|BatchStatementBuilder addStatement execute| CqlSession
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
msmygit
marked this pull request as ready for review
June 5, 2026 18:38
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="tempto-core/src/main/java/io/prestodb/tempto/internal/query/CassandraQueryExecutor.java" line_range="103-110" />
<code_context>
.map(definition -> getJDBCType(definition.getType()))
.collect(toList());
List<String> columnNames = definitions.stream()
- .map(ColumnDefinitions.Definition::getName)
+ .map(ColumnDefinition::getName)
+ .map(Object::toString)
.collect(toList());
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Prefer using CqlIdentifier#asInternal (or similar) over toString() for column names.
In the 4.x driver, `ColumnDefinition#getName` returns a `CqlIdentifier`, and `toString()` preserves quoting and case. This can diverge from the internal CQL name and break consumers expecting unquoted, lowercase column names. Please use `def.getName().asInternal()` (or the equivalent accessor in this driver version) instead of `Object::toString` when building `columnNames`.
```suggestion
List<JDBCType> types = definitions.stream()
.map(definition -> getJDBCType(definition.getType()))
.collect(toList());
List<String> columnNames = definitions.stream()
.map(definition -> definition.getName().asInternal())
.collect(toList());
```
</issue_to_address>
### Comment 2
<location path="tempto-core/src/main/java/io/prestodb/tempto/internal/query/CassandraQueryExecutor.java" line_range="171-172" />
<code_context>
-
- if (session == null || session.isClosed()) {
- session = cluster.connect();
+ if (session != null && !session.isClosed()) {
+ session.close();
}
}
</code_context>
<issue_to_address>
**nitpick:** The null check on a final session field is redundant.
Because `session` is a `final` field set in the constructor and never reassigned, it cannot be null here. This condition can be reduced to checking `!session.isClosed()` before closing, which also makes the lifecycle assumptions clearer.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
103
to
110
| List<JDBCType> types = definitions.stream() | ||
| .map(definition -> getJDBCType(definition.getType())) | ||
| .collect(toList()); | ||
|
|
||
| List<String> columnNames = definitions.stream() | ||
| .map(ColumnDefinitions.Definition::getName) | ||
| .map(ColumnDefinition::getName) | ||
| .map(Object::toString) | ||
| .collect(toList()); |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Prefer using CqlIdentifier#asInternal (or similar) over toString() for column names.
In the 4.x driver, ColumnDefinition#getName returns a CqlIdentifier, and toString() preserves quoting and case. This can diverge from the internal CQL name and break consumers expecting unquoted, lowercase column names. Please use def.getName().asInternal() (or the equivalent accessor in this driver version) instead of Object::toString when building columnNames.
Suggested change
| List<JDBCType> types = definitions.stream() | |
| .map(definition -> getJDBCType(definition.getType())) | |
| .collect(toList()); | |
| List<String> columnNames = definitions.stream() | |
| .map(ColumnDefinitions.Definition::getName) | |
| .map(ColumnDefinition::getName) | |
| .map(Object::toString) | |
| .collect(toList()); | |
| List<JDBCType> types = definitions.stream() | |
| .map(definition -> getJDBCType(definition.getType())) | |
| .collect(toList()); | |
| List<String> columnNames = definitions.stream() | |
| .map(definition -> definition.getName().asInternal()) | |
| .collect(toList()); |
Contributor
Author
There was a problem hiding this comment.
Analysis: the asInternal() suggestion and its repercussions
First, the mechanics behind the suggestion. In the 4.x driver, ColumnDefinition#getName() (and ColumnMetadata#getName(), TableMetadata#getName()) return a CqlIdentifier. Per CqlIdentifier.java:
- asInternal() → the raw name exactly as stored in the schema, never quoted, case preserved ("Foo" → Foo).
- toString() → defined as asCql(true), the pretty CQL form. asCql adds double-quotes whenever the identifier needs them — i.e. mixed case, a reserved keyword, or special characters. The CqlIdentifierTest shows this
directly: fromInternal("foo") → foo, but fromInternal("Foo") → "Foo" (quoted), and a reserved word like table → "table".
So the two only diverge for mixed-case / reserved / special-char identifiers. For a plain lowercase name (the overwhelmingly common case) asInternal() and toString() are byte-for-byte identical, so most existing
tests see no change at all.
The reviewer is also right that this is a regression from 3.x: there getName() returned a plain String (the unquoted internal name), which is exactly what asInternal() reproduces. toString() does not.
Repercussion 1 — the same pattern exists in three places, and the fix is not the same for all three
The suggestion patches only lines 107–110, but .map(Object::toString) on a CqlIdentifier also appears at:
- getColumnNames() (140–143)
- getTableNames() (162–165)
And here's the catch — asInternal() is not universally the right replacement, because these three feed very different consumers:
| Site | Consumer | What the name is used for | Correct form |
|---|---|---|---|
| executeQuery (107–110) | QueryResult.columnNamesIndexes → QueryAssert.column(String name) via tryFindColumnIndex | a lookup key compared against the bare name a test passes | asInternal() ✅ |
| getColumnNames (140–143) | CassandraBatchLoader.createInsertQuery → embedded into INSERT INTO t (col,...) VALUES(?) | a CQL fragment | needs quoting |
| getTableNames (162–165) | dropStaleMutableTables → DROP TABLE ks. | a CQL DDL fragment | needs quoting |
For the two CQL-embedding paths, switching to asInternal() would actively break mixed-case/reserved identifiers:
- A column whose internal name is Foo: asInternal() yields Foo, producing INSERT INTO t (Foo) VALUES(?). Cassandra lowercases unquoted identifiers, so this resolves to a nonexistent foo and the insert fails.
toString()/asCql() yields "Foo", which is the correct thing to splice into CQL.
So the reviewer's reasoning ("toString() can diverge and break consumers expecting unquoted names") is correct for the result-column path but inverted for the loader/DDL paths — there the quoting is exactly what you
want.
Repercussion 2 — the executeQuery change is actually a net positive beyond what the reviewer claims
For that one site, asInternal() doesn't just match style — it fixes a real latent bug. columnNames becomes the key set in QueryResult's bimap, and QueryAssert.column("myColumn", ...) looks them up by bare name. With
toString(), a query selecting a quoted/mixed-case column would be stored under "MyColumn" and the test's lookup by MyColumn would silently fail with "No column with name". asInternal() restores 3.x behavior and makes
the lookup work.
Recommendation
- Lines 107–110 (executeQuery): adopt the suggestion — definition.getName().asInternal(). Correct and an improvement.
- Lines 140–143 (getColumnNames) and 162–165 (getTableNames): do not blindly switch to asInternal(). These splice into CQL, so the safe choice is to keep a quoting-aware form. Cleanest is to make the intent explicit
with getName().asCql(true) (what toString() already does) rather than relying on Object::toString, or — more robustly — change CassandraBatchLoader/dropTable to take CqlIdentifiers and let the driver handle quoting.
If you only "fix" 107–110, the three sites become inconsistent for non-trivial identifiers, which is a subtle trap for future readers.
Practical blast radius
Because tempto's own mutable table names and typical test schemas use lowercase identifiers, all three forms coincide in practice today, so this is unlikely to change any current test outcome. The risk is entirely
around case-sensitive / quoted / reserved-word identifiers — which is precisely the scenario the reviewer is flagging, and precisely where applying their fix uniformly would do harm.
wanglinsong
approved these changes
Jun 8, 2026
wanglinsong
left a comment
wanglinsong
left a comment
Member
There was a problem hiding this comment.
Based on the discussion with @msmygit, the updates were verified.
unidevel
approved these changes
Jun 8, 2026
6 tasks
czentgr
pushed a commit
to prestodb/presto
that referenced
this pull request
Jun 22, 2026
## Description Upgrade to latest [Cassandra Java Driver 4.x](https://apache.github.io/cassandra-java-driver). Depends on `tempto` PR: prestodb/tempto#302 (which is already reviewed, merged, & released). ## Motivation and Context <!---Why is this change required? What problem does it solve?--> <!---If it fixes an open issue, please link to the issue here.--> Cassandra Java Driver `3.x` versions has reached EOL and is mostly not receiving any fixes or features. [**DataStax** *has* already donated it to the ASF and is spearheading the `4.x` series](https://groups.google.com/a/lists.datastax.com/g/java-driver-user/c/9JsrEJqaYOc/m/QJQQCj5MBQAJ). Resolves #26852 #26762 ## Impact <!---Describe any public API or user-facing feature change or any performance impact--> This will get presto to a more stable and future-forward and supported non-EOL Cassandra Java driver. ## Test Plan <!---Please fill in how you tested your change--> All relevant tests are now updated to leverage the latest Cassandra Java driver flavor ## Contributor checklist - [x ] Please make sure your submission complies with our [contributing guide](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md), in particular [code style](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#code-style) and [commit standards](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#commit-standards). - [x] PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced. - [x] Documented new properties (with its default value), SQL syntax, functions, or other functionality. - [x] If release notes are required, they follow the [release notes guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines). - [x] Adequate tests were added if applicable. - [x] CI passed. - [ ] If adding new dependencies, verified they have an [OpenSSF Scorecard](https://securityscorecards.dev/#the-checks) score of 5.0 or higher (or obtained explicit TSC approval for lower scores). ## Release Notes Please follow [release notes guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines) and fill in the release notes below. ``` == RELEASE NOTES == Cassandra Connector Changes * Upgrade to Cassandra Java Driver `4.x` ``` ## Summary by Sourcery Upgrade the Cassandra connector to use the Cassandra Java Driver 4.x and adapt the connector’s session, metadata, type handling, and configuration to the new driver APIs, including support for DataStax Astra secure connect bundles. New Features: - Add support for configuring Cassandra connections via DataStax Astra secure connect bundles. - Introduce a ReopeningSession wrapper to transparently reopen CqlSession instances on failure. Enhancements: - Refactor Cassandra client wiring to build CqlSession with programmatic driver configuration and updated retry, load-balancing, and speculative execution settings. - Update connector metadata, schema, and token range handling to the new driver 4.x metadata and token map model. - Adjust Cassandra type mappings and value conversions to the driver 4.x type system and Java time APIs. - Simplify configuration by removing deprecated protocol and whitelist settings and relying on driver 4.x defaults. - Update write path (page sink) and test utilities to use the new query builder and statement APIs. - Replace host-based replica/address handling with the new Node-based APIs and adapt split/token code accordingly. Build: - Replace the legacy DataStax driver dependency with the Apache Cassandra Java Driver 4.x core and query-builder modules, and add the LZ4 runtime and jsr305 annotations dependencies.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related presto PR with temp jitpack tests are at prestodb/presto#27029