Skip to content
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

Fix JdbcRecordCursor.isNull for types mapped to slice #5711

Closed
wants to merge 1 commit into from

Conversation

elonazoulay
Copy link
Member

This fixes an issue when the postgresql plugin is
used to connect to cockroachdb: https://go.crdb.dev/issue-v/40195/v20.1

This fixes an issue when the postgresql plugin is
used to connect to cockroachdb: https://go.crdb.dev/issue-v/40195/v20.1
@cla-bot cla-bot bot added the cla-signed label Oct 27, 2020
electrum
electrum previously approved these changes Oct 27, 2020
@electrum
Copy link
Member

Is that link right? I don't see how it relates to this issue.

@electrum
Copy link
Member

I'm not sure this is the correct fix. There are various types mapped to Slice including long decimal, varbinary, etc., for which calling getString() actually seems less appropriate than getObject(). Which data types require calling getString() rather than getObject()? It might be better to add a specific PostgreSQL fix for just those types.

// JDBC is kind of dumb: we need to read the field and then ask
// if it was null, which means we are wasting effort here.
// We could save the result of the field access if it matters.
resultSet.getString(columnIndex);
Copy link
Member

Choose a reason for hiding this comment

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

I do not see a reason why resultSet.getString() should be better than resultSet.getObject().
The code is redundant and can easily be accidentally removed with a refactor.
Can we have a test for this?

Also, if this is cockrach specific behavior, the correct approach is to provide SliceReadFunction and implement isNull for that case.

@electrum electrum self-requested a review October 27, 2020 20:20
@electrum electrum dismissed their stale review October 27, 2020 20:20

See comments

@findepi
Copy link
Member

findepi commented Oct 27, 2020

@elonazoulay would it be possible to have a real cockroachdb connector?
For example, we have a a Redshift connector even though PostgreSQL connector "somewhat works" for Redshift.
Or, we're adding Yugabyte connector even though Cassandra connector "mostly works" for Yugabyte (#5352).
I think Cockroach DB is a similar story.

@findepi
Copy link
Member

findepi commented Oct 27, 2020

It seems it should be pretty straightforward to test too, given https://www.testcontainers.org/modules/databases/cockroachdb/ exists.

@elonazoulay
Copy link
Member Author

@findepi, yes! We were thinking to add time travel as well, so queries via the presto connector minimize contention with operational queries: https://www.cockroachlabs.com/docs/stable/as-of-system-time.html

@mosabua
Copy link
Member

mosabua commented Oct 20, 2022

👋 @elonazoulay - this PR has become inactive. If you're still interested in working on it, please let us know, and we can try to get reviewers to help with that.

Also I started a cockroachdb connector PR, but it became stale. Anyone interested can take it over or start again. See https://github.com/simpligility/trino/tree/cockroachdb and #13771 and #8317

We're working on closing out old and inactive PRs, so if you're too busy or this has too many merge conflicts to be worth picking back up, we'll be making another pass to close it out in a few weeks.

@colebow colebow closed this Nov 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants