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.

Sign up for GitHub

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

Jump to bottom

Add query table function for query pass-through to Cassandra connector #15973

Merged
merged 3 commits into from
Feb 15, 2023
Merged

Add query table function for query pass-through to Cassandra connector #15973

merged 3 commits into from
Feb 15, 2023

Conversation

ebyhr
Copy link
Member

@ebyhr ebyhr commented Feb 4, 2023 •
edited
Loading

Description

Add query table function for query pass-through to Cassandra connector

Release notes

(x) Release notes are required, with the following suggested text:

# Cassandra
* Add `query` table function for query pass-through to the connector. ({issue}`15973`)

@ebyhr ebyhr self-assigned this Feb 4, 2023
@cla-bot cla-bot bot added the cla-signed label Feb 4, 2023
@github-actions github-actions bot added the docs label Feb 4, 2023
@ebyhr ebyhr force-pushed the ebi/cassandra-query-pass-through branch from 094257d to 9a6a0d4 Compare February 4, 2023 09:43
@ebyhr ebyhr marked this pull request as ready for review February 4, 2023 10:07
@ebyhr ebyhr requested review from Praveen2112 and hashhar February 5, 2023 23:07
Praveen2112
Praveen2112 reviewed Feb 10, 2023
View reviewed changes
plugin/trino-cassandra/src/main/java/io/trino/plugin/cassandra/CassandraSplitManager.java Outdated Show resolved Hide resolved
plugin/trino-cassandra/src/main/java/io/trino/plugin/cassandra/CassandraTableHandle.java Outdated Show resolved Hide resolved
plugin/trino-cassandra/src/main/java/io/trino/plugin/cassandra/CassandraTableHandle.java Outdated Show resolved Hide resolved
plugin/trino-cassandra/src/main/java/io/trino/plugin/cassandra/CassandraTableHandle.java Outdated Show resolved Hide resolved
plugin/trino-cassandra/src/main/java/io/trino/plugin/cassandra/CassandraMetadata.java Outdated Show resolved Hide resolved
plugin/trino-cassandra/src/test/java/io/trino/plugin/cassandra/TestJsonCassandraHandles.java
@@ -37,20 +37,24 @@
public class TestJsonCassandraHandles
{
private static final Map<String, Object> TABLE_HANDLE_AS_MAP = ImmutableMap.of(
"schemaName", "cassandra_schema",
Copy link
Member

Choose a reason for hiding this comment

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

As a follow up - can we use assertJsonRoundTrip for testing these handles ?

plugin/trino-cassandra/src/test/java/io/trino/plugin/cassandra/TestCassandraConnectorTest.java Show resolved Hide resolved
@ebyhr
Copy link
Member Author

ebyhr commented Feb 10, 2023

Addressed comments.

@ebyhr ebyhr force-pushed the ebi/cassandra-query-pass-through branch from 650d3aa to 176e091 Compare February 10, 2023 11:12
Praveen2112
Praveen2112 reviewed Feb 10, 2023
View reviewed changes
plugin/trino-cassandra/src/main/java/io/trino/plugin/cassandra/CassandraMetadata.java
@@ -119,16 +125,31 @@ public CassandraTableHandle getTableHandle(ConnectorSession session, SchemaTable

private static SchemaTableName getTableName(ConnectorTableHandle tableHandle)
Copy link
Member

Choose a reason for hiding this comment

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

no changes requested : Maybe as a followup - can we inline the method - we could do the cast operation in method which invokes this one.

plugin/trino-cassandra/src/main/java/io/trino/plugin/cassandra/ptf/Query.java Outdated Show resolved Hide resolved
plugin/trino-cassandra/src/main/java/io/trino/plugin/cassandra/ptf/Query.java Show resolved Hide resolved
plugin/trino-cassandra/src/test/java/io/trino/plugin/cassandra/TestCassandraConnectorTest.java
Comment on lines +1409 to +1414
onCassandra("CREATE TABLE " + tableName + "(col BIGINT PRIMARY KEY)");

onCassandra("INSERT INTO " + tableName + "(col) VALUES (1)");
Copy link
Member

Choose a reason for hiding this comment

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

Can we use testTable so we don't have to have a dedicated query executor for cassandra and we don't have to drop the table.

Copy link
Member Author

@ebyhr ebyhr Feb 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Note that TestCassandraTable doesn't support single column with primary key. Using onCassandra is simpler than TestCassandraTable style.

Copy link
Member

Choose a reason for hiding this comment

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

In this case can we move dropping operations to finally block ?

Copy link
Member

Choose a reason for hiding this comment

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

Since we use onCassandra at a some places - Can we add support for TestCassandraTable with a single column primary key ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Let me handle in follow-up PR.

plugin/trino-cassandra/src/test/java/io/trino/plugin/cassandra/TestCassandraConnectorTest.java Show resolved Hide resolved
plugin/trino-cassandra/src/main/java/io/trino/plugin/cassandra/CassandraTableHandle.java Outdated Show resolved Hide resolved
plugin/trino-cassandra/src/main/java/io/trino/plugin/cassandra/CassandraSplitManager.java Outdated Show resolved Hide resolved
plugin/trino-cassandra/src/main/java/io/trino/plugin/cassandra/CassandraSplitManager.java Outdated Show resolved Hide resolved
plugin/trino-cassandra/src/main/java/io/trino/plugin/cassandra/CassandraSession.java Outdated
@@ -270,7 +270,7 @@ public CassandraTable getTable(SchemaTableName schemaTableName)
.sorted(comparing(CassandraColumnHandle::getOrdinalPosition))
.collect(toList());

CassandraTableHandle tableHandle = new CassandraTableHandle(tableMeta.getKeyspace().asInternal(), tableMeta.getName().asInternal());
CassandraTableHandle tableHandle = new CassandraTableHandle(new CassandraNamedRelationHandle(tableMeta.getKeyspace().asInternal(), tableMeta.getName().asInternal()));
Copy link
Member

Choose a reason for hiding this comment

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

What is your opinion on returning as CassandraNamedRelationHandle ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Could you elaborate on that?

Copy link
Member

Choose a reason for hiding this comment

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

Currently CassandraTable has two fields - CassandraTableHandle and List<CassandraColumnHandle. CassandraTableHandle is wrapper for both named and query based relation - which might be an overkill here. Instead can we have something like this
public CassandraTable(CassandraNamedRelationHandle tableHandle, List<CassandraColumnHandle> columns)

Just thinking out aloud.

plugin/trino-cassandra/src/main/java/io/trino/plugin/cassandra/CassandraRecordCursor.java Show resolved Hide resolved
@ebyhr ebyhr force-pushed the ebi/cassandra-query-pass-through branch from 176e091 to 45cb706 Compare February 10, 2023 13:54
@ebyhr
Copy link
Member Author

ebyhr commented Feb 10, 2023

Addressed comments.

@ebyhr
Copy link
Member Author

ebyhr commented Feb 13, 2023

I found a correctness issue when duplicated columns exist with different cases. e.g. col & COL. I'm fixing the issue now.

@ebyhr
Use name column matching in CassandraRecordCursor
4dda16b 
This is needed for upcoming query pass-through function.
@ebyhr ebyhr force-pushed the ebi/cassandra-query-pass-through branch from 45cb706 to 38fb388 Compare February 13, 2023 12:08
@ebyhr ebyhr requested a review from Praveen2112 February 13, 2023 21:40
Praveen2112
Praveen2112 approved these changes Feb 14, 2023
View reviewed changes
Copy link
Member

@Praveen2112 Praveen2112 left a comment

Choose a reason for hiding this comment

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

LGTM. Minor comments. Thanks for working on this.

plugin/trino-cassandra/src/main/java/io/trino/plugin/cassandra/CassandraMetadata.java Outdated Show resolved Hide resolved
plugin/trino-cassandra/src/main/java/io/trino/plugin/cassandra/CassandraMetadata.java Outdated Show resolved Hide resolved
plugin/trino-cassandra/src/main/java/io/trino/plugin/cassandra/CassandraMetadata.java Outdated Show resolved Hide resolved
plugin/trino-cassandra/src/main/java/io/trino/plugin/cassandra/CassandraSession.java Outdated
@@ -270,7 +270,7 @@ public CassandraTable getTable(SchemaTableName schemaTableName)
.sorted(comparing(CassandraColumnHandle::getOrdinalPosition))
.collect(toList());

CassandraTableHandle tableHandle = new CassandraTableHandle(tableMeta.getKeyspace().asInternal(), tableMeta.getName().asInternal());
CassandraTableHandle tableHandle = new CassandraTableHandle(new CassandraNamedRelationHandle(tableMeta.getKeyspace().asInternal(), tableMeta.getName().asInternal()));
Copy link
Member

Choose a reason for hiding this comment

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

Currently CassandraTable has two fields - CassandraTableHandle and List<CassandraColumnHandle. CassandraTableHandle is wrapper for both named and query based relation - which might be an overkill here. Instead can we have something like this
public CassandraTable(CassandraNamedRelationHandle tableHandle, List<CassandraColumnHandle> columns)

Just thinking out aloud.

plugin/trino-cassandra/src/test/java/io/trino/plugin/cassandra/TestCassandraConnectorTest.java
Comment on lines +1409 to +1414
onCassandra("CREATE TABLE " + tableName + "(col BIGINT PRIMARY KEY)");

onCassandra("INSERT INTO " + tableName + "(col) VALUES (1)");
Copy link
Member

Choose a reason for hiding this comment

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

In this case can we move dropping operations to finally block ?

plugin/trino-cassandra/src/test/java/io/trino/plugin/cassandra/TestCassandraConnectorTest.java Show resolved Hide resolved
plugin/trino-cassandra/src/test/java/io/trino/plugin/cassandra/TestCassandraConnectorTest.java
Comment on lines +1409 to +1414
onCassandra("CREATE TABLE " + tableName + "(col BIGINT PRIMARY KEY)");

onCassandra("INSERT INTO " + tableName + "(col) VALUES (1)");
Copy link
Member

Choose a reason for hiding this comment

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

Since we use onCassandra at a some places - Can we add support for TestCassandraTable with a single column primary key ?

plugin/trino-cassandra/src/test/java/io/trino/plugin/cassandra/TestCassandraConnectorTest.java Show resolved Hide resolved
@ebyhr ebyhr force-pushed the ebi/cassandra-query-pass-through branch from 38fb388 to f825e49 Compare February 15, 2023 04:12
@ebyhr ebyhr merged commit f370b13 into trinodb:master Feb 15, 2023
@ebyhr ebyhr deleted the ebi/cassandra-query-pass-through branch February 15, 2023 09:06
@ebyhr ebyhr mentioned this pull request Feb 15, 2023
Closed
@github-actions github-actions bot added this to the 407 milestone Feb 15, 2023
@colebow colebow mentioned this pull request Feb 16, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Praveen2112 Praveen2112 Praveen2112 approved these changes

@hashhar hashhar Awaiting requested review from hashhar

Assignees

@ebyhr ebyhr

Labels
cla-signed docs
Milestone
407
Development

Successfully merging this pull request may close these issues.
2 participants
@ebyhr @Praveen2112